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

[James Carter <jwcart2@gmail.com>]

On Mon, Jul 12, 2021 at 3:00 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Fri, Jul 9, 2021 at 4:11 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.
>
> Misspelling: identifer -> identifier
>
> >
> > 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>
> > ---
> >  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..3d3ef955 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)curr->data;
> > +                       }
>
> Hello,
> This patch looks good to me, and it actually also fixes
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=35494&q=selinux&can=2
> (memory leak when compiling "(categoryset e(u)) (categoryset u(e))",
> because the categorysets were expanded).
>
> Nevertheless when building with clang 12, I get the following warning:
>
> ../cil/src/cil_resolve_ast.c:3308:10: error: cast to smaller integer
> type 'enum cil_flavor' from 'void *'
> [-Werror,-Wvoid-pointer-to-enum-cast]
>                                 op = (enum cil_flavor)curr->data;
>                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This issue can be fixed by introducing an intermediate cast to
> uintptr_t, like what was done in commit 32f8ed3d6b0b ("libsepol/cil:
> introduce intermediate cast to silence -Wvoid-pointer-to-enum-cast"):
>
>     op = (enum cil_flavor)(uintptr_t)curr->data;
>
> If this looks good to you, could you please fix this?
>

I will send out a v2 with the fix.
Thanks for the review,
Jim

> Cheers,
> Nicolas
>