| Message ID | 20210819165332.58896-2-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] libsepol/cil: Remove redundant syntax checking | expand |
On Thu, Aug 19, 2021 at 6:53 PM James Carter <jwcart2@gmail.com> wrote: > > The function __cil_verify_syntax() is used to check the syntax of > CIL rules (and a few other common things like contexts and class > permissions). It does not correctly check the syntax combination > "CIL_SYN_STRING | CIL_SYN_N_LISTS, CIL_SYN_N_LISTS | CIL_SYN_END". > This should mean either a string followed by any number of lists > or any number of lists followed by the end of the rule. Instead, > while allowing the correct syntax, it allows any number of lists > followed by a string followed by any number of more lists followed > by the end of the rule and, also, any number of lists followed by a > string followed by the end of the rule. > > Refactor the function to make it clearer to follow and so that once > checking begins for CIL_SYN_N_LISTS or CIL_SYN_N_STRINGS, then only > strings or lists are allowed until the end of the rule is found. In > addition, always check for CIL_SYN_END at the end. > > Signed-off-by: James Carter <jwcart2@gmail.com> Hello, This looks much clearer, thanks! I have two minor comments: * I find "if (s[i] & CIL_SYN_... && ...)" harder to read than "if ((s[i] & CIL_SYM_...) && ...)" and I would prefer using parenthesis around the bitwise operations. * The variables i and len can be "unsigned int" instead of "int" (or even "size_t", all the more when the length is computed with "syntax_len = sizeof(syntax)/sizeof(*syntax);" in one caller of __cil_verify_syntax). As these comments are more about making the code clearer to my mind than fixing things, I do not consider them to be blocker. Feel free to apply this patch without change or to send another version. For these 2 patches: Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org> Thanks, Nicolas > --- > libsepol/cil/src/cil_verify.c | 71 ++++++++++++----------------------- > 1 file changed, 23 insertions(+), 48 deletions(-) > > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c > index fc8a8a40..b1c2270e 100644 > --- a/libsepol/cil/src/cil_verify.c > +++ b/libsepol/cil/src/cil_verify.c > @@ -146,68 +146,43 @@ exit: > > int __cil_verify_syntax(struct cil_tree_node *parse_current, enum cil_syntax s[], int len) > { > - int rc = SEPOL_ERR; > - int num_extras = 0; > struct cil_tree_node *c = parse_current; > int i = 0; > - while (i < len) { > - if ((s[i] & CIL_SYN_END) && c == NULL) { > - break; > - } > > - if (s[i] & CIL_SYN_N_LISTS || s[i] & CIL_SYN_N_STRINGS) { > - if (c == NULL) { > - if (num_extras > 0) { > - i++; > - continue; > + while (i < len && c != NULL) { > + if (s[i] & CIL_SYN_STRING && c->data != NULL && c->cl_head == NULL) { > + c = c->next; > + i++; > + } else if (s[i] & CIL_SYN_LIST && c->data == NULL && c->cl_head != NULL) { > + c = c->next; > + i++; > + } else if (s[i] & CIL_SYN_EMPTY_LIST && c->data == NULL && c->cl_head == NULL) { > + c = c->next; > + i++; > + } else if (s[i] & CIL_SYN_N_LISTS || s[i] & CIL_SYN_N_STRINGS) { > + while (c != NULL) { > + if (s[i] & CIL_SYN_N_LISTS && c->data == NULL && c->cl_head != NULL) { > + c = c->next; > + } else if (s[i] & CIL_SYN_N_STRINGS && c->data != NULL && c->cl_head == NULL) { > + c = c->next; > } else { > goto exit; > } > - } else if ((s[i] & CIL_SYN_N_LISTS) && (c->data == NULL && c->cl_head != NULL)) { > - c = c->next; > - num_extras++; > - continue; > - } else if ((s[i] & CIL_SYN_N_STRINGS) && (c->data != NULL && c->cl_head == NULL)) { > - c = c->next; > - num_extras++; > - continue; > } > - } > - > - if (c == NULL) { > + i++; > + break; /* Only CIL_SYN_END allowed after these */ > + } else { > goto exit; > } > + } > > - if (s[i] & CIL_SYN_STRING) { > - if (c->data != NULL && c->cl_head == NULL) { > - c = c->next; > - i++; > - continue; > - } > - } > - > - if (s[i] & CIL_SYN_LIST) { > - if (c->data == NULL && c->cl_head != NULL) { > - c = c->next; > - i++; > - continue; > - } > - } > - > - if (s[i] & CIL_SYN_EMPTY_LIST) { > - if (c->data == NULL && c->cl_head == NULL) { > - c = c->next; > - i++; > - continue; > - } > - } > - goto exit; > + if (i < len && s[i] & CIL_SYN_END && c == NULL) { > + return SEPOL_OK; > } > - return SEPOL_OK; > > exit: > cil_log(CIL_ERR, "Invalid syntax\n"); > - return rc; > + return SEPOL_ERR; > } > > int cil_verify_expr_syntax(struct cil_tree_node *current, enum cil_flavor op, enum cil_flavor expr_flavor) > -- > 2.31.1 >
diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c index fc8a8a40..b1c2270e 100644 --- a/libsepol/cil/src/cil_verify.c +++ b/libsepol/cil/src/cil_verify.c @@ -146,68 +146,43 @@ exit: int __cil_verify_syntax(struct cil_tree_node *parse_current, enum cil_syntax s[], int len) { - int rc = SEPOL_ERR; - int num_extras = 0; struct cil_tree_node *c = parse_current; int i = 0; - while (i < len) { - if ((s[i] & CIL_SYN_END) && c == NULL) { - break; - } - if (s[i] & CIL_SYN_N_LISTS || s[i] & CIL_SYN_N_STRINGS) { - if (c == NULL) { - if (num_extras > 0) { - i++; - continue; + while (i < len && c != NULL) { + if (s[i] & CIL_SYN_STRING && c->data != NULL && c->cl_head == NULL) { + c = c->next; + i++; + } else if (s[i] & CIL_SYN_LIST && c->data == NULL && c->cl_head != NULL) { + c = c->next; + i++; + } else if (s[i] & CIL_SYN_EMPTY_LIST && c->data == NULL && c->cl_head == NULL) { + c = c->next; + i++; + } else if (s[i] & CIL_SYN_N_LISTS || s[i] & CIL_SYN_N_STRINGS) { + while (c != NULL) { + if (s[i] & CIL_SYN_N_LISTS && c->data == NULL && c->cl_head != NULL) { + c = c->next; + } else if (s[i] & CIL_SYN_N_STRINGS && c->data != NULL && c->cl_head == NULL) { + c = c->next; } else { goto exit; } - } else if ((s[i] & CIL_SYN_N_LISTS) && (c->data == NULL && c->cl_head != NULL)) { - c = c->next; - num_extras++; - continue; - } else if ((s[i] & CIL_SYN_N_STRINGS) && (c->data != NULL && c->cl_head == NULL)) { - c = c->next; - num_extras++; - continue; } - } - - if (c == NULL) { + i++; + break; /* Only CIL_SYN_END allowed after these */ + } else { goto exit; } + } - if (s[i] & CIL_SYN_STRING) { - if (c->data != NULL && c->cl_head == NULL) { - c = c->next; - i++; - continue; - } - } - - if (s[i] & CIL_SYN_LIST) { - if (c->data == NULL && c->cl_head != NULL) { - c = c->next; - i++; - continue; - } - } - - if (s[i] & CIL_SYN_EMPTY_LIST) { - if (c->data == NULL && c->cl_head == NULL) { - c = c->next; - i++; - continue; - } - } - goto exit; + if (i < len && s[i] & CIL_SYN_END && c == NULL) { + return SEPOL_OK; } - return SEPOL_OK; exit: cil_log(CIL_ERR, "Invalid syntax\n"); - return rc; + return SEPOL_ERR; } int cil_verify_expr_syntax(struct cil_tree_node *current, enum cil_flavor op, enum cil_flavor expr_flavor)
The function __cil_verify_syntax() is used to check the syntax of CIL rules (and a few other common things like contexts and class permissions). It does not correctly check the syntax combination "CIL_SYN_STRING | CIL_SYN_N_LISTS, CIL_SYN_N_LISTS | CIL_SYN_END". This should mean either a string followed by any number of lists or any number of lists followed by the end of the rule. Instead, while allowing the correct syntax, it allows any number of lists followed by a string followed by any number of more lists followed by the end of the rule and, also, any number of lists followed by a string followed by the end of the rule. Refactor the function to make it clearer to follow and so that once checking begins for CIL_SYN_N_LISTS or CIL_SYN_N_STRINGS, then only strings or lists are allowed until the end of the rule is found. In addition, always check for CIL_SYN_END at the end. Signed-off-by: James Carter <jwcart2@gmail.com> --- libsepol/cil/src/cil_verify.c | 71 ++++++++++++----------------------- 1 file changed, 23 insertions(+), 48 deletions(-)