On Wed, Jan 20, 2021 at 5:09 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > 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 Merged. Thanks! Nicolas
