[PATCH 1/6] libsepol: do not decode out-of-bound rolebounds

[PATCH 1/6] libsepol: do not decode out-of-bound rolebounds

[Nicolas Iooss]

While fuzzing /usr/libexec/hll/pp, a policy module was generated with a
role->bounds larger that the number of roles in the policy.

This issue has been found while fuzzing hll/pp with the American Fuzzy
Lop.

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

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index a87bc15e7610..c99790eb76e7 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -2165,7 +2165,9 @@ static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN
 			}
 		}
 
-		if (role->bounds > 0) {
+		if (role->bounds >= pdb->p_roles.nprim) {
+			log_err("Warning: role %s defines an out-of-bound rolebounds", key);
+		} else if (role->bounds > 0) {
 			cil_println(indent, "(rolebounds %s %s)", key, pdb->p_role_val_to_name[role->bounds - 1]);
 		}
 		break;
-- 
2.29.2

[PATCH 2/6] libsepol: ensure that hashtab_search is not called with a NULL key

[Nicolas Iooss]

While fuzzing /usr/libexec/hll/pp, a policy module was generated which
triggered a NULL result when doing:

    key = pdb->sym_val_to_name[sym][i];

Detect such unexpected behavior to exit with an error instead of
crashing.

This issue has been found while fuzzing hll/pp with the American Fuzzy
Lop.

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

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index c99790eb76e7..99360a9afdd0 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -3459,6 +3459,10 @@ static int required_scopes_to_cil(int indent, struct policydb *pdb, struct avrul
 		map = decl->required.scope[sym];
 		ebitmap_for_each_positive_bit(&map, node, i) {
 			key = pdb->sym_val_to_name[sym][i];
+			if (key == NULL) {
+				rc = -1;
+				goto exit;
+			}
 
 			scope_datum = hashtab_search(pdb->scope[sym].table, key);
 			if (scope_datum == NULL) {
-- 
2.29.2

Re: [PATCH 2/6] libsepol: ensure that hashtab_search is not called with a NULL key

[James Carter]

On Wed, Dec 30, 2020 at 5:10 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> While fuzzing /usr/libexec/hll/pp, a policy module was generated which
> triggered a NULL result when doing:
>
>     key = pdb->sym_val_to_name[sym][i];
>
> Detect such unexpected behavior to exit with an error instead of
> crashing.
>
> This issue has been found while fuzzing hll/pp with the American Fuzzy
> Lop.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/src/module_to_cil.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index c99790eb76e7..99360a9afdd0 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -3459,6 +3459,10 @@ static int required_scopes_to_cil(int indent, struct policydb *pdb, struct avrul
>                 map = decl->required.scope[sym];
>                 ebitmap_for_each_positive_bit(&map, node, i) {
>                         key = pdb->sym_val_to_name[sym][i];
> +                       if (key == NULL) {
> +                               rc = -1;
> +                               goto exit;
> +                       }
>
>                         scope_datum = hashtab_search(pdb->scope[sym].table, key);
>                         if (scope_datum == NULL) {
> --
> 2.29.2
>

This is a similar case as the previous patch. There are other places
where a check could go, but the check really should happen when
reading the binary policy.

Jim

Re: [PATCH 2/6] libsepol: ensure that hashtab_search is not called with a NULL key

[Nicolas Iooss]

On Mon, Jan 4, 2021 at 5:31 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Wed, Dec 30, 2020 at 5:10 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > While fuzzing /usr/libexec/hll/pp, a policy module was generated which
> > triggered a NULL result when doing:
> >
> >     key = pdb->sym_val_to_name[sym][i];
> >
> > Detect such unexpected behavior to exit with an error instead of
> > crashing.
> >
> > This issue has been found while fuzzing hll/pp with the American Fuzzy
> > Lop.
> >
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > ---
> >  libsepol/src/module_to_cil.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> > index c99790eb76e7..99360a9afdd0 100644
> > --- a/libsepol/src/module_to_cil.c
> > +++ b/libsepol/src/module_to_cil.c
> > @@ -3459,6 +3459,10 @@ static int required_scopes_to_cil(int indent, struct policydb *pdb, struct avrul
> >                 map = decl->required.scope[sym];
> >                 ebitmap_for_each_positive_bit(&map, node, i) {
> >                         key = pdb->sym_val_to_name[sym][i];
> > +                       if (key == NULL) {
> > +                               rc = -1;
> > +                               goto exit;
> > +                       }
> >
> >                         scope_datum = hashtab_search(pdb->scope[sym].table, key);
> >                         if (scope_datum == NULL) {
> > --
> > 2.29.2
> >
>
> This is a similar case as the previous patch. There are other places
> where a check could go, but the check really should happen when
> reading the binary policy.

I agree, thanks for your review! I have sent a new patch which
introduces a check in policydb_read
(https://lore.kernel.org/selinux/20210106080836.344857-1-nicolas.iooss@m4x.org/T/).

Nicolas

[PATCH 3/6] libsepol/cil: constify some strings

[Nicolas Iooss]

Function cil_add_file() copies its input into a newly-allocated buffer,
and does not modify "name". State these properties in the types of
parameters by adding "const" qualifiers.

This enables using LibFuzzer directly on cil_add_file(), without a
warning about discarding "const" qualifier:

    fuzz-secilc.c: In function ‘LLVMFuzzerTestOneInput’:
    fuzz-secilc.c:57:31: warning: passing argument 3 of ‘cil_add_file’
    discards ‘const’ qualifier from pointer target type
    [-Wdiscarded-qualifiers]
       57 |  if (cil_add_file(db, "fuzz", data, size) != SEPOL_OK)
          |                               ^~~~
    In file included from fuzz-secilc.c:26:
    /usr/include/sepol/cil/cil.h:45:57: note: expected ‘char *’ but
    argument is of type ‘const uint8_t *’ {aka ‘const unsigned char *’}
       45 | extern int cil_add_file(cil_db_t *db, char *name, char *data, size_t size);
          |                                                   ~~~~~~^~~~

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/include/cil/cil.h | 4 ++--
 libsepol/cil/src/cil.c         | 2 +-
 libsepol/cil/src/cil_log.c     | 6 +++---
 libsepol/cil/src/cil_parser.c  | 2 +-
 libsepol/cil/src/cil_parser.h  | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/libsepol/cil/include/cil/cil.h b/libsepol/cil/include/cil/cil.h
index f8cfc3be5015..e6f4503eb33a 100644
--- a/libsepol/cil/include/cil/cil.h
+++ b/libsepol/cil/include/cil/cil.h
@@ -42,7 +42,7 @@ typedef struct cil_db cil_db_t;
 extern void cil_db_init(cil_db_t **db);
 extern void cil_db_destroy(cil_db_t **db);
 
-extern int cil_add_file(cil_db_t *db, char *name, char *data, size_t size);
+extern int cil_add_file(cil_db_t *db, const char *name, const char *data, size_t size);
 
 extern int cil_compile(cil_db_t *db);
 extern int cil_build_policydb(cil_db_t *db, sepol_policydb_t **sepol_db);
@@ -67,7 +67,7 @@ enum cil_log_level {
 	CIL_INFO
 };
 extern void cil_set_log_level(enum cil_log_level lvl);
-extern void cil_set_log_handler(void (*handler)(int lvl, char *msg));
+extern void cil_set_log_handler(void (*handler)(int lvl, const char *msg));
 
 #ifdef __GNUC__
 __attribute__ ((format(printf, 2, 3)))
diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
index bb7f06d5c4b3..99c8e288912c 100644
--- a/libsepol/cil/src/cil.c
+++ b/libsepol/cil/src/cil.c
@@ -500,7 +500,7 @@ void cil_root_destroy(struct cil_root *root)
 	free(root);
 }
 
-int cil_add_file(cil_db_t *db, char *name, char *data, size_t size)
+int cil_add_file(cil_db_t *db, const char *name, const char *data, size_t size)
 {
 	char *buffer = NULL;
 	int rc;
diff --git a/libsepol/cil/src/cil_log.c b/libsepol/cil/src/cil_log.c
index b222b155120a..a8e4d2e94a78 100644
--- a/libsepol/cil/src/cil_log.c
+++ b/libsepol/cil/src/cil_log.c
@@ -37,14 +37,14 @@
 
 static enum cil_log_level cil_log_level = CIL_ERR;
 
-void cil_default_log_handler(__attribute__((unused)) int lvl, char *msg)
+void cil_default_log_handler(__attribute__((unused)) int lvl, const char *msg)
 {
 	fprintf(stderr, "%s", msg);
 }
 
-void (*cil_log_handler)(int lvl, char *msg) = &cil_default_log_handler;
+void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
 
-void cil_set_log_handler(void (*handler)(int lvl, char *msg))
+void cil_set_log_handler(void (*handler)(int lvl, const char *msg))
 {
 	cil_log_handler = handler;
 }
diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
index a8af1dce2c4b..b62043b95806 100644
--- a/libsepol/cil/src/cil_parser.c
+++ b/libsepol/cil/src/cil_parser.c
@@ -196,7 +196,7 @@ static void add_cil_path(struct cil_tree_node **current, char *path)
 	insert_node(node, *current);
 }
 
-int cil_parser(char *_path, char *buffer, uint32_t size, struct cil_tree **parse_tree)
+int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree **parse_tree)
 {
 
 	int paren_count = 0;
diff --git a/libsepol/cil/src/cil_parser.h b/libsepol/cil/src/cil_parser.h
index 02ecb784e95c..1cec63944fdf 100644
--- a/libsepol/cil/src/cil_parser.h
+++ b/libsepol/cil/src/cil_parser.h
@@ -32,6 +32,6 @@
 
 #include "cil_tree.h"
 
-int cil_parser(char *path, char *buffer, uint32_t size, struct cil_tree **parse_tree);
+int cil_parser(const char *path, char *buffer, uint32_t size, struct cil_tree **parse_tree);
 
 #endif /* CIL_PARSER_H_ */
-- 
2.29.2

Re: [PATCH 3/6] libsepol/cil: constify some strings

[James Carter]

On Wed, Dec 30, 2020 at 5:09 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> Function cil_add_file() copies its input into a newly-allocated buffer,
> and does not modify "name". State these properties in the types of
> parameters by adding "const" qualifiers.
>
> This enables using LibFuzzer directly on cil_add_file(), without a
> warning about discarding "const" qualifier:
>
>     fuzz-secilc.c: In function ‘LLVMFuzzerTestOneInput’:
>     fuzz-secilc.c:57:31: warning: passing argument 3 of ‘cil_add_file’
>     discards ‘const’ qualifier from pointer target type
>     [-Wdiscarded-qualifiers]
>        57 |  if (cil_add_file(db, "fuzz", data, size) != SEPOL_OK)
>           |                               ^~~~
>     In file included from fuzz-secilc.c:26:
>     /usr/include/sepol/cil/cil.h:45:57: note: expected ‘char *’ but
>     argument is of type ‘const uint8_t *’ {aka ‘const unsigned char *’}
>        45 | extern int cil_add_file(cil_db_t *db, char *name, char *data, size_t size);
>           |                                                   ~~~~~~^~~~
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/include/cil/cil.h | 4 ++--
>  libsepol/cil/src/cil.c         | 2 +-
>  libsepol/cil/src/cil_log.c     | 6 +++---
>  libsepol/cil/src/cil_parser.c  | 2 +-
>  libsepol/cil/src/cil_parser.h  | 2 +-
>  5 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/libsepol/cil/include/cil/cil.h b/libsepol/cil/include/cil/cil.h
> index f8cfc3be5015..e6f4503eb33a 100644
> --- a/libsepol/cil/include/cil/cil.h
> +++ b/libsepol/cil/include/cil/cil.h
> @@ -42,7 +42,7 @@ typedef struct cil_db cil_db_t;
>  extern void cil_db_init(cil_db_t **db);
>  extern void cil_db_destroy(cil_db_t **db);
>
> -extern int cil_add_file(cil_db_t *db, char *name, char *data, size_t size);
> +extern int cil_add_file(cil_db_t *db, const char *name, const char *data, size_t size);
>
>  extern int cil_compile(cil_db_t *db);
>  extern int cil_build_policydb(cil_db_t *db, sepol_policydb_t **sepol_db);
> @@ -67,7 +67,7 @@ enum cil_log_level {
>         CIL_INFO
>  };
>  extern void cil_set_log_level(enum cil_log_level lvl);
> -extern void cil_set_log_handler(void (*handler)(int lvl, char *msg));
> +extern void cil_set_log_handler(void (*handler)(int lvl, const char *msg));
>
>  #ifdef __GNUC__
>  __attribute__ ((format(printf, 2, 3)))
> diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
> index bb7f06d5c4b3..99c8e288912c 100644
> --- a/libsepol/cil/src/cil.c
> +++ b/libsepol/cil/src/cil.c
> @@ -500,7 +500,7 @@ void cil_root_destroy(struct cil_root *root)
>         free(root);
>  }
>
> -int cil_add_file(cil_db_t *db, char *name, char *data, size_t size)
> +int cil_add_file(cil_db_t *db, const char *name, const char *data, size_t size)
>  {
>         char *buffer = NULL;
>         int rc;
> diff --git a/libsepol/cil/src/cil_log.c b/libsepol/cil/src/cil_log.c
> index b222b155120a..a8e4d2e94a78 100644
> --- a/libsepol/cil/src/cil_log.c
> +++ b/libsepol/cil/src/cil_log.c
> @@ -37,14 +37,14 @@
>
>  static enum cil_log_level cil_log_level = CIL_ERR;
>
> -void cil_default_log_handler(__attribute__((unused)) int lvl, char *msg)
> +void cil_default_log_handler(__attribute__((unused)) int lvl, const char *msg)
>  {
>         fprintf(stderr, "%s", msg);
>  }
>
> -void (*cil_log_handler)(int lvl, char *msg) = &cil_default_log_handler;
> +void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
>
> -void cil_set_log_handler(void (*handler)(int lvl, char *msg))
> +void cil_set_log_handler(void (*handler)(int lvl, const char *msg))
>  {
>         cil_log_handler = handler;
>  }
> diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
> index a8af1dce2c4b..b62043b95806 100644
> --- a/libsepol/cil/src/cil_parser.c
> +++ b/libsepol/cil/src/cil_parser.c
> @@ -196,7 +196,7 @@ static void add_cil_path(struct cil_tree_node **current, char *path)
>         insert_node(node, *current);
>  }
>
> -int cil_parser(char *_path, char *buffer, uint32_t size, struct cil_tree **parse_tree)
> +int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree **parse_tree)
>  {
>
>         int paren_count = 0;
> diff --git a/libsepol/cil/src/cil_parser.h b/libsepol/cil/src/cil_parser.h
> index 02ecb784e95c..1cec63944fdf 100644
> --- a/libsepol/cil/src/cil_parser.h
> +++ b/libsepol/cil/src/cil_parser.h
> @@ -32,6 +32,6 @@
>
>  #include "cil_tree.h"
>
> -int cil_parser(char *path, char *buffer, uint32_t size, struct cil_tree **parse_tree);
> +int cil_parser(const char *path, char *buffer, uint32_t size, struct cil_tree **parse_tree);
>
>  #endif /* CIL_PARSER_H_ */
> --
> 2.29.2
>

Re: [PATCH 3/6] libsepol/cil: constify some strings

[James Carter]

Applied.

Thanks,
Jim

On Mon, Jan 4, 2021 at 11:33 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Wed, Dec 30, 2020 at 5:09 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > Function cil_add_file() copies its input into a newly-allocated buffer,
> > and does not modify "name". State these properties in the types of
> > parameters by adding "const" qualifiers.
> >
> > This enables using LibFuzzer directly on cil_add_file(), without a
> > warning about discarding "const" qualifier:
> >
> >     fuzz-secilc.c: In function ‘LLVMFuzzerTestOneInput’:
> >     fuzz-secilc.c:57:31: warning: passing argument 3 of ‘cil_add_file’
> >     discards ‘const’ qualifier from pointer target type
> >     [-Wdiscarded-qualifiers]
> >        57 |  if (cil_add_file(db, "fuzz", data, size) != SEPOL_OK)
> >           |                               ^~~~
> >     In file included from fuzz-secilc.c:26:
> >     /usr/include/sepol/cil/cil.h:45:57: note: expected ‘char *’ but
> >     argument is of type ‘const uint8_t *’ {aka ‘const unsigned char *’}
> >        45 | extern int cil_add_file(cil_db_t *db, char *name, char *data, size_t size);
> >           |                                                   ~~~~~~^~~~
> >
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>
> > ---
> >  libsepol/cil/include/cil/cil.h | 4 ++--
> >  libsepol/cil/src/cil.c         | 2 +-
> >  libsepol/cil/src/cil_log.c     | 6 +++---
> >  libsepol/cil/src/cil_parser.c  | 2 +-
> >  libsepol/cil/src/cil_parser.h  | 2 +-
> >  5 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/libsepol/cil/include/cil/cil.h b/libsepol/cil/include/cil/cil.h
> > index f8cfc3be5015..e6f4503eb33a 100644
> > --- a/libsepol/cil/include/cil/cil.h
> > +++ b/libsepol/cil/include/cil/cil.h
> > @@ -42,7 +42,7 @@ typedef struct cil_db cil_db_t;
> >  extern void cil_db_init(cil_db_t **db);
> >  extern void cil_db_destroy(cil_db_t **db);
> >
> > -extern int cil_add_file(cil_db_t *db, char *name, char *data, size_t size);
> > +extern int cil_add_file(cil_db_t *db, const char *name, const char *data, size_t size);
> >
> >  extern int cil_compile(cil_db_t *db);
> >  extern int cil_build_policydb(cil_db_t *db, sepol_policydb_t **sepol_db);
> > @@ -67,7 +67,7 @@ enum cil_log_level {
> >         CIL_INFO
> >  };
> >  extern void cil_set_log_level(enum cil_log_level lvl);
> > -extern void cil_set_log_handler(void (*handler)(int lvl, char *msg));
> > +extern void cil_set_log_handler(void (*handler)(int lvl, const char *msg));
> >
> >  #ifdef __GNUC__
> >  __attribute__ ((format(printf, 2, 3)))
> > diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
> > index bb7f06d5c4b3..99c8e288912c 100644
> > --- a/libsepol/cil/src/cil.c
> > +++ b/libsepol/cil/src/cil.c
> > @@ -500,7 +500,7 @@ void cil_root_destroy(struct cil_root *root)
> >         free(root);
> >  }
> >
> > -int cil_add_file(cil_db_t *db, char *name, char *data, size_t size)
> > +int cil_add_file(cil_db_t *db, const char *name, const char *data, size_t size)
> >  {
> >         char *buffer = NULL;
> >         int rc;
> > diff --git a/libsepol/cil/src/cil_log.c b/libsepol/cil/src/cil_log.c
> > index b222b155120a..a8e4d2e94a78 100644
> > --- a/libsepol/cil/src/cil_log.c
> > +++ b/libsepol/cil/src/cil_log.c
> > @@ -37,14 +37,14 @@
> >
> >  static enum cil_log_level cil_log_level = CIL_ERR;
> >
> > -void cil_default_log_handler(__attribute__((unused)) int lvl, char *msg)
> > +void cil_default_log_handler(__attribute__((unused)) int lvl, const char *msg)
> >  {
> >         fprintf(stderr, "%s", msg);
> >  }
> >
> > -void (*cil_log_handler)(int lvl, char *msg) = &cil_default_log_handler;
> > +void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
> >
> > -void cil_set_log_handler(void (*handler)(int lvl, char *msg))
> > +void cil_set_log_handler(void (*handler)(int lvl, const char *msg))
> >  {
> >         cil_log_handler = handler;
> >  }
> > diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
> > index a8af1dce2c4b..b62043b95806 100644
> > --- a/libsepol/cil/src/cil_parser.c
> > +++ b/libsepol/cil/src/cil_parser.c
> > @@ -196,7 +196,7 @@ static void add_cil_path(struct cil_tree_node **current, char *path)
> >         insert_node(node, *current);
> >  }
> >
> > -int cil_parser(char *_path, char *buffer, uint32_t size, struct cil_tree **parse_tree)
> > +int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree **parse_tree)
> >  {
> >
> >         int paren_count = 0;
> > diff --git a/libsepol/cil/src/cil_parser.h b/libsepol/cil/src/cil_parser.h
> > index 02ecb784e95c..1cec63944fdf 100644
> > --- a/libsepol/cil/src/cil_parser.h
> > +++ b/libsepol/cil/src/cil_parser.h
> > @@ -32,6 +32,6 @@
> >
> >  #include "cil_tree.h"
> >
> > -int cil_parser(char *path, char *buffer, uint32_t size, struct cil_tree **parse_tree);
> > +int cil_parser(const char *path, char *buffer, uint32_t size, struct cil_tree **parse_tree);
> >
> >  #endif /* CIL_PARSER_H_ */
> > --
> > 2.29.2
> >

[PATCH 4/6] libsepol/cil: fix NULL pointer dereference when parsing an improper integer

[Nicolas Iooss]

OSS-Fuzz found a NULL pointer dereference when the CIL compiler tries to
compile a policy with an invalid integer:

    $ echo '(ioportcon(2())n)' > tmp.cil
    $ secilc tmp.cil
    Segmentation fault (core dumped)

This is because strtol() is called with a NULL pointer, in
cil_fill_integer().

Fix this by checking that int_node->data is not NULL. While at it, use
strtoul() instead of strtol() to parse an unsigned integer.

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

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 67801def0dc0..0c9015cef578 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -5566,15 +5566,15 @@ int cil_fill_integer(struct cil_tree_node *int_node, uint32_t *integer, int base
 {
 	int rc = SEPOL_ERR;
 	char *endptr = NULL;
-	int val;
+	unsigned long val;
 
-	if (int_node == NULL || integer == NULL) {
+	if (int_node == NULL || int_node->data == NULL || integer == NULL) {
 		goto exit;
 	}
 
 	errno = 0;
-	val = strtol(int_node->data, &endptr, base);
-	if (errno != 0 || endptr == int_node->data || *endptr != '\0') {
+	val = strtoul(int_node->data, &endptr, base);
+	if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) {
 		rc = SEPOL_ERR;
 		goto exit;
 	}
@@ -5594,7 +5594,7 @@ int cil_fill_integer64(struct cil_tree_node *int_node, uint64_t *integer, int ba
 	char *endptr = NULL;
 	uint64_t val;
 
-	if (int_node == NULL || integer == NULL) {
+	if (int_node == NULL || int_node->data == NULL || integer == NULL) {
 		goto exit;
 	}
 
-- 
2.29.2

Re: [PATCH 4/6] libsepol/cil: fix NULL pointer dereference when parsing an improper integer

[William Roberts]

On Wed, Dec 30, 2020 at 4:09 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> OSS-Fuzz found a NULL pointer dereference when the CIL compiler tries to
> compile a policy with an invalid integer:
>
>     $ echo '(ioportcon(2())n)' > tmp.cil
>     $ secilc tmp.cil
>     Segmentation fault (core dumped)
>
> This is because strtol() is called with a NULL pointer, in
> cil_fill_integer().
>
> Fix this by checking that int_node->data is not NULL. While at it, use
> strtoul() instead of strtol() to parse an unsigned integer.
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28456
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/cil/src/cil_build_ast.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index 67801def0dc0..0c9015cef578 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -5566,15 +5566,15 @@ int cil_fill_integer(struct cil_tree_node *int_node, uint32_t *integer, int base
>  {
>         int rc = SEPOL_ERR;
>         char *endptr = NULL;
> -       int val;
> +       unsigned long val;
>
> -       if (int_node == NULL || integer == NULL) {
> +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
>                 goto exit;
>         }
>
>         errno = 0;
> -       val = strtol(int_node->data, &endptr, base);
> -       if (errno != 0 || endptr == int_node->data || *endptr != '\0') {
> +       val = strtoul(int_node->data, &endptr, base);
> +       if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) {

I wonder if compilers/static analysis tools will balk on this as
strtoul's return, an unsigned long,
on a 32 bit machine will be 32 bits, so this could have a dead
expression as val > UINT32_MAX
will always be false. Perhaps use the strtoull variant to always have 64 bits?

>                 rc = SEPOL_ERR;
>                 goto exit;
>         }
> @@ -5594,7 +5594,7 @@ int cil_fill_integer64(struct cil_tree_node *int_node, uint64_t *integer, int ba
>         char *endptr = NULL;
>         uint64_t val;
>
> -       if (int_node == NULL || integer == NULL) {
> +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
>                 goto exit;
>         }
>
> --
> 2.29.2
>

Re: [PATCH 4/6] libsepol/cil: fix NULL pointer dereference when parsing an improper integer

[Nicolas Iooss]

On Thu, Dec 31, 2020 at 4:04 PM William Roberts
<bill.c.roberts@gmail.com> wrote:
>
> On Wed, Dec 30, 2020 at 4:09 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > OSS-Fuzz found a NULL pointer dereference when the CIL compiler tries to
> > compile a policy with an invalid integer:
> >
> >     $ echo '(ioportcon(2())n)' > tmp.cil
> >     $ secilc tmp.cil
> >     Segmentation fault (core dumped)
> >
> > This is because strtol() is called with a NULL pointer, in
> > cil_fill_integer().
> >
> > Fix this by checking that int_node->data is not NULL. While at it, use
> > strtoul() instead of strtol() to parse an unsigned integer.
> >
> > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28456
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > ---
> >  libsepol/cil/src/cil_build_ast.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> > index 67801def0dc0..0c9015cef578 100644
> > --- a/libsepol/cil/src/cil_build_ast.c
> > +++ b/libsepol/cil/src/cil_build_ast.c
> > @@ -5566,15 +5566,15 @@ int cil_fill_integer(struct cil_tree_node *int_node, uint32_t *integer, int base
> >  {
> >         int rc = SEPOL_ERR;
> >         char *endptr = NULL;
> > -       int val;
> > +       unsigned long val;
> >
> > -       if (int_node == NULL || integer == NULL) {
> > +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
> >                 goto exit;
> >         }
> >
> >         errno = 0;
> > -       val = strtol(int_node->data, &endptr, base);
> > -       if (errno != 0 || endptr == int_node->data || *endptr != '\0') {
> > +       val = strtoul(int_node->data, &endptr, base);
> > +       if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) {
>
> I wonder if compilers/static analysis tools will balk on this as
> strtoul's return, an unsigned long,
> on a 32 bit machine will be 32 bits, so this could have a dead
> expression as val > UINT32_MAX
> will always be false. Perhaps use the strtoull variant to always have 64 bits?

In my humble opinion, a compiler or a static analyzer which warn about
the fact that "comparing an unsigned long value to UINT32_MAX is
always true" have an issue, because this seems to be the most natural
way of checking that a potentially-64-bit number can be safely
downcasted to 32 bits.

I find the suggestion of using strtoull to get a 32-bit integer to be
very hackish, considering that on 32-bit systems, strtoul does the job
fine (returning with errno = ERANGE when the value is too large) and
64-bit integers are using pairs of registers to be stored. If this
code ever causes issues with some compilers, some preprocessor logic
(such as "#if ULONG_MAX > UINT32_MAX") could be added to hide "val >
UINT32_MAX" from 32-bit compilers. Nevertheless in an effort to keep
the amount of preprocessor code as low as possible, I do not want to
include such logic right now.

In short, I am not willing to change this patch unless someone reports
a regression due to "val > UINT32_MAX".

Thanks for your review!
Nicolas

> >                 rc = SEPOL_ERR;
> >                 goto exit;
> >         }
> > @@ -5594,7 +5594,7 @@ int cil_fill_integer64(struct cil_tree_node *int_node, uint64_t *integer, int ba
> >         char *endptr = NULL;
> >         uint64_t val;
> >
> > -       if (int_node == NULL || integer == NULL) {
> > +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
> >                 goto exit;
> >         }
> >
> > --
> > 2.29.2
> >

Re: [PATCH 4/6] libsepol/cil: fix NULL pointer dereference when parsing an improper integer

[William Roberts]

On Sat, Jan 2, 2021 at 5:13 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Thu, Dec 31, 2020 at 4:04 PM William Roberts
> <bill.c.roberts@gmail.com> wrote:
> >
> > On Wed, Dec 30, 2020 at 4:09 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> > >
> > > OSS-Fuzz found a NULL pointer dereference when the CIL compiler tries to
> > > compile a policy with an invalid integer:
> > >
> > >     $ echo '(ioportcon(2())n)' > tmp.cil
> > >     $ secilc tmp.cil
> > >     Segmentation fault (core dumped)
> > >
> > > This is because strtol() is called with a NULL pointer, in
> > > cil_fill_integer().
> > >
> > > Fix this by checking that int_node->data is not NULL. While at it, use
> > > strtoul() instead of strtol() to parse an unsigned integer.
> > >
> > > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28456
> > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > > ---
> > >  libsepol/cil/src/cil_build_ast.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> > > index 67801def0dc0..0c9015cef578 100644
> > > --- a/libsepol/cil/src/cil_build_ast.c
> > > +++ b/libsepol/cil/src/cil_build_ast.c
> > > @@ -5566,15 +5566,15 @@ int cil_fill_integer(struct cil_tree_node *int_node, uint32_t *integer, int base
> > >  {
> > >         int rc = SEPOL_ERR;
> > >         char *endptr = NULL;
> > > -       int val;
> > > +       unsigned long val;
> > >
> > > -       if (int_node == NULL || integer == NULL) {
> > > +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
> > >                 goto exit;
> > >         }
> > >
> > >         errno = 0;
> > > -       val = strtol(int_node->data, &endptr, base);
> > > -       if (errno != 0 || endptr == int_node->data || *endptr != '\0') {
> > > +       val = strtoul(int_node->data, &endptr, base);
> > > +       if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) {
> >
> > I wonder if compilers/static analysis tools will balk on this as
> > strtoul's return, an unsigned long,
> > on a 32 bit machine will be 32 bits, so this could have a dead
> > expression as val > UINT32_MAX
> > will always be false. Perhaps use the strtoull variant to always have 64 bits?
>
> In my humble opinion, a compiler or a static analyzer which warn about
> the fact that "comparing an unsigned long value to UINT32_MAX is
> always true" have an issue, because this seems to be the most natural
> way of checking that a potentially-64-bit number can be safely
> downcasted to 32 bits.
>
> I find the suggestion of using strtoull to get a 32-bit integer to be
> very hackish, considering that on 32-bit systems, strtoul does the job
> fine (returning with errno = ERANGE when the value is too large) and
> 64-bit integers are using pairs of registers to be stored. If this
> code ever causes issues with some compilers, some preprocessor logic
> (such as "#if ULONG_MAX > UINT32_MAX") could be added to hide "val >
> UINT32_MAX" from 32-bit compilers. Nevertheless in an effort to keep
> the amount of preprocessor code as low as possible, I do not want to
> include such logic right now.

The reason I bring this up, is that I've personally encountered this
on Android when porting
libraries before. But these we're with much older GCC/CLANG variants.
When Android's
build went from gcc to clang, I remember clang being extra noisy and
these -Wtype-limits
warnings being a constant battle when trying to enable -Wextra.

t really boils down to the warning (per the man page):
-Wtype-limits
Warn if a comparison is always true or always false due to the limited
range of the data type, but do not warn for constant expressions. For
example, warn if an unsigned variable is compared against zero with <
or >=. This warning is also enabled by -Wextra.

But I tested this with an arm32v7 Docker Container running Ubuntu 20.04 using:
- GCC 9.3.0
- CLANG 10.0

as well as the NDK clang version Android (6454773 based on r365631c2)
clang version 9.0.8
cross compiling from x86_64 machine and it doesn't reproduce.

I wonder if they changed the behavior a bit because of having to
deal with this error all the time, and it was no fun. What's more
perplexing is that the uint8_t and uint16_t cases
trigger the warning and the uint32_t and uint64_t cases do not. I can
understand skipping the warning on
the unsigned long because of the word length changing from 8 to 4
bytes, and the 4 byte check
ending up being always false and harmless, but in the fixed width
case... I wonder why the compiler gods chose so.

        uint8_t y = (uint8_t)argc;
        if(y > UINT8_MAX) { printf("foo: %u\n", y); }  // always false

        uint16_t w = (uint16_t)argc;
        if(w > UINT16_MAX) { printf("foo: %u\n", w); }  // always false

        uint32_t q = (uint32_t)argc;
        if(q > UINT32_MAX) { printf("foo: %u\n", q); }  // always false

        uint64_t z = (uint64_t)argc;
        if(z > UINT64_MAX) { printf("foo: %llu\n", z); }  // always false

a.c:23:7: warning: comparison is always false due to limited range of
data type [-Wtype-limits]
   23 |  if(y > UINT8_MAX) { printf("foo: %u\n", y); }  // always false

>
> In short, I am not willing to change this patch unless someone reports
> a regression due to "val > UINT32_MAX".

Based on my tests with modern versions of toolchains, I think we're
fine. I don't know why
we're fine, but we appear so.

>
> Thanks for your review!
> Nicolas

ack for the whole series since this was my only worry.

>
> > >                 rc = SEPOL_ERR;
> > >                 goto exit;
> > >         }
> > > @@ -5594,7 +5594,7 @@ int cil_fill_integer64(struct cil_tree_node *int_node, uint64_t *integer, int ba
> > >         char *endptr = NULL;
> > >         uint64_t val;
> > >
> > > -       if (int_node == NULL || integer == NULL) {
> > > +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
> > >                 goto exit;
> > >         }
> > >
> > > --
> > > 2.29.2
> > >
>

Re: [PATCH 4/6] libsepol/cil: fix NULL pointer dereference when parsing an improper integer

[James Carter]

On Wed, Dec 30, 2020 at 5:09 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> OSS-Fuzz found a NULL pointer dereference when the CIL compiler tries to
> compile a policy with an invalid integer:
>
>     $ echo '(ioportcon(2())n)' > tmp.cil
>     $ secilc tmp.cil
>     Segmentation fault (core dumped)
>
> This is because strtol() is called with a NULL pointer, in
> cil_fill_integer().
>
> Fix this by checking that int_node->data is not NULL. While at it, use
> strtoul() instead of strtol() to parse an unsigned integer.
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28456
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_build_ast.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index 67801def0dc0..0c9015cef578 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -5566,15 +5566,15 @@ int cil_fill_integer(struct cil_tree_node *int_node, uint32_t *integer, int base
>  {
>         int rc = SEPOL_ERR;
>         char *endptr = NULL;
> -       int val;
> +       unsigned long val;
>
> -       if (int_node == NULL || integer == NULL) {
> +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
>                 goto exit;
>         }
>
>         errno = 0;
> -       val = strtol(int_node->data, &endptr, base);
> -       if (errno != 0 || endptr == int_node->data || *endptr != '\0') {
> +       val = strtoul(int_node->data, &endptr, base);
> +       if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) {
>                 rc = SEPOL_ERR;
>                 goto exit;
>         }
> @@ -5594,7 +5594,7 @@ int cil_fill_integer64(struct cil_tree_node *int_node, uint64_t *integer, int ba
>         char *endptr = NULL;
>         uint64_t val;
>
> -       if (int_node == NULL || integer == NULL) {
> +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
>                 goto exit;
>         }
>
> --
> 2.29.2
>

Re: [PATCH 4/6] libsepol/cil: fix NULL pointer dereference when parsing an improper integer

[William Roberts]

On Mon, Jan 4, 2021 at 11:03 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Wed, Dec 30, 2020 at 5:09 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > OSS-Fuzz found a NULL pointer dereference when the CIL compiler tries to
> > compile a policy with an invalid integer:
> >
> >     $ echo '(ioportcon(2())n)' > tmp.cil
> >     $ secilc tmp.cil
> >     Segmentation fault (core dumped)
> >
> > This is because strtol() is called with a NULL pointer, in
> > cil_fill_integer().
> >
> > Fix this by checking that int_node->data is not NULL. While at it, use
> > strtoul() instead of strtol() to parse an unsigned integer.
> >
> > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28456
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>
> > ---
> >  libsepol/cil/src/cil_build_ast.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> > index 67801def0dc0..0c9015cef578 100644
> > --- a/libsepol/cil/src/cil_build_ast.c
> > +++ b/libsepol/cil/src/cil_build_ast.c
> > @@ -5566,15 +5566,15 @@ int cil_fill_integer(struct cil_tree_node *int_node, uint32_t *integer, int base
> >  {
> >         int rc = SEPOL_ERR;
> >         char *endptr = NULL;
> > -       int val;
> > +       unsigned long val;
> >
> > -       if (int_node == NULL || integer == NULL) {
> > +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
> >                 goto exit;
> >         }
> >
> >         errno = 0;
> > -       val = strtol(int_node->data, &endptr, base);
> > -       if (errno != 0 || endptr == int_node->data || *endptr != '\0') {
> > +       val = strtoul(int_node->data, &endptr, base);
> > +       if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) {
> >                 rc = SEPOL_ERR;
> >                 goto exit;
> >         }
> > @@ -5594,7 +5594,7 @@ int cil_fill_integer64(struct cil_tree_node *int_node, uint64_t *integer, int ba
> >         char *endptr = NULL;
> >         uint64_t val;
> >
> > -       if (int_node == NULL || integer == NULL) {
> > +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
> >                 goto exit;
> >         }
> >
> > --
> > 2.29.2
> >

It turns out when GCC fixes a bug with -Wtype-limits, this will cause
a regression. The current top-level Makefile includes
exporting CFLAGS -Wextra which will enable this warning. I find it
surprising this has been a known gcc issue for some time
and that clang has the same bug.

See:
  - https://gcc.gnu.org/pipermail/gcc-help/2021-January/139755.html

I would fix it now.

Bill

[PATCH 5/6] libsepol/cil: fix out-of-bound read in cil_print_recursive_blockinherit

[Nicolas Iooss]

OSS-Fuzz found a heap buffer overflow (out-of-bound reads) when the CIL
compiler tries to report a recursive blockinherit with an optional
block:

    $ echo '(block b (optional o (blockinherit b)))' > tmp.cil
    $ secilc tmp.cil
    Segmentation fault (core dumped)

This is because cil_print_recursive_blockinherit() assumes that all
nodes are either CIL_BLOCK or CIL_BLOCKINHERIT. Add support for other
block kinds, using cil_node_to_string() to show them.

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

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index f6deb1002fbd..ecd05dfa5dab 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -2343,11 +2343,13 @@ void cil_print_recursive_blockinherit(struct cil_tree_node *bi_node, struct cil_
 	for (curr = bi_node; curr != terminating_node; curr = curr->parent) {
 		if (curr->flavor == CIL_BLOCK) {
 			cil_list_prepend(trace, CIL_NODE, curr);
-		} else {
+		} else if (curr->flavor == CIL_BLOCKINHERIT) {
 			if (curr != bi_node) {
 				cil_list_prepend(trace, CIL_NODE, NODE(((struct cil_blockinherit *)curr->data)->block));
 			}
 			cil_list_prepend(trace, CIL_NODE, curr);
+		} else {
+			cil_list_prepend(trace, CIL_NODE, curr);
 		}
 	}
 	cil_list_prepend(trace, CIL_NODE, terminating_node);
@@ -2356,8 +2358,12 @@ void cil_print_recursive_blockinherit(struct cil_tree_node *bi_node, struct cil_
 		curr = item->data;
 		if (curr->flavor == CIL_BLOCK) {
 			cil_tree_log(curr, CIL_ERR, "block %s", DATUM(curr->data)->name);
-		} else {
+		} else if (curr->flavor == CIL_BLOCKINHERIT) {
 			cil_tree_log(curr, CIL_ERR, "blockinherit %s", ((struct cil_blockinherit *)curr->data)->block_str);
+		} else if (curr->flavor == CIL_OPTIONAL) {
+			cil_tree_log(curr, CIL_ERR, "optional %s", DATUM(curr->data)->name);
+		} else {
+			cil_tree_log(curr, CIL_ERR, "%s", cil_node_to_string(curr));
 		}
 	}
 
-- 
2.29.2

Re: [PATCH 5/6] libsepol/cil: fix out-of-bound read in cil_print_recursive_blockinherit

[James Carter]

On Wed, Dec 30, 2020 at 5:09 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> OSS-Fuzz found a heap buffer overflow (out-of-bound reads) when the CIL
> compiler tries to report a recursive blockinherit with an optional
> block:
>
>     $ echo '(block b (optional o (blockinherit b)))' > tmp.cil
>     $ secilc tmp.cil
>     Segmentation fault (core dumped)
>
> This is because cil_print_recursive_blockinherit() assumes that all
> nodes are either CIL_BLOCK or CIL_BLOCKINHERIT. Add support for other
> block kinds, using cil_node_to_string() to show them.
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28462
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_resolve_ast.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index f6deb1002fbd..ecd05dfa5dab 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -2343,11 +2343,13 @@ void cil_print_recursive_blockinherit(struct cil_tree_node *bi_node, struct cil_
>         for (curr = bi_node; curr != terminating_node; curr = curr->parent) {
>                 if (curr->flavor == CIL_BLOCK) {
>                         cil_list_prepend(trace, CIL_NODE, curr);
> -               } else {
> +               } else if (curr->flavor == CIL_BLOCKINHERIT) {
>                         if (curr != bi_node) {
>                                 cil_list_prepend(trace, CIL_NODE, NODE(((struct cil_blockinherit *)curr->data)->block));
>                         }
>                         cil_list_prepend(trace, CIL_NODE, curr);
> +               } else {
> +                       cil_list_prepend(trace, CIL_NODE, curr);
>                 }
>         }
>         cil_list_prepend(trace, CIL_NODE, terminating_node);
> @@ -2356,8 +2358,12 @@ void cil_print_recursive_blockinherit(struct cil_tree_node *bi_node, struct cil_
>                 curr = item->data;
>                 if (curr->flavor == CIL_BLOCK) {
>                         cil_tree_log(curr, CIL_ERR, "block %s", DATUM(curr->data)->name);
> -               } else {
> +               } else if (curr->flavor == CIL_BLOCKINHERIT) {
>                         cil_tree_log(curr, CIL_ERR, "blockinherit %s", ((struct cil_blockinherit *)curr->data)->block_str);
> +               } else if (curr->flavor == CIL_OPTIONAL) {
> +                       cil_tree_log(curr, CIL_ERR, "optional %s", DATUM(curr->data)->name);
> +               } else {
> +                       cil_tree_log(curr, CIL_ERR, "%s", cil_node_to_string(curr));
>                 }
>         }
>
> --
> 2.29.2
>

Re: [PATCH 5/6] libsepol/cil: fix out-of-bound read in cil_print_recursive_blockinherit

[James Carter]

Applied.

Thanks,
Jim

On Mon, Jan 4, 2021 at 1:17 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Wed, Dec 30, 2020 at 5:09 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > OSS-Fuzz found a heap buffer overflow (out-of-bound reads) when the CIL
> > compiler tries to report a recursive blockinherit with an optional
> > block:
> >
> >     $ echo '(block b (optional o (blockinherit b)))' > tmp.cil
> >     $ secilc tmp.cil
> >     Segmentation fault (core dumped)
> >
> > This is because cil_print_recursive_blockinherit() assumes that all
> > nodes are either CIL_BLOCK or CIL_BLOCKINHERIT. Add support for other
> > block kinds, using cil_node_to_string() to show them.
> >
> > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28462
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>
> > ---
> >  libsepol/cil/src/cil_resolve_ast.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > index f6deb1002fbd..ecd05dfa5dab 100644
> > --- a/libsepol/cil/src/cil_resolve_ast.c
> > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > @@ -2343,11 +2343,13 @@ void cil_print_recursive_blockinherit(struct cil_tree_node *bi_node, struct cil_
> >         for (curr = bi_node; curr != terminating_node; curr = curr->parent) {
> >                 if (curr->flavor == CIL_BLOCK) {
> >                         cil_list_prepend(trace, CIL_NODE, curr);
> > -               } else {
> > +               } else if (curr->flavor == CIL_BLOCKINHERIT) {
> >                         if (curr != bi_node) {
> >                                 cil_list_prepend(trace, CIL_NODE, NODE(((struct cil_blockinherit *)curr->data)->block));
> >                         }
> >                         cil_list_prepend(trace, CIL_NODE, curr);
> > +               } else {
> > +                       cil_list_prepend(trace, CIL_NODE, curr);
> >                 }
> >         }
> >         cil_list_prepend(trace, CIL_NODE, terminating_node);
> > @@ -2356,8 +2358,12 @@ void cil_print_recursive_blockinherit(struct cil_tree_node *bi_node, struct cil_
> >                 curr = item->data;
> >                 if (curr->flavor == CIL_BLOCK) {
> >                         cil_tree_log(curr, CIL_ERR, "block %s", DATUM(curr->data)->name);
> > -               } else {
> > +               } else if (curr->flavor == CIL_BLOCKINHERIT) {
> >                         cil_tree_log(curr, CIL_ERR, "blockinherit %s", ((struct cil_blockinherit *)curr->data)->block_str);
> > +               } else if (curr->flavor == CIL_OPTIONAL) {
> > +                       cil_tree_log(curr, CIL_ERR, "optional %s", DATUM(curr->data)->name);
> > +               } else {
> > +                       cil_tree_log(curr, CIL_ERR, "%s", cil_node_to_string(curr));
> >                 }
> >         }
> >
> > --
> > 2.29.2
> >

[PATCH 6/6] libsepol/cil: destroy perm_datums when __cil_resolve_perms fails

[Nicolas Iooss]

When __cil_resolve_perms fails, it does not destroy perm_datums, which
leads to a memory leak reported by OSS-Fuzz with the following CIL
policy:

    (class cl01())
    (classorder(cl01))
    (type at02)
    (type tpr3)
    (allow at02 tpr3(cl01((s))))

Calling cil_list_destroy() fixes the issue.

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

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index ecd05dfa5dab..255f17ae7e30 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -146,6 +146,7 @@ static int __cil_resolve_perms(symtab_t *class_symtab, symtab_t *common_symtab,
 	return SEPOL_OK;
 
 exit:
+	cil_list_destroy(perm_datums, CIL_FALSE);
 	return rc;
 }
 
-- 
2.29.2

Re: [PATCH 6/6] libsepol/cil: destroy perm_datums when __cil_resolve_perms fails

[William Roberts]

On Wed, Dec 30, 2020 at 4:10 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> When __cil_resolve_perms fails, it does not destroy perm_datums, which
> leads to a memory leak reported by OSS-Fuzz with the following CIL
> policy:
>
>     (class cl01())
>     (classorder(cl01))
>     (type at02)
>     (type tpr3)
>     (allow at02 tpr3(cl01((s))))
>
> Calling cil_list_destroy() fixes the issue.
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28466
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/cil/src/cil_resolve_ast.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index ecd05dfa5dab..255f17ae7e30 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -146,6 +146,7 @@ static int __cil_resolve_perms(symtab_t *class_symtab, symtab_t *common_symtab,
>         return SEPOL_OK;
>
>  exit:
> +       cil_list_destroy(perm_datums, CIL_FALSE);
>         return rc;
>  }
>
> --
> 2.29.2
>

ack on all but patch #4, comments sent for that patch.

Re: [PATCH 6/6] libsepol/cil: destroy perm_datums when __cil_resolve_perms fails

[James Carter]

On Wed, Dec 30, 2020 at 5:10 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> When __cil_resolve_perms fails, it does not destroy perm_datums, which
> leads to a memory leak reported by OSS-Fuzz with the following CIL
> policy:
>
>     (class cl01())
>     (classorder(cl01))
>     (type at02)
>     (type tpr3)
>     (allow at02 tpr3(cl01((s))))
>
> Calling cil_list_destroy() fixes the issue.
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28466
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_resolve_ast.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index ecd05dfa5dab..255f17ae7e30 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -146,6 +146,7 @@ static int __cil_resolve_perms(symtab_t *class_symtab, symtab_t *common_symtab,
>         return SEPOL_OK;
>
>  exit:
> +       cil_list_destroy(perm_datums, CIL_FALSE);
>         return rc;
>  }
>
> --
> 2.29.2
>

Re: [PATCH 6/6] libsepol/cil: destroy perm_datums when __cil_resolve_perms fails

[James Carter]

Applied.

Thanks,
Jim

On Mon, Jan 4, 2021 at 1:18 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Wed, Dec 30, 2020 at 5:10 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > When __cil_resolve_perms fails, it does not destroy perm_datums, which
> > leads to a memory leak reported by OSS-Fuzz with the following CIL
> > policy:
> >
> >     (class cl01())
> >     (classorder(cl01))
> >     (type at02)
> >     (type tpr3)
> >     (allow at02 tpr3(cl01((s))))
> >
> > Calling cil_list_destroy() fixes the issue.
> >
> > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28466
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>
> > ---
> >  libsepol/cil/src/cil_resolve_ast.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > index ecd05dfa5dab..255f17ae7e30 100644
> > --- a/libsepol/cil/src/cil_resolve_ast.c
> > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > @@ -146,6 +146,7 @@ static int __cil_resolve_perms(symtab_t *class_symtab, symtab_t *common_symtab,
> >         return SEPOL_OK;
> >
> >  exit:
> > +       cil_list_destroy(perm_datums, CIL_FALSE);
> >         return rc;
> >  }
> >
> > --
> > 2.29.2
> >

Re: [PATCH 1/6] libsepol: do not decode out-of-bound rolebounds

[James Carter]

[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]

On Wed, Dec 30, 2020 at 5:11 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> While fuzzing /usr/libexec/hll/pp, a policy module was generated with a
> role->bounds larger that the number of roles in the policy.
>
> This issue has been found while fuzzing hll/pp with the American Fuzzy
> Lop.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/src/module_to_cil.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index a87bc15e7610..c99790eb76e7 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -2165,7 +2165,9 @@ static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN
>                         }
>                 }
>
> -               if (role->bounds > 0) {
> +               if (role->bounds >= pdb->p_roles.nprim) {
> +                       log_err("Warning: role %s defines an out-of-bound rolebounds", key);
> +               } else if (role->bounds > 0) {
>                         cil_println(indent, "(rolebounds %s %s)", key, pdb->p_role_val_to_name[role->bounds - 1]);
>                 }
>                 break;
> --
> 2.29.2
>

There are other places where the bounds value is used as an index and
type datums also have bounds that are used in the same way.

Correct me if I am wrong, but I think that this can only occur by
crafting a binary (and not as a result of a policy being compiled). So
I think the correct place for the check would be when the binary file
is read.

I'll have to test to be sure, but I think the attached patch might do
the proper checking.

Jim

[-- Attachment #2: bounds.patch --]
[-- Type: text/x-patch, Size: 909 bytes --]

diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index ce8f3ad7..f8839539 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -1030,6 +1030,8 @@ static int role_index(hashtab_key_t key, hashtab_datum_t datum, void *datap)
 		return -EINVAL;
 	if (p->p_role_val_to_name[role->s.value - 1] != NULL)
 		return -EINVAL;
+	if (role->bounds > p->p_roles.nprim)
+		return _EINVAL;
 	p->p_role_val_to_name[role->s.value - 1] = (char *)key;
 	p->role_val_to_struct[role->s.value - 1] = role;
 
@@ -1049,6 +1051,8 @@ static int type_index(hashtab_key_t key, hashtab_datum_t datum, void *datap)
 			return -EINVAL;
 		if (p->p_type_val_to_name[typdatum->s.value - 1] != NULL)
 			return -EINVAL;
+		if (typedatum->bounds > p->p_types.nprim)
+			return -EINVAL;
 		p->p_type_val_to_name[typdatum->s.value - 1] = (char *)key;
 		p->type_val_to_struct[typdatum->s.value - 1] = typdatum;
 	}

Re: [PATCH 1/6] libsepol: do not decode out-of-bound rolebounds

[James Carter]

[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]

On Mon, Jan 4, 2021 at 10:48 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Wed, Dec 30, 2020 at 5:11 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > While fuzzing /usr/libexec/hll/pp, a policy module was generated with a
> > role->bounds larger that the number of roles in the policy.
> >
> > This issue has been found while fuzzing hll/pp with the American Fuzzy
> > Lop.
> >
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > ---
> >  libsepol/src/module_to_cil.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> > index a87bc15e7610..c99790eb76e7 100644
> > --- a/libsepol/src/module_to_cil.c
> > +++ b/libsepol/src/module_to_cil.c
> > @@ -2165,7 +2165,9 @@ static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN
> >                         }
> >                 }
> >
> > -               if (role->bounds > 0) {
> > +               if (role->bounds >= pdb->p_roles.nprim) {
> > +                       log_err("Warning: role %s defines an out-of-bound rolebounds", key);
> > +               } else if (role->bounds > 0) {
> >                         cil_println(indent, "(rolebounds %s %s)", key, pdb->p_role_val_to_name[role->bounds - 1]);
> >                 }
> >                 break;
> > --
> > 2.29.2
> >
>
> There are other places where the bounds value is used as an index and
> type datums also have bounds that are used in the same way.
>
> Correct me if I am wrong, but I think that this can only occur by
> crafting a binary (and not as a result of a policy being compiled). So
> I think the correct place for the check would be when the binary file
> is read.
>
> I'll have to test to be sure, but I think the attached patch might do
> the proper checking.
>

Oops, that patch had typos. This one.

> Jim

[-- Attachment #2: bounds.patch --]
[-- Type: text/x-patch, Size: 908 bytes --]

diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index ce8f3ad7..6faaaa58 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -1030,6 +1030,8 @@ static int role_index(hashtab_key_t key, hashtab_datum_t datum, void *datap)
 		return -EINVAL;
 	if (p->p_role_val_to_name[role->s.value - 1] != NULL)
 		return -EINVAL;
+	if (role->bounds > p->p_roles.nprim)
+		return -EINVAL;
 	p->p_role_val_to_name[role->s.value - 1] = (char *)key;
 	p->role_val_to_struct[role->s.value - 1] = role;
 
@@ -1049,6 +1051,8 @@ static int type_index(hashtab_key_t key, hashtab_datum_t datum, void *datap)
 			return -EINVAL;
 		if (p->p_type_val_to_name[typdatum->s.value - 1] != NULL)
 			return -EINVAL;
+		if (typdatum->bounds > p->p_types.nprim)
+			return -EINVAL;
 		p->p_type_val_to_name[typdatum->s.value - 1] = (char *)key;
 		p->type_val_to_struct[typdatum->s.value - 1] = typdatum;
 	}

Re: [PATCH 1/6] libsepol: do not decode out-of-bound rolebounds

[Nicolas Iooss]

On Mon, Jan 4, 2021 at 4:51 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Mon, Jan 4, 2021 at 10:48 AM James Carter <jwcart2@gmail.com> wrote:
> >
> > On Wed, Dec 30, 2020 at 5:11 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> > >
> > > While fuzzing /usr/libexec/hll/pp, a policy module was generated with a
> > > role->bounds larger that the number of roles in the policy.
> > >
> > > This issue has been found while fuzzing hll/pp with the American Fuzzy
> > > Lop.
> > >
> > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > > ---
> > >  libsepol/src/module_to_cil.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> > > index a87bc15e7610..c99790eb76e7 100644
> > > --- a/libsepol/src/module_to_cil.c
> > > +++ b/libsepol/src/module_to_cil.c
> > > @@ -2165,7 +2165,9 @@ static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN
> > >                         }
> > >                 }
> > >
> > > -               if (role->bounds > 0) {
> > > +               if (role->bounds >= pdb->p_roles.nprim) {
> > > +                       log_err("Warning: role %s defines an out-of-bound rolebounds", key);
> > > +               } else if (role->bounds > 0) {
> > >                         cil_println(indent, "(rolebounds %s %s)", key, pdb->p_role_val_to_name[role->bounds - 1]);
> > >                 }
> > >                 break;
> > > --
> > > 2.29.2
> > >
> >
> > There are other places where the bounds value is used as an index and
> > type datums also have bounds that are used in the same way.
> >
> > Correct me if I am wrong, but I think that this can only occur by
> > crafting a binary (and not as a result of a policy being compiled). So
> > I think the correct place for the check would be when the binary file
> > is read.
> >
> > I'll have to test to be sure, but I think the attached patch might do
> > the proper checking.
> >
>
> Oops, that patch had typos. This one.

I agree, and I confirm your patch fixed the crash I experienced. Thanks!
Nicolas