On Wed, Jan 6, 2021 at 7:43 PM James Carter <jwcart2@xxxxxxxxx> 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@xxxxxxx> > Signed-off-by: James Carter <jwcart2@xxxxxxxxx> > --- > 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@xxxxxxx> (I also tested them and they work fine) Thanks! Nicolas
