On Tue, Jan 5, 2021 at 6:18 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > On Tue, Jan 5, 2021 at 10:54 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 | 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 > > > > I confirm this patch fixes the issue reported in > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28467. > Nevertheless counting the items of a symtab_t using a callback feels > strange. Can class->perms.table->nel (or a new accessor function that > returns the table->nel of a symtab_t structure) be used to replace > num_class_perms? > I don't know why it won't work. Let me give it a try and resend the patch. Thanks for the suggestion. Jim > Thanks, > Nicolas >
