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).
> + 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
>
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
