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

* [PATCH] libsepol/cil: Fix heap-use-after-free in __class_reset_perm_values()
@ 2021-01-05 21:54 James Carter
  2021-01-05 23:18 ` Nicolas Iooss
  0 siblings, 1 reply; 3+ messages in thread
From: James Carter @ 2021-01-05 21:54 UTC (permalink / raw)
  To: selinux; +Cc: James Carter, Nicolas Iooss

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 | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
index 43e6b88e..569f630b 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -9,6 +9,16 @@ static inline void cil_reset_level(struct cil_level *level);
 static inline void cil_reset_levelrange(struct cil_levelrange *levelrange);
 static inline void cil_reset_context(struct cil_context *context);
 
+static int __class_count_perms(__attribute__((unused)) hashtab_key_t k, __attribute__((unused)) hashtab_datum_t d, void *args)
+{
+	int *num = (int *)args;
+
+	(*num)++;
+
+	*((int *)args) = *num;
+
+	return SEPOL_OK;
+}
 
 static int __class_reset_perm_values(__attribute__((unused)) hashtab_key_t k, hashtab_datum_t d, void *args)
 {
@@ -22,11 +32,15 @@ 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_class_perms = 0;
+		int num_common_perms;
+		cil_symtab_map(&class->perms, __class_count_perms, &num_class_perms);
+		num_common_perms = class->num_perms - num_class_perms;
+		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 = num_class_perms;
 		class->common = NULL; /* Must make this NULL or there will be an error when re-resolving */
 	}
 	class->ordered = CIL_FALSE;
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 3+ messages in thread