[PATCH 0/3] Fuzzing secilc with AFL

[PATCH 0/3] Fuzzing secilc with AFL

[Nicolas Iooss]

Hello,

Last week I started fuzzing secilc with american fuzzy lop, mainly
because I was curious of the result. I started with a small CIL policy,
for which AFL quickly found one crashing mutation (fixed by commit
c303ca910add ("libsepol/cil: Check for too many permissions in classes
and commons")).

Then I unleashed AFL on the files in secilc and it found 19 unique
crashing inputs over the week-end. I started digging through these
inputs to uncover bugs and this patchset consists in the patches I
wrote and quickly tested tonight.

If anyone is interested in running AFL too, I published my setup
(along with my custom Makefile and some other patches) on
https://github.com/fishilico/selinux (branch 'master'). I wrote
run_afl.sh (in the root of this project) as a wrapper to afl-fuzz to
make fuzzing start with "./run_afl.sh secilc".

Cheers,
Nicolas

PS: I am quite busy in the next weeks so I may be quite slow to reply
to feedbacks and to send other patches.

Nicolas Iooss (3):
  libsepol/cil: make cil_resolve_name() fail for '.'
  libsepol/cil: fix double-free in cil categories parser
  libsepol/cil: fix memory leak in __cil_fill_expr()

 libsepol/cil/src/cil_build_ast.c   | 2 ++
 libsepol/cil/src/cil_resolve_ast.c | 8 +++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.10.0

[PATCH 1/3] libsepol/cil: make cil_resolve_name() fail for '.'

[Nicolas Iooss]

This CIL policy makes secilc crash with a NULL pointer dereference:

    (class CLASS (PERM))
    (classorder (CLASS))
    (sid SID)
    (sidorder (SID))
    (user USER)
    (role ROLE)
    (type TYPE)
    (category CAT)
    (categoryorder (CAT))
    (sensitivity SENS)
    (sensitivityorder (SENS))
    (sensitivitycategory SENS (CAT))
    (allow TYPE self (CLASS (PERM)))
    (roletype ROLE TYPE)
    (userrole USER ROLE)
    (userlevel USER (SENS))
    (userrange USER ((SENS)(SENS (CAT))))
    (sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))

    (allow . self (CLASS (PERM)))

Using "." in the allow statement makes strtok_r() return NULL in
cil_resolve_name() and this result is then used in a call to
cil_symtab_get_datum(), which is thus invalid.

Instead of crashing, make secilc fail with an error message.

This bug has been found by fuzzing secilc with american fuzzy lop.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_resolve_ast.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 917adf8d23da..5b86908c4120 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -4027,7 +4027,13 @@ int cil_resolve_name(struct cil_tree_node *ast_node, char *name, enum cil_sym_in
 		char *current = strtok_r(name_dup, ".", &sp);
 		char *next = strtok_r(NULL, ".", &sp);
 		symtab_t *symtab = NULL;
-		
+
+		if (current == NULL) {
+			/* Only dots */
+			cil_tree_log(ast_node, CIL_ERR, "Invalid name %s", name);
+			goto exit;
+		}
+
 		node = ast_node;
 		if (*name == '.') {
 			/* Leading '.' */
-- 
2.10.0

[PATCH 2/3] libsepol/cil: fix double-free in cil categories parser

[Nicolas Iooss]

When cil_fill_cats() fails to parse an expression and destroys a
category set, it fails to reset *cats to NULL. This makes this object be
destroyed again in cil_destroy_catset().

This bug can be triggered by the following policy:

    (class CLASS (PERM))
    (classorder (CLASS))
    (sid SID)
    (sidorder (SID))
    (user USER)
    (role ROLE)
    (type TYPE)
    (category CAT)
    (categoryorder (CAT))
    (sensitivity SENS)
    (sensitivityorder (SENS))
    (sensitivitycategory SENS (CAT))
    (allow TYPE self (CLASS (PERM)))
    (roletype ROLE TYPE)
    (userrole USER ROLE)
    (userlevel USER (SENS))
    (userrange USER ((SENS)(SENS (CAT))))
    (sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))

    (categoryset cats (range unknown))

This bug has been found by fuzzing secilc with american fuzzy lop.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_build_ast.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index a96c2a95ca3f..f57bd21358d3 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -5481,6 +5481,7 @@ int cil_fill_cats(struct cil_tree_node *curr, struct cil_cats **cats)
 	rc = cil_gen_expr(curr, CIL_CAT, &(*cats)->str_expr);
 	if (rc != SEPOL_OK) {
 		cil_destroy_cats(*cats);
+		*cats = NULL;
 	}
 
 	return rc;
-- 
2.10.0

[PATCH 3/3] libsepol/cil: fix memory leak in __cil_fill_expr()

[Nicolas Iooss]

__cil_fill_expr() initializes 'cil_list *sub_expr' but does not destroy
it when __cil_fill_expr_helper() fails. This list is therefore leaked
when __cil_fill_expr() returns.

This occurs when secilc compiles the following policy:

    (class CLASS (PERM))
    (classorder (CLASS))
    (sid SID)
    (sidorder (SID))
    (user USER)
    (role ROLE)
    (type TYPE)
    (category CAT)
    (categoryorder (CAT))
    (sensitivity SENS)
    (sensitivityorder (SENS))
    (sensitivitycategory SENS (CAT))
    (allow TYPE self (CLASS (PERM)))
    (roletype ROLE TYPE)
    (userrole USER ROLE)
    (userlevel USER (SENS))
    (userrange USER ((SENS)(SENS (CAT))))
    (sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))

    (categoryset cats (not (range unknown)))

This bug has been found using gcc address sanitizer.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_build_ast.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index f57bd21358d3..ee283b535147 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -2562,6 +2562,7 @@ static int __cil_fill_expr(struct cil_tree_node *current, enum cil_flavor flavor
 		cil_list_init(&sub_expr, flavor);
 		rc = __cil_fill_expr_helper(current->cl_head, flavor, sub_expr, depth);
 		if (rc != SEPOL_OK) {
+			cil_list_destroy(&sub_expr, CIL_TRUE);
 			goto exit;
 		}
 		cil_list_append(expr, CIL_LIST, sub_expr);
-- 
2.10.0

Re: [PATCH 1/3] libsepol/cil: make cil_resolve_name() fail for '.'

[James Carter]

On 10/03/2016 04:46 PM, Nicolas Iooss wrote:
> This CIL policy makes secilc crash with a NULL pointer dereference:
>
>     (class CLASS (PERM))
>     (classorder (CLASS))
>     (sid SID)
>     (sidorder (SID))
>     (user USER)
>     (role ROLE)
>     (type TYPE)
>     (category CAT)
>     (categoryorder (CAT))
>     (sensitivity SENS)
>     (sensitivityorder (SENS))
>     (sensitivitycategory SENS (CAT))
>     (allow TYPE self (CLASS (PERM)))
>     (roletype ROLE TYPE)
>     (userrole USER ROLE)
>     (userlevel USER (SENS))
>     (userrange USER ((SENS)(SENS (CAT))))
>     (sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))
>
>     (allow . self (CLASS (PERM)))
>
> Using "." in the allow statement makes strtok_r() return NULL in
> cil_resolve_name() and this result is then used in a call to
> cil_symtab_get_datum(), which is thus invalid.
>
> Instead of crashing, make secilc fail with an error message.
>
> This bug has been found by fuzzing secilc with american fuzzy lop.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/cil/src/cil_resolve_ast.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 917adf8d23da..5b86908c4120 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -4027,7 +4027,13 @@ int cil_resolve_name(struct cil_tree_node *ast_node, char *name, enum cil_sym_in
>  		char *current = strtok_r(name_dup, ".", &sp);
>  		char *next = strtok_r(NULL, ".", &sp);
>  		symtab_t *symtab = NULL;
> -		
> +
> +		if (current == NULL) {
> +			/* Only dots */
> +			cil_tree_log(ast_node, CIL_ERR, "Invalid name %s", name);

I added "free(name_dup);" here when I applied the patch.

Jim

> +			goto exit;
> +		}
> +
>  		node = ast_node;
>  		if (*name == '.') {
>  			/* Leading '.' */
>


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

Re: [PATCH 0/3] Fuzzing secilc with AFL

[James Carter]

On 10/03/2016 04:46 PM, Nicolas Iooss wrote:
> Hello,
>
> Last week I started fuzzing secilc with american fuzzy lop, mainly
> because I was curious of the result. I started with a small CIL policy,
> for which AFL quickly found one crashing mutation (fixed by commit
> c303ca910add ("libsepol/cil: Check for too many permissions in classes
> and commons")).
>
> Then I unleashed AFL on the files in secilc and it found 19 unique
> crashing inputs over the week-end. I started digging through these
> inputs to uncover bugs and this patchset consists in the patches I
> wrote and quickly tested tonight.
>
> If anyone is interested in running AFL too, I published my setup
> (along with my custom Makefile and some other patches) on
> https://github.com/fishilico/selinux (branch 'master'). I wrote
> run_afl.sh (in the root of this project) as a wrapper to afl-fuzz to
> make fuzzing start with "./run_afl.sh secilc".
>
> Cheers,
> Nicolas
>
> PS: I am quite busy in the next weeks so I may be quite slow to reply
> to feedbacks and to send other patches.

Applied these with the one addition that I noted.

Thanks,
Jim

>
> Nicolas Iooss (3):
>   libsepol/cil: make cil_resolve_name() fail for '.'
>   libsepol/cil: fix double-free in cil categories parser
>   libsepol/cil: fix memory leak in __cil_fill_expr()
>
>  libsepol/cil/src/cil_build_ast.c   | 2 ++
>  libsepol/cil/src/cil_resolve_ast.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
>


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency