Re: [PATCH v2] libsepol/cil: Fix handling category sets in an expression

[Nicolas Iooss <nicolas.iooss@m4x.org>]

On Mon, Jul 12, 2021 at 3:50 PM James Carter <jwcart2@gmail.com> wrote:
>
> There are two problems that need to be addressed when resolving an
> expression with category sets.
>
> 1. Only expand anonymous category sets in an expression.
>
> Commit 982ec302b67f3c7f8df667dadb67352b1e4a6d18 (libsepol/cil:
> Account for anonymous category sets in an expression) attempted to
> properly handle anonymous category sets when resolving category
> expressions. Unfortunately, it did not check whether a category set
> was actually an anonymous category set and expanded all category
> sets in an expression. If a category set refers to itself in the
> expression, then everything from the name of the category set to the
> end of the expression is ignored.
>
> For example, the rule "(categoryset cs (c0 cs c1 c2))", would be
> equivalent to the rule "(categoryset cs (c0))" as everything from
> "cs" to the end would be dropped. The secilc-fuzzer found that the
> rule "(categoryset cat (not cat))" would cause a segfault since
> "(not)" is not a valid expression and it is assumed to be valid
> during later evaluation because syntax checking has already been
> done.
>
> Instead, check whether or not the category set is anonymous before
> expanding it when resolving an expression.
>
> 2. Category sets cannot be used in a category range
>
> A category range can be used to specify a large number of categories.
> The range "(range c0 c1023)" refers to 1024 categories. Only categories
> and category aliases can be used in a range. Determining if an
> identifier is a category, an alias, or a set can only be done after
> resolving the identifer.
>
> Keep track of the current operator as an expression is being resolved
> and if the expression involves categories and a category set is
> encountered, then return an error if the expression is a category
> range.
>
> Signed-off-by: James Carter <jwcart2@gmail.com>

Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Thanks!
Nicolas

> ---
>  libsepol/cil/src/cil_resolve_ast.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 145d4e74..18007324 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -3228,6 +3228,7 @@ int cil_resolve_expr(enum cil_flavor expr_type, struct cil_list *str_expr, struc
>         struct cil_symtab_datum *res_datum = NULL;
>         enum cil_sym_index sym_index =  CIL_SYM_UNKNOWN;
>         struct cil_list *datum_sub_expr;
> +       enum cil_flavor op = CIL_NONE;
>
>         switch (str_expr->flavor) {
>         case CIL_BOOL:
> @@ -3263,14 +3264,24 @@ int cil_resolve_expr(enum cil_flavor expr_type, struct cil_list *str_expr, struc
>                         }
>                         if (sym_index == CIL_SYM_CATS && NODE(res_datum)->flavor == CIL_CATSET) {
>                                 struct cil_catset *catset = (struct cil_catset *)res_datum;
> -                               if (!catset->cats->datum_expr) {
> -                                       rc = cil_resolve_expr(expr_type, catset->cats->str_expr, &catset->cats->datum_expr, parent, extra_args);
> -                                       if (rc != SEPOL_OK) {
> -                                               goto exit;
> +                               if (op == CIL_RANGE) {
> +                                       cil_tree_log(parent, CIL_ERR, "Category set not allowed in category range");
> +                                       rc = SEPOL_ERR;
> +                                       goto exit;
> +                               }
> +                               if (!res_datum->name) {
> +                                       /* Anonymous category sets need to be resolved when encountered */
> +                                       if (!catset->cats->datum_expr) {
> +                                               rc = cil_resolve_expr(expr_type, catset->cats->str_expr, &catset->cats->datum_expr, parent, extra_args);
> +                                               if (rc != SEPOL_OK) {
> +                                                       goto exit;
> +                                               }
>                                         }
> +                                       cil_copy_list(catset->cats->datum_expr, &datum_sub_expr);
> +                                       cil_list_append(*datum_expr, CIL_LIST, datum_sub_expr);
> +                               } else {
> +                                       cil_list_append(*datum_expr, CIL_DATUM, res_datum);
>                                 }
> -                               cil_copy_list(catset->cats->datum_expr, &datum_sub_expr);
> -                               cil_list_append(*datum_expr, CIL_LIST, datum_sub_expr);
>                         } else {
>                                 if (sym_index == CIL_SYM_TYPES && (expr_type == CIL_CONSTRAIN || expr_type == CIL_VALIDATETRANS)) {
>                                         cil_type_used(res_datum, CIL_ATTR_CONSTRAINT);
> @@ -3287,9 +3298,12 @@ int cil_resolve_expr(enum cil_flavor expr_type, struct cil_list *str_expr, struc
>                         break;
>                 }
>                 default:
> +                       if (curr->flavor == CIL_OP) {
> +                               op = (enum cil_flavor)(uintptr_t)curr->data;
> +                       }
>                         cil_list_append(*datum_expr, curr->flavor, curr->data);
>                         break;
> -               }
> +               }
>         }
>         return SEPOL_OK;
>
> --
> 2.31.1
>