[PATCH 1/4] libsepol/cil: remove useless print statement

[PATCH 1/4] libsepol/cil: remove useless print statement

[Nicolas Iooss]

cil_copy_expandtypeattribute prints "cil_copy_expandtypeattribute 656"
which is quite annoying. Remove the fprintf statement responsible for
this.

While at it, remove another one in cil_tree_print_node()

Fixes: https://lore.kernel.org/selinux/3c2ab876-b0b7-42eb-573d-e5b450a7125a@gmail.com/T/#u
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_copy_ast.c | 1 -
 libsepol/cil/src/cil_tree.c     | 1 -
 2 files changed, 2 deletions(-)

diff --git a/libsepol/cil/src/cil_copy_ast.c b/libsepol/cil/src/cil_copy_ast.c
index 67dd8528f526..c9aada9db348 100644
--- a/libsepol/cil/src/cil_copy_ast.c
+++ b/libsepol/cil/src/cil_copy_ast.c
@@ -653,7 +653,6 @@ int cil_copy_expandtypeattribute(__attribute__((unused)) struct cil_db *db, void
 	struct cil_expandtypeattribute *orig = data;
 	struct cil_expandtypeattribute *new = NULL;
 
-	fprintf(stderr, "%s %u\n", __func__, __LINE__);
 	cil_expandtypeattribute_init(&new);
 
 	if (orig->attr_strs != NULL) {
diff --git a/libsepol/cil/src/cil_tree.c b/libsepol/cil/src/cil_tree.c
index 3ab5e868d15e..886412d1b974 100644
--- a/libsepol/cil/src/cil_tree.c
+++ b/libsepol/cil/src/cil_tree.c
@@ -710,7 +710,6 @@ void cil_tree_print_node(struct cil_tree_node *node)
 		case CIL_EXPANDTYPEATTRIBUTE: {
 			struct cil_expandtypeattribute *attr = node->data;
 
-			fprintf(stderr, "%s %u\n", __func__, __LINE__);
 			cil_log(CIL_INFO, "(EXPANDTYPEATTRIBUTE ");
 			cil_tree_print_expr(attr->attr_datums, attr->attr_strs);
 			cil_log(CIL_INFO, "%s)\n",attr->expand ?
-- 
2.29.2

[PATCH 2/4] libsepol/cil: fix NULL pointer dereference when using an unused alias

[Nicolas Iooss]

OSS-Fuzz found a NULL pointer dereference when the CIL compiler tries to
compile a policy where a categoryalias references an unused
categoryalias:

    $ echo '(categoryalias c0)(categoryalias c1)(categoryaliasactual c0 c1)' > tmp.cil
    $ secil tmp.cil
    Segmentation fault (core dumped)

In such a case, a1 can become NULL in cil_resolve_alias_to_actual().
Add a check to report an error when this occurs. Now the error message
is:

    Alias c0 references an unused alias c1 at tmp.cil:1

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28471
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_resolve_ast.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 255f17ae7e30..0c85eabe5a81 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -556,6 +556,10 @@ int cil_resolve_alias_to_actual(struct cil_tree_node *current, enum cil_flavor f
 	a1_node = a1->datum.nodes->head->data;
 
 	while (flavor != a1_node->flavor) {
+		if (a1->actual == NULL) {
+			cil_tree_log(current, CIL_ERR, "Alias %s references an unused alias %s", alias->datum.name, a1->datum.name);
+			return SEPOL_ERR;
+		}
 		a1 = a1->actual;
 		a1_node = a1->datum.nodes->head->data;
 		steps += 1;
-- 
2.29.2

[PATCH 3/4] libsepol/cil: do not add a stack variable to a list

[Nicolas Iooss]

OSS-Fuzz found a heap use-after-free when the CIL compiler destroys its
database after failing to compile the following policy:

    (validatetrans x (eq t3 (a)))

This is caused by the fact that the validatetrans AST object references
a stack variable local to __cil_fill_constraint_leaf_expr, when parsing
the list "(a)":

    struct cil_list *sub_list;
    cil_fill_list(current->next->next->cl_head, leaf_expr_flavor, &sub_list);
    cil_list_append(*leaf_expr, CIL_LIST, &sub_list);

Drop the & sign to really add the list like it is supposed to be.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28507
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_build_ast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 0c9015cef578..4caff3cb3c98 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -2714,7 +2714,7 @@ static int __cil_fill_constraint_leaf_expr(struct cil_tree_node *current, enum c
 	} else if (r_flavor == CIL_LIST) {
 		struct cil_list *sub_list;
 		cil_fill_list(current->next->next->cl_head, leaf_expr_flavor, &sub_list);
-		cil_list_append(*leaf_expr, CIL_LIST, &sub_list);
+		cil_list_append(*leaf_expr, CIL_LIST, sub_list);
 	} else {
 		cil_list_append(*leaf_expr, CIL_CONS_OPERAND, (void *)r_flavor);
 	}
-- 
2.29.2

[PATCH 4/4] libsepol/cil: propagate failure of cil_fill_list()

[Nicolas Iooss]

OSS-Fuzz found a Null-dereference READ in the CIL compiler when trying
to compile the following policy:

    (optional o (validatetrans x (eq t3 (a ()))))

With some logs, secilc reports:

    Invalid syntax
    Destroying Parse Tree
    Resolving AST
    Failed to resolve validatetrans statement at fuzz:1
    Disabling optional 'o' at tmp.cil:1

So there is an "Invalid syntax" error, but the compilation continues.
Fix this issue by stopping the compilation when cil_fill_list() reports
an error:

    Invalid syntax
    Bad expression tree for constraint
    Bad validatetrans declaration at tmp.cil:1

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29061
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_build_ast.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 4caff3cb3c98..0ea90cf92186 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -2713,7 +2713,11 @@ static int __cil_fill_constraint_leaf_expr(struct cil_tree_node *current, enum c
 		cil_list_append(*leaf_expr, CIL_STRING, current->next->next->data);
 	} else if (r_flavor == CIL_LIST) {
 		struct cil_list *sub_list;
-		cil_fill_list(current->next->next->cl_head, leaf_expr_flavor, &sub_list);
+		rc = cil_fill_list(current->next->next->cl_head, leaf_expr_flavor, &sub_list);
+		if (rc != SEPOL_OK) {
+			cil_list_destroy(leaf_expr, CIL_TRUE);
+			goto exit;
+		}
 		cil_list_append(*leaf_expr, CIL_LIST, sub_list);
 	} else {
 		cil_list_append(*leaf_expr, CIL_CONS_OPERAND, (void *)r_flavor);
-- 
2.29.2

Re: [PATCH 4/4] libsepol/cil: propagate failure of cil_fill_list()

[William Roberts]

On Wed, Dec 30, 2020 at 2:13 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> OSS-Fuzz found a Null-dereference READ in the CIL compiler when trying
> to compile the following policy:
>
>     (optional o (validatetrans x (eq t3 (a ()))))
>
> With some logs, secilc reports:
>
>     Invalid syntax
>     Destroying Parse Tree
>     Resolving AST
>     Failed to resolve validatetrans statement at fuzz:1
>     Disabling optional 'o' at tmp.cil:1
>
> So there is an "Invalid syntax" error, but the compilation continues.
> Fix this issue by stopping the compilation when cil_fill_list() reports
> an error:
>
>     Invalid syntax
>     Bad expression tree for constraint
>     Bad validatetrans declaration at tmp.cil:1
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29061
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/cil/src/cil_build_ast.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index 4caff3cb3c98..0ea90cf92186 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -2713,7 +2713,11 @@ static int __cil_fill_constraint_leaf_expr(struct cil_tree_node *current, enum c
>                 cil_list_append(*leaf_expr, CIL_STRING, current->next->next->data);
>         } else if (r_flavor == CIL_LIST) {
>                 struct cil_list *sub_list;
> -               cil_fill_list(current->next->next->cl_head, leaf_expr_flavor, &sub_list);
> +               rc = cil_fill_list(current->next->next->cl_head, leaf_expr_flavor, &sub_list);
> +               if (rc != SEPOL_OK) {
> +                       cil_list_destroy(leaf_expr, CIL_TRUE);
> +                       goto exit;
> +               }
>                 cil_list_append(*leaf_expr, CIL_LIST, sub_list);
>         } else {
>                 cil_list_append(*leaf_expr, CIL_CONS_OPERAND, (void *)r_flavor);
> --
> 2.29.2
>

ack for the series.

Re: [PATCH 4/4] libsepol/cil: propagate failure of cil_fill_list()

[James Carter]

On Thu, Dec 31, 2020 at 10:29 AM William Roberts
<bill.c.roberts@gmail.com> wrote:
>
> On Wed, Dec 30, 2020 at 2:13 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > OSS-Fuzz found a Null-dereference READ in the CIL compiler when trying
> > to compile the following policy:
> >
> >     (optional o (validatetrans x (eq t3 (a ()))))
> >
> > With some logs, secilc reports:
> >
> >     Invalid syntax
> >     Destroying Parse Tree
> >     Resolving AST
> >     Failed to resolve validatetrans statement at fuzz:1
> >     Disabling optional 'o' at tmp.cil:1
> >
> > So there is an "Invalid syntax" error, but the compilation continues.
> > Fix this issue by stopping the compilation when cil_fill_list() reports
> > an error:
> >
> >     Invalid syntax
> >     Bad expression tree for constraint
> >     Bad validatetrans declaration at tmp.cil:1
> >
> > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29061
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > ---
> >  libsepol/cil/src/cil_build_ast.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> > index 4caff3cb3c98..0ea90cf92186 100644
> > --- a/libsepol/cil/src/cil_build_ast.c
> > +++ b/libsepol/cil/src/cil_build_ast.c
> > @@ -2713,7 +2713,11 @@ static int __cil_fill_constraint_leaf_expr(struct cil_tree_node *current, enum c
> >                 cil_list_append(*leaf_expr, CIL_STRING, current->next->next->data);
> >         } else if (r_flavor == CIL_LIST) {
> >                 struct cil_list *sub_list;
> > -               cil_fill_list(current->next->next->cl_head, leaf_expr_flavor, &sub_list);
> > +               rc = cil_fill_list(current->next->next->cl_head, leaf_expr_flavor, &sub_list);
> > +               if (rc != SEPOL_OK) {
> > +                       cil_list_destroy(leaf_expr, CIL_TRUE);
> > +                       goto exit;
> > +               }
> >                 cil_list_append(*leaf_expr, CIL_LIST, sub_list);
> >         } else {
> >                 cil_list_append(*leaf_expr, CIL_CONS_OPERAND, (void *)r_flavor);
> > --
> > 2.29.2
> >
>
> ack for the series.

I've applied this series.

Thanks,
Jim