On Fri, Jul 9, 2021 at 4:11 PM James Carter <jwcart2@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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?
Cheers,
Nicolas
