[PATCH 1/2] libsepol/cil: Update symtab nprim field when adding or removing datums

[PATCH 1/2] libsepol/cil: Update symtab nprim field when adding or removing datums

[James Carter]

This field is suppose to be used to track the number of primary names in
the symtab. It was not being updated or used.

Increment the nprim field when a new datum is added to the symtab and
decrement the field when a datum is removed.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_symtab.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libsepol/cil/src/cil_symtab.c b/libsepol/cil/src/cil_symtab.c
index 2970b863..579a888e 100644
--- a/libsepol/cil/src/cil_symtab.c
+++ b/libsepol/cil/src/cil_symtab.c
@@ -92,6 +92,7 @@ int cil_symtab_insert(symtab_t *symtab, hashtab_key_t key, struct cil_symtab_dat
 		datum->name = key;
 		datum->fqn = key;
 		datum->symtab = symtab;
+		symtab->nprim++;
 		cil_list_append(datum->nodes, CIL_NODE, node);
 	} else if (rc == SEPOL_EEXIST) {
 		cil_list_append(datum->nodes, CIL_NODE, node);
@@ -111,6 +112,7 @@ void cil_symtab_remove_datum(struct cil_symtab_datum *datum)
 	}
 
 	hashtab_remove(symtab->table, datum->name, NULL, NULL);
+	symtab->nprim--;
 	datum->symtab = NULL;
 }
 
-- 
2.25.4

[PATCH 2/2] libsepol/cil: Fix heap-use-after-free in __class_reset_perm_values()

[James Carter]

Nicolas Iooss reports:
  A few weeks ago, OSS-Fuzz got configured in order to fuzz the CIL
  policy compiler (cf.
  https://github.com/SELinuxProject/selinux/issues/215 and
  https://github.com/google/oss-fuzz/pull/4790). It reported a bunch of
  simple issues, for which I will submit patches. There are also more
  subtle bugs, like the one triggered by this CIL policy:

  (class CLASS (PERM))
  (classorder (CLASS))
  (sid SID)
  (sidorder (SID))
  (sensitivity SENS)
  (sensitivityorder (SENS))
  (type TYPE)
  (allow TYPE self (CLASS (PERM)))

  (block b
      (optional o
          (sensitivitycategory SENS (C)) ; Non-existing category
  triggers disabling the optional
          (common COMMON (PERM1))
          (classcommon CLASS COMMON)
          (allow TYPE self (CLASS (PERM1)))
      )
  )

  On my computer, secilc manages to build this policy fine, but when
  clang's Address Sanitizer is enabled, running secilc leads to the
  following report:

  $ make -C libsepol/src CC=clang CFLAGS='-g -fsanitize=address' libsepol.a
  $ clang -g -fsanitize=address secilc/secilc.c libsepol/src/libsepol.a
  -o my_secilc
  $ ./my_secilc -vv testcase.cil
  Parsing testcase.cil
  Building AST from Parse Tree
  Destroying Parse Tree
  Resolving AST
  Failed to resolve sensitivitycategory statement at testcase.cil:12
  Disabling optional 'o' at testcase.cil:11
  Resetting declarations
  =================================================================
  ==181743==ERROR: AddressSanitizer: heap-use-after-free on address
  0x6070000000c0 at pc 0x55ff7e445d24 bp 0x7ffe7eecfba0 sp
  0x7ffe7eecfb98
  READ of size 4 at 0x6070000000c0 thread T0
      #0 0x55ff7e445d23 in __class_reset_perm_values
  /git/selinux-userspace/libsepol/src/../cil/src/cil_reset_ast.c:17:17

The problem is that the optional branch is destroyed when it is disabled,
so the common has already been destroyed when the reset code tries to
access the number of common permissions, so that it can change the
value of the class permissions back to their original values.

The solution is to count the number of class permissions and then
calculate the number of common permissions.

Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_reset_ast.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
index 43e6b88e..52e5f640 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -22,11 +22,12 @@ static int __class_reset_perm_values(__attribute__((unused)) hashtab_key_t k, ha
 static void cil_reset_class(struct cil_class *class)
 {
 	if (class->common != NULL) {
-		struct cil_class *common = class->common;
-		cil_symtab_map(&class->perms, __class_reset_perm_values, &common->num_perms);
+		/* Must assume that the common has been destroyed */
+		int num_common_perms = class->num_perms - class->perms.nprim;
+		cil_symtab_map(&class->perms, __class_reset_perm_values, &num_common_perms);
 		/* during a re-resolve, we need to reset the common, so a classcommon
 		 * statement isn't seen as a duplicate */
-		class->num_perms -= common->num_perms;
+		class->num_perms = class->perms.nprim;
 		class->common = NULL; /* Must make this NULL or there will be an error when re-resolving */
 	}
 	class->ordered = CIL_FALSE;
-- 
2.25.4

Re: [PATCH 2/2] libsepol/cil: Fix heap-use-after-free in __class_reset_perm_values()

[Nicolas Iooss]

On Wed, Jan 6, 2021 at 7:43 PM James Carter <jwcart2@gmail.com> wrote:
>
> Nicolas Iooss reports:
>   A few weeks ago, OSS-Fuzz got configured in order to fuzz the CIL
>   policy compiler (cf.
>   https://github.com/SELinuxProject/selinux/issues/215 and
>   https://github.com/google/oss-fuzz/pull/4790). It reported a bunch of
>   simple issues, for which I will submit patches. There are also more
>   subtle bugs, like the one triggered by this CIL policy:
>
>   (class CLASS (PERM))
>   (classorder (CLASS))
>   (sid SID)
>   (sidorder (SID))
>   (sensitivity SENS)
>   (sensitivityorder (SENS))
>   (type TYPE)
>   (allow TYPE self (CLASS (PERM)))
>
>   (block b
>       (optional o
>           (sensitivitycategory SENS (C)) ; Non-existing category
>   triggers disabling the optional
>           (common COMMON (PERM1))
>           (classcommon CLASS COMMON)
>           (allow TYPE self (CLASS (PERM1)))
>       )
>   )
>
>   On my computer, secilc manages to build this policy fine, but when
>   clang's Address Sanitizer is enabled, running secilc leads to the
>   following report:
>
>   $ make -C libsepol/src CC=clang CFLAGS='-g -fsanitize=address' libsepol.a
>   $ clang -g -fsanitize=address secilc/secilc.c libsepol/src/libsepol.a
>   -o my_secilc
>   $ ./my_secilc -vv testcase.cil
>   Parsing testcase.cil
>   Building AST from Parse Tree
>   Destroying Parse Tree
>   Resolving AST
>   Failed to resolve sensitivitycategory statement at testcase.cil:12
>   Disabling optional 'o' at testcase.cil:11
>   Resetting declarations
>   =================================================================
>   ==181743==ERROR: AddressSanitizer: heap-use-after-free on address
>   0x6070000000c0 at pc 0x55ff7e445d24 bp 0x7ffe7eecfba0 sp
>   0x7ffe7eecfb98
>   READ of size 4 at 0x6070000000c0 thread T0
>       #0 0x55ff7e445d23 in __class_reset_perm_values
>   /git/selinux-userspace/libsepol/src/../cil/src/cil_reset_ast.c:17:17
>
> The problem is that the optional branch is destroyed when it is disabled,
> so the common has already been destroyed when the reset code tries to
> access the number of common permissions, so that it can change the
> value of the class permissions back to their original values.
>
> The solution is to count the number of class permissions and then
> calculate the number of common permissions.
>
> Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/src/cil_reset_ast.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
> index 43e6b88e..52e5f640 100644
> --- a/libsepol/cil/src/cil_reset_ast.c
> +++ b/libsepol/cil/src/cil_reset_ast.c
> @@ -22,11 +22,12 @@ static int __class_reset_perm_values(__attribute__((unused)) hashtab_key_t k, ha
>  static void cil_reset_class(struct cil_class *class)
>  {
>         if (class->common != NULL) {
> -               struct cil_class *common = class->common;
> -               cil_symtab_map(&class->perms, __class_reset_perm_values, &common->num_perms);
> +               /* Must assume that the common has been destroyed */
> +               int num_common_perms = class->num_perms - class->perms.nprim;
> +               cil_symtab_map(&class->perms, __class_reset_perm_values, &num_common_perms);
>                 /* during a re-resolve, we need to reset the common, so a classcommon
>                  * statement isn't seen as a duplicate */
> -               class->num_perms -= common->num_perms;
> +               class->num_perms = class->perms.nprim;
>                 class->common = NULL; /* Must make this NULL or there will be an error when re-resolving */
>         }
>         class->ordered = CIL_FALSE;
> --
> 2.25.4
>

For the 2 patches: Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org> (I
also tested them and they work fine)

Thanks!
Nicolas

Re: [PATCH 2/2] libsepol/cil: Fix heap-use-after-free in __class_reset_perm_values()

[Nicolas Iooss]

On Wed, Jan 20, 2021 at 5:09 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Wed, Jan 6, 2021 at 7:43 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > Nicolas Iooss reports:
> >   A few weeks ago, OSS-Fuzz got configured in order to fuzz the CIL
> >   policy compiler (cf.
> >   https://github.com/SELinuxProject/selinux/issues/215 and
> >   https://github.com/google/oss-fuzz/pull/4790). It reported a bunch of
> >   simple issues, for which I will submit patches. There are also more
> >   subtle bugs, like the one triggered by this CIL policy:
> >
> >   (class CLASS (PERM))
> >   (classorder (CLASS))
> >   (sid SID)
> >   (sidorder (SID))
> >   (sensitivity SENS)
> >   (sensitivityorder (SENS))
> >   (type TYPE)
> >   (allow TYPE self (CLASS (PERM)))
> >
> >   (block b
> >       (optional o
> >           (sensitivitycategory SENS (C)) ; Non-existing category
> >   triggers disabling the optional
> >           (common COMMON (PERM1))
> >           (classcommon CLASS COMMON)
> >           (allow TYPE self (CLASS (PERM1)))
> >       )
> >   )
> >
> >   On my computer, secilc manages to build this policy fine, but when
> >   clang's Address Sanitizer is enabled, running secilc leads to the
> >   following report:
> >
> >   $ make -C libsepol/src CC=clang CFLAGS='-g -fsanitize=address' libsepol.a
> >   $ clang -g -fsanitize=address secilc/secilc.c libsepol/src/libsepol.a
> >   -o my_secilc
> >   $ ./my_secilc -vv testcase.cil
> >   Parsing testcase.cil
> >   Building AST from Parse Tree
> >   Destroying Parse Tree
> >   Resolving AST
> >   Failed to resolve sensitivitycategory statement at testcase.cil:12
> >   Disabling optional 'o' at testcase.cil:11
> >   Resetting declarations
> >   =================================================================
> >   ==181743==ERROR: AddressSanitizer: heap-use-after-free on address
> >   0x6070000000c0 at pc 0x55ff7e445d24 bp 0x7ffe7eecfba0 sp
> >   0x7ffe7eecfb98
> >   READ of size 4 at 0x6070000000c0 thread T0
> >       #0 0x55ff7e445d23 in __class_reset_perm_values
> >   /git/selinux-userspace/libsepol/src/../cil/src/cil_reset_ast.c:17:17
> >
> > The problem is that the optional branch is destroyed when it is disabled,
> > so the common has already been destroyed when the reset code tries to
> > access the number of common permissions, so that it can change the
> > value of the class permissions back to their original values.
> >
> > The solution is to count the number of class permissions and then
> > calculate the number of common permissions.
> >
> > Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> >  libsepol/cil/src/cil_reset_ast.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
> > index 43e6b88e..52e5f640 100644
> > --- a/libsepol/cil/src/cil_reset_ast.c
> > +++ b/libsepol/cil/src/cil_reset_ast.c
> > @@ -22,11 +22,12 @@ static int __class_reset_perm_values(__attribute__((unused)) hashtab_key_t k, ha
> >  static void cil_reset_class(struct cil_class *class)
> >  {
> >         if (class->common != NULL) {
> > -               struct cil_class *common = class->common;
> > -               cil_symtab_map(&class->perms, __class_reset_perm_values, &common->num_perms);
> > +               /* Must assume that the common has been destroyed */
> > +               int num_common_perms = class->num_perms - class->perms.nprim;
> > +               cil_symtab_map(&class->perms, __class_reset_perm_values, &num_common_perms);
> >                 /* during a re-resolve, we need to reset the common, so a classcommon
> >                  * statement isn't seen as a duplicate */
> > -               class->num_perms -= common->num_perms;
> > +               class->num_perms = class->perms.nprim;
> >                 class->common = NULL; /* Must make this NULL or there will be an error when re-resolving */
> >         }
> >         class->ordered = CIL_FALSE;
> > --
> > 2.25.4
> >
>
> For the 2 patches: Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org> (I
> also tested them and they work fine)
>
> Thanks!
> Nicolas

Merged. Thanks!
Nicolas