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? Thanks, Nicolas
