* [PATCH 1/2] libsepol/cil: Remove redundant syntax checking
@ 2021-08-19 16:53 James Carter
2021-08-19 16:53 ` [PATCH 2/2] libsepol/cil: Fix syntax checking in __cil_verify_syntax() James Carter
0 siblings, 1 reply; 3+ messages in thread
From: James Carter @ 2021-08-19 16:53 UTC (permalink / raw)
To: selinux; +Cc: nicolas.iooss, James Carter
For every call to cil_fill_classperms_list(), the syntax of the
whole rule, including the class permissions, has already been
checked. There is no reason to check it again. Also, because the
class permissions appear in the middle of some rules, like
constraints, the syntax array does not end with CIL_SYN_END. This
is the only case where the syntax array does not end with CIL_SYN_END.
This prevents __cil_verify_syntax() from requiring that the syntax
array ends with CIL_SYN_END.
Remove the redundant syntax checking in cil_fill_classperms_list().
Signed-off-by: James Carter <jwcart2@gmail.com>
---
libsepol/cil/src/cil_build_ast.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 9da90883..514cac8d 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -736,20 +736,11 @@ int cil_fill_classperms_list(struct cil_tree_node *parse_current, struct cil_lis
{
int rc = SEPOL_ERR;
struct cil_tree_node *curr;
- enum cil_syntax syntax[] = {
- CIL_SYN_STRING | CIL_SYN_LIST,
- };
- int syntax_len = sizeof(syntax)/sizeof(*syntax);
if (parse_current == NULL || cp_list == NULL) {
goto exit;
}
- rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
- if (rc != SEPOL_OK) {
- goto exit;
- }
-
cil_list_init(cp_list, CIL_CLASSPERMS);
curr = parse_current->cl_head;
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] libsepol/cil: Fix syntax checking in __cil_verify_syntax()
2021-08-19 16:53 [PATCH 1/2] libsepol/cil: Remove redundant syntax checking James Carter
@ 2021-08-19 16:53 ` James Carter
2021-09-01 19:20 ` Nicolas Iooss
0 siblings, 1 reply; 3+ messages in thread
From: James Carter @ 2021-08-19 16:53 UTC (permalink / raw)
To: selinux; +Cc: nicolas.iooss, James Carter
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH 2/2] libsepol/cil: Fix syntax checking in __cil_verify_syntax()
2021-08-19 16:53 ` [PATCH 2/2] libsepol/cil: Fix syntax checking in __cil_verify_syntax() James Carter
@ 2021-09-01 19:20 ` Nicolas Iooss
0 siblings, 0 replies; 3+ messages in thread
From: Nicolas Iooss @ 2021-09-01 19:20 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-01 19:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-19 16:53 [PATCH 1/2] libsepol/cil: Remove redundant syntax checking James Carter
2021-08-19 16:53 ` [PATCH 2/2] libsepol/cil: Fix syntax checking in __cil_verify_syntax() James Carter
2021-09-01 19:20 ` Nicolas Iooss
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox