Nicolas Iooss <nicolas.iooss@xxxxxxx> writes: > 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> It's merged now. Thanks! > 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 >>
