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. 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 >
