| Message ID | 20201015204352.569018-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | libsepol/cil: Give error for more than one true or false block | expand |
On Thu, Oct 15, 2020 at 10:44 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > --- > 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). > + 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. > + } > + > + 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 >
On Tue, Oct 20, 2020 at 8:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Oct 15, 2020 at 10:44 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > > --- > > 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. >
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"); + 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; + } + } + + 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);
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@gmail.com> --- 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(-)