On Tue, Oct 20, 2020 at 8:07 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> On Thu, Oct 15, 2020 at 10:44 PM James Carter <jwcart2@xxxxxxxxx> wrote:
> > Both tunableif and booleanif use conditional blocks (either true or
> > false). No ordering is imposed, so a false block can be first (or even
> > the only) block. Checks are made to ensure that the first and second
> > (if it exists) blocks are either true or false, but no checks are made
> > to ensure that there is only one true and/or one false block. If there
> > are more than one true or false block, only the first will be used and
> > the other will be ignored.
> >
> > Create a function, cil_verify_conditional_blocks(), that gives an error
> > along with a message if more than one true or false block is specified
> > and call that function when building tunableif and booleanif blocks in
> > the AST.
> >
> > Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> > ---
> > libsepol/cil/src/cil_build_ast.c | 44 +++++---------------------------
> > libsepol/cil/src/cil_verify.c | 35 +++++++++++++++++++++++++
> > libsepol/cil/src/cil_verify.h | 1 +
> > 3 files changed, 42 insertions(+), 38 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> > index 3aabb05e..a8955834 100644
> > --- a/libsepol/cil/src/cil_build_ast.c
> > +++ b/libsepol/cil/src/cil_build_ast.c
> > @@ -2821,7 +2821,6 @@ int cil_gen_boolif(struct cil_db *db, struct cil_tree_node *parse_current, struc
> > int syntax_len = sizeof(syntax)/sizeof(*syntax);
> > struct cil_booleanif *bif = NULL;
> > struct cil_tree_node *next = NULL;
> > - struct cil_tree_node *cond = NULL;
> > int rc = SEPOL_ERR;
> >
> > if (db == NULL || parse_current == NULL || ast_node == NULL) {
> > @@ -2841,27 +2840,12 @@ int cil_gen_boolif(struct cil_db *db, struct cil_tree_node *parse_current, struc
> > goto exit;
> > }
> >
> > - cond = parse_current->next->next;
> > -
> > - /* Destroying expr tree after stack is created*/
> > - if (cond->cl_head->data != CIL_KEY_CONDTRUE &&
> > - cond->cl_head->data != CIL_KEY_CONDFALSE) {
> > - rc = SEPOL_ERR;
> > - cil_log(CIL_ERR, "Conditional neither true nor false\n");
> > + rc = cil_verify_conditional_blocks(parse_current->next->next);
> > + if (rc != SEPOL_OK) {
> > goto exit;
> > }
> >
> > - if (cond->next != NULL) {
> > - cond = cond->next;
> > - if (cond->cl_head->data != CIL_KEY_CONDTRUE &&
> > - cond->cl_head->data != CIL_KEY_CONDFALSE) {
> > - rc = SEPOL_ERR;
> > - cil_log(CIL_ERR, "Conditional neither true nor false\n");
> > - goto exit;
> > - }
> > - }
> > -
> > -
> > + /* Destroying expr tree */
> > next = parse_current->next->next;
> > cil_tree_subtree_destroy(parse_current->next);
> > parse_current->next = next;
> > @@ -2905,7 +2889,6 @@ int cil_gen_tunif(struct cil_db *db, struct cil_tree_node *parse_current, struct
> > int syntax_len = sizeof(syntax)/sizeof(*syntax);
> > struct cil_tunableif *tif = NULL;
> > struct cil_tree_node *next = NULL;
> > - struct cil_tree_node *cond = NULL;
> > int rc = SEPOL_ERR;
> >
> > if (db == NULL || parse_current == NULL || ast_node == NULL) {
> > @@ -2924,27 +2907,12 @@ int cil_gen_tunif(struct cil_db *db, struct cil_tree_node *parse_current, struct
> > goto exit;
> > }
> >
> > - cond = parse_current->next->next;
> > -
> > - if (cond->cl_head->data != CIL_KEY_CONDTRUE &&
> > - cond->cl_head->data != CIL_KEY_CONDFALSE) {
> > - rc = SEPOL_ERR;
> > - cil_log(CIL_ERR, "Conditional neither true nor false\n");
> > + rc = cil_verify_conditional_blocks(parse_current->next->next);
> > + if (rc != SEPOL_OK) {
> > goto exit;
> > }
> >
> > - if (cond->next != NULL) {
> > - cond = cond->next;
> > -
> > - if (cond->cl_head->data != CIL_KEY_CONDTRUE &&
> > - cond->cl_head->data != CIL_KEY_CONDFALSE) {
> > - rc = SEPOL_ERR;
> > - cil_log(CIL_ERR, "Conditional neither true nor false\n");
> > - goto exit;
> > - }
> > - }
> > -
> > - /* Destroying expr tree after stack is created*/
> > + /* Destroying expr tree */
> > next = parse_current->next->next;
> > cil_tree_subtree_destroy(parse_current->next);
> > parse_current->next = next;
> > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
> > index c73bbeee..7b3d2a8b 100644
> > --- a/libsepol/cil/src/cil_verify.c
> > +++ b/libsepol/cil/src/cil_verify.c
> > @@ -324,6 +324,41 @@ exit:
> > return SEPOL_ERR;
> > }
> >
> > +int cil_verify_conditional_blocks(struct cil_tree_node *current)
> > +{
> > + int found_true = CIL_FALSE;
> > + int found_false = CIL_FALSE;
> > +
> > + if (current->cl_head->data == CIL_KEY_CONDTRUE) {
> > + found_true = CIL_TRUE;
> > + } else if (current->cl_head->data == CIL_KEY_CONDFALSE) {
> > + found_false = CIL_TRUE;
> > + } else {
> > + cil_tree_log(current,CIL_ERR,"Expected true or false block in conditional");
>
> Please put a space after each comma in the argument list (same in the
> other cil_tree_log() calls in this function).
>
Sure, no problem.
> > + return SEPOL_ERR;
> > + }
> > +
> > + current = current->next;
> > + if (current != NULL) {
> > + if (current->cl_head->data == CIL_KEY_CONDTRUE) {
> > + if (found_true) {
> > + cil_tree_log(current,CIL_ERR,"More than one true block in conditional");
> > + return SEPOL_ERR;
> > + }
> > + } else if (current->cl_head->data == CIL_KEY_CONDFALSE) {
> > + if (found_false) {
> > + cil_tree_log(current,CIL_ERR,"More than one false block in conditional");
> > + return SEPOL_ERR;
> > + }
> > + } else {
> > + cil_tree_log(current,CIL_ERR,"Expected true or false block in conditional");
> > + return SEPOL_ERR;
> > + }
>
> Perhaps this is checked somewhere else or I'm missing something, but
> shouldn't this function also fail if there are more than two blocks
> (i.e. current->next != NULL here)? Not that it would cause any damage
> (I guess the extra blocks would be just ignored), but IMHO it's better
> to be strict about it.
More than two blocks will fail the syntax check, so no additional
checks are needed.
I'll send an updated patch.
Thanks,
Jim
>
> > + }
> > +
> > + return SEPOL_OK;
> > +}
> > +
> > int cil_verify_no_self_reference(struct cil_symtab_datum *datum, struct cil_list *datum_list)
> > {
> > struct cil_list_item *i;
> > diff --git a/libsepol/cil/src/cil_verify.h b/libsepol/cil/src/cil_verify.h
> > index bda1565f..905761b0 100644
> > --- a/libsepol/cil/src/cil_verify.h
> > +++ b/libsepol/cil/src/cil_verify.h
> > @@ -61,6 +61,7 @@ int __cil_verify_syntax(struct cil_tree_node *parse_current, enum cil_syntax s[]
> > int cil_verify_expr_syntax(struct cil_tree_node *current, enum cil_flavor op, enum cil_flavor expr_flavor);
> > int cil_verify_constraint_leaf_expr_syntax(enum cil_flavor l_flavor, enum cil_flavor r_flavor, enum cil_flavor op, enum cil_flavor expr_flavor);
> > int cil_verify_constraint_expr_syntax(struct cil_tree_node *current, enum cil_flavor op);
> > +int cil_verify_conditional_blocks(struct cil_tree_node *current);
> > int cil_verify_no_self_reference(struct cil_symtab_datum *datum, struct cil_list *datum_list);
> > int __cil_verify_ranges(struct cil_list *list);
> > int __cil_verify_ordered_node_helper(struct cil_tree_node *node, uint32_t *finished, void *extra_args);
> > --
> > 2.25.4
> >
>
> --
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.
>
