On Mon, Oct 4, 2021 at 6:13 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > On Tue, Sep 28, 2021 at 7:55 PM James Carter <jwcart2@xxxxxxxxx> wrote: > > > > Type bounds are checked when creating the CIL binary using libsepol > > functions on the binary policy db. The bad rule is reported and, to > > provide better error reporting, a search is made for matching rules > > in the CIL policy. These matching rules as well as their parents are > > written out with their locations to make it easier to find the rules > > that violate the type bounds. > > > > It is possible to craft CIL policies where there are many rules > > that violate a bounds check each with many matching rules as well. > > This can make the error messages very difficult to deal with. For > > example, if there are 100 rules in the binary policy db that violate > > a type bounds and each of these rules has 100 matches, then 10,000 > > matching rules along with their parents will be written out as part > > of the error message. > > > > Limit the error reporting to two rules for each type bounds violation > > along with two matches for each of those rules. > > > > This problem was found with the secilc-fuzzer. > > > > Signed-off-by: James Carter <jwcart2@xxxxxxxxx> > > Hello, > I confirm this fixes the issue reported in > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=36246&q=selinux&can=2 > which can be reproduced with this CIL policy: > > (class CLASS (PERM)) > (classorder (CLASS)) > (sid SID) > (sidorder (SID)) > (user USER) > (role ROLE) > (type TYPE) > (category CAT) > (categoryorder (CAT)) > (sensitivity SENS) > (sensitivityorder (SENS)) > (sensitivitycategory SENS (CAT)) > (allow TYPE self (CLASS (PERM))) > (roletype ROLE TYPE) > (userrole USER ROLE) > (userlevel USER (SENS)) > (userrange USER ((SENS)(SENS (CAT)))) > (sidcontext SID (USER ROLE TYPE ((SENS)(SENS)))) > (boolean BOOL false) > (type TYPE_2) > (typeattribute TYPEATTR) > (block B0 > (blockinherit B1)(block B1 > (blockinherit B2)(block B2 > (blockinherit B3)(block B3 > (blockinherit B4)(block B4 > (blockinherit B5)(block B5 > (blockinherit B6)(block B6 > (blockinherit B7)(block B7 > (type TYPE_3) > (typebounds TYPE_2 TYPE_3) > (typeattributeset TYPEATTR TYPE_3) > (booleanif BOOL(true(allow TYPEATTR TYPE (CLASS (PERM))))) > )))))))) > > Acked-by: Nicolas Iooss <nicolas.iooss@xxxxxxx> > > By the way, even though the patch looks good to me, my personal > preference would have been to use "unsigned int" types (or size_t) to > count rules, instead of signed int. > I went with int types because numbad, which is assigned a value in the libsepol function bounds_check_type(), is an int. I would want them to all be the same type and I don't want to change bounds_check_type(), so I think that I will leave them as ints. Thanks for the review. Jim > Thanks, > Nicolas > > > --- > > libsepol/cil/src/cil_binary.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c > > index 4a80cb56..ec5f01e5 100644 > > --- a/libsepol/cil/src/cil_binary.c > > +++ b/libsepol/cil/src/cil_binary.c > > @@ -4825,6 +4825,7 @@ static int cil_check_type_bounds(const struct cil_db *db, policydb_t *pdb, void > > avtab_ptr_t cur; > > struct cil_avrule target; > > struct cil_tree_node *n1 = NULL; > > + int count_bad = 0; > > > > *violation = CIL_TRUE; > > > > @@ -4838,6 +4839,8 @@ static int cil_check_type_bounds(const struct cil_db *db, policydb_t *pdb, void > > for (cur = bad; cur; cur = cur->next) { > > struct cil_list_item *i2; > > struct cil_list *matching; > > + int num_matching = 0; > > + int count_matching = 0; > > > > rc = cil_avrule_from_sepol(pdb, cur, &target, type_value_to_cil, class_value_to_cil, perm_value_to_cil); > > if (rc != SEPOL_OK) { > > @@ -4855,6 +4858,9 @@ static int cil_check_type_bounds(const struct cil_db *db, policydb_t *pdb, void > > bounds_destroy_bad(bad); > > goto exit; > > } > > + cil_list_for_each(i2, matching) { > > + num_matching++; > > + } > > cil_list_for_each(i2, matching) { > > struct cil_tree_node *n2 = i2->data; > > struct cil_avrule *r2 = n2->data; > > @@ -4865,9 +4871,19 @@ static int cil_check_type_bounds(const struct cil_db *db, policydb_t *pdb, void > > __cil_print_parents(" ", n2); > > __cil_print_rule(" ", "allow", r2); > > } > > + count_matching++; > > + if (count_matching >= 2) { > > + cil_log(CIL_ERR, " Only first 2 of %d matching rules shown\n", num_matching); > > + break; > > + } > > } > > cil_list_destroy(&matching, CIL_FALSE); > > cil_list_destroy(&target.perms.classperms, CIL_TRUE); > > + count_bad++; > > + if (count_bad >= 2) { > > + cil_log(CIL_ERR, " Only first 2 of %d bad rules shown\n", numbad); > > + break; > > + } > > } > > bounds_destroy_bad(bad); > > } > > -- > > 2.31.1 > > >
