* [PATCH] libsepol/cil: Limit the amount of reporting for bounds failures
@ 2021-09-28 17:55 James Carter
2021-10-04 22:13 ` Nicolas Iooss
0 siblings, 1 reply; 4+ messages in thread
From: James Carter @ 2021-09-28 17:55 UTC (permalink / raw)
To: selinux; +Cc: nicolas.iooss, James Carter
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@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] libsepol/cil: Limit the amount of reporting for bounds failures
2021-09-28 17:55 [PATCH] libsepol/cil: Limit the amount of reporting for bounds failures James Carter
@ 2021-10-04 22:13 ` Nicolas Iooss
2021-10-05 21:01 ` James Carter
2021-10-06 10:28 ` Petr Lautrbach
0 siblings, 2 replies; 4+ messages in thread
From: Nicolas Iooss @ 2021-10-04 22:13 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list
On Tue, Sep 28, 2021 at 7:55 PM James Carter <jwcart2@gmail.com> 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@gmail.com>
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@m4x.org>
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libsepol/cil: Limit the amount of reporting for bounds failures
2021-10-04 22:13 ` Nicolas Iooss
@ 2021-10-05 21:01 ` James Carter
2021-10-06 10:28 ` Petr Lautrbach
1 sibling, 0 replies; 4+ messages in thread
From: James Carter @ 2021-10-05 21:01 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
On Mon, Oct 4, 2021 at 6:13 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Tue, Sep 28, 2021 at 7:55 PM James Carter <jwcart2@gmail.com> 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@gmail.com>
>
> 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@m4x.org>
>
> 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
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libsepol/cil: Limit the amount of reporting for bounds failures
2021-10-04 22:13 ` Nicolas Iooss
2021-10-05 21:01 ` James Carter
@ 2021-10-06 10:28 ` Petr Lautrbach
1 sibling, 0 replies; 4+ messages in thread
From: Petr Lautrbach @ 2021-10-06 10:28 UTC (permalink / raw)
To: SElinux list, Nicolas Iooss, James Carter
Nicolas Iooss <nicolas.iooss@m4x.org> writes:
> On Tue, Sep 28, 2021 at 7:55 PM James Carter <jwcart2@gmail.com> 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@gmail.com>
>
> 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@m4x.org>
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
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-06 10:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-28 17:55 [PATCH] libsepol/cil: Limit the amount of reporting for bounds failures James Carter
2021-10-04 22:13 ` Nicolas Iooss
2021-10-05 21:01 ` James Carter
2021-10-06 10:28 ` Petr Lautrbach
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.