On Wed, Jan 6, 2021 at 3:09 AM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > While fuzzing /usr/libexec/hll/pp, a policy module was generated which > made "key" be NULL in required_scopes_to_cil() when doing: > > key = pdb->sym_val_to_name[sym][i]; > > This was caused by using a decl->required.scope[sym] bitmap which was > not consistent with the policy symbols. > > Ensure this consistency when loading a binary policy by introducing a > new function which is called after policydb_index_others(), so that > p->sym_val_to_name[sym] can be used to check whether a symbol exists, in > a performent way (instead of searching the hash table > p->symtab[sym].table). > > This issue has been found while fuzzing hll/pp with the American Fuzzy > Lop. > > Signed-off-by: Nicolas Iooss <nicolas.iooss@xxxxxxx> > --- > This replaces patch "libsepol: ensure that hashtab_search is not called > with a NULL key" > (https://lore.kernel.org/selinux/CAP+JOzQcM03WUJ-Fg2LuLxzCGiagJnuyJozv7ed6f34vnKEKXA@xxxxxxxxxxxxxx/T/#m059ac9bc2bdb9e4c436ebe3cef03124de25f1f06) > > libsepol/src/policydb.c | 48 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > index 6faaaa5895d0..f43d5c67463e 100644 > --- a/libsepol/src/policydb.c > +++ b/libsepol/src/policydb.c > @@ -1209,6 +1209,51 @@ int policydb_index_decls(sepol_handle_t * handle, policydb_t * p) > return 0; > } > > +/* > + * Verify the consistency of declarations, after the symbols were indexed > + */ > +int policydb_verify_decl_symbols(sepol_handle_t * handle, policydb_t * p) > +{ > + avrule_block_t *curblock; > + avrule_decl_t *decl; > + struct ebitmap_node *node; > + unsigned int i, sym; > + > + for (curblock = p->global; curblock != NULL; curblock = curblock->next) { > + for (decl = curblock->branch_list; decl != NULL; > + decl = decl->next) { > + for (sym = 0; sym < SYM_NUM; sym++) { > + ebitmap_for_each_positive_bit(&decl->declared.scope[sym], node, i) { > + if (i >= p->symtab[sym].nprim) { > + ERR(handle, "too large value for symbol in declared scope %u: %u >= %u", > + decl->decl_id, i, p->symtab[sym].nprim); > + return -1; > + } > + if (p->sym_val_to_name[sym][i] == NULL) { > + ERR(handle, "invalid symbol %u in declared scope %u", > + i, decl->decl_id); > + return -1; > + } > + } > + ebitmap_for_each_positive_bit(&decl->required.scope[sym], node, i) { > + if (i >= p->symtab[sym].nprim) { > + ERR(handle, "too large value for symbol in required scope %u: %u >= %u", > + decl->decl_id, i, p->symtab[sym].nprim); > + return -1; > + } > + if (p->sym_val_to_name[sym][i] == NULL) { > + ERR(handle, "invalid symbol %u in required scope %u", > + i, decl->decl_id); > + return -1; > + } > + } > + } > + } > + } > + > + return 0; > +} > + > /* > * Define the other val_to_name and val_to_struct arrays > * in a policy database structure. > @@ -4501,6 +4546,9 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose) > if (policydb_index_others(fp->handle, p, verbose)) > goto bad; > > + if (policydb_verify_decl_symbols(fp->handle, p)) > + goto bad; > + > if (ocontext_read(info, p, fp) == -1) { > goto bad; > } > -- > 2.30.0 > This checks modular policy, but not kernel. I am working on a patch (or patch set) to do more checking of the policy binary. I am glad you did this patch though, because I would have completely forgot about checking the parts for modular policies. I will incorporate your patch in with what I am doing. Thanks, Jim
