On Thu, Aug 19, 2021 at 6:53 PM James Carter <jwcart2@xxxxxxxxx> 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@xxxxxxxxx>
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@xxxxxxx>
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
>
