* [PATCH] libsepol/cil: Fix handling category sets in an expression
@ 2021-07-09 14:11 James Carter
2021-07-12 7:00 ` Nicolas Iooss
0 siblings, 1 reply; 3+ messages in thread
From: James Carter @ 2021-07-09 14:11 UTC (permalink / raw)
To: selinux; +Cc: James Carter
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>
---
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;
+ }
cil_list_append(*datum_expr, curr->flavor, curr->data);
break;
- }
+ }
}
return SEPOL_OK;
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] libsepol/cil: Fix handling category sets in an expression
2021-07-09 14:11 [PATCH] libsepol/cil: Fix handling category sets in an expression James Carter
@ 2021-07-12 7:00 ` Nicolas Iooss
2021-07-12 13:13 ` James Carter
0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Iooss @ 2021-07-12 7:00 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list
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?
Cheers,
Nicolas
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libsepol/cil: Fix handling category sets in an expression
2021-07-12 7:00 ` Nicolas Iooss
@ 2021-07-12 13:13 ` James Carter
0 siblings, 0 replies; 3+ messages in thread
From: James Carter @ 2021-07-12 13:13 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-07-12 13:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-09 14:11 [PATCH] libsepol/cil: Fix handling category sets in an expression James Carter
2021-07-12 7:00 ` Nicolas Iooss
2021-07-12 13:13 ` James Carter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.