Skip to content

Commit

Permalink
libsepol/cil: Fix heap-use-after-free in __class_reset_perm_values()
Browse files Browse the repository at this point in the history
Nicolas Iooss reports:
  A few weeks ago, OSS-Fuzz got configured in order to fuzz the CIL
  policy compiler (cf.
  SELinuxProject#215 and
  google/oss-fuzz#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>
  • Loading branch information
jwcart2 authored and fishilico committed Jan 14, 2021
1 parent 23a5e7f commit e5ae266
Showing 1 changed file with 4 additions and 3 deletions.
7 changes: 4 additions & 3 deletions libsepol/cil/src/cil_reset_ast.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit e5ae266

Please sign in to comment.