[PATCH 1/2] libsepol/cil: Limit the amount of reporting for neverallow violations

[PATCH 1/2] libsepol/cil: Limit the amount of reporting for neverallow violations

[James Carter]

When there is a neverallow violation, a search is made for all of
the rules that violate the neverallow. The violating rules as well
as their parents are written out to make it easier to find these
rules.

If there is a lot of rules that violate a neverallow, then this
amount of reporting is too much. Instead, only print out the first
two rules (with their parents) that match the violated neverallow
rule along with the total number of rules that violate the
neverallow.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_binary.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 4ac8ce8d..04a5d053 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -4640,6 +4640,8 @@ static int __cil_print_neverallow_failure(const struct cil_db *db, struct cil_tr
 	char *neverallow_str;
 	char *allow_str;
 	enum cil_flavor avrule_flavor;
+	int num_matching = 0;
+	int count_matching = 0;
 
 	target.rule_kind = CIL_AVRULE_ALLOWED;
 	target.is_extended = cil_rule->is_extended;
@@ -4666,11 +4668,19 @@ static int __cil_print_neverallow_failure(const struct cil_db *db, struct cil_tr
 		goto exit;
 	}
 
+	cil_list_for_each(i2, matching) {
+		num_matching++;
+	}
 	cil_list_for_each(i2, matching) {
 		n2 = i2->data;
 		r2 = n2->data;
 		__cil_print_parents("    ", n2);
 		__cil_print_rule("      ", allow_str, r2);
+		count_matching++;
+		if (count_matching >= 2) {
+			cil_log(CIL_ERR, "    Only first 2 of %d matching rules shown\n", num_matching);
+			break;
+		}
 	}
 	cil_log(CIL_ERR,"\n");
 	cil_list_destroy(&matching, CIL_FALSE);
-- 
2.31.1

[PATCH 2/2] libsepol/cil: Limit the amount of reporting for context rule conflicts

[James Carter]

When there are conflicting context rules, the location of the
conflicting rules are written out. If there are many duplicates of
the same context rule, there will be many pairs of conflicts written
out. This hides the fact that all of the rules are the same and can
make it hard to see the different conflicts.

Report all the duplicate conflicting rules together and only report
the first 10 conflicts of the same rule.

Fixes problem found by oss-fuzz (#39735)

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_post.c | 46 +++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
index 7e2c2b9a..f0520abe 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -2280,7 +2280,8 @@ static int __cil_post_report_conflict(struct cil_tree_node *node, uint32_t *fini
 static int __cil_post_process_context_rules(struct cil_sort *sort, int (*compar)(const void *, const void *), int (*concompar)(const void *, const void *), struct cil_db *db, enum cil_flavor flavor, const char *flavor_str)
 {
 	uint32_t count = sort->count;
-	uint32_t i, j = 0, removed = 0;
+	uint32_t i = 0, j, removed = 0;
+	int conflicting = 0;
 	int rc = SEPOL_OK;
 
 	if (count < 2) {
@@ -2289,36 +2290,41 @@ static int __cil_post_process_context_rules(struct cil_sort *sort, int (*compar)
 
 	qsort(sort->array, sort->count, sizeof(sort->array), compar);
 
-	for (i=1; i<count; i++) {
+	for (j=1; j<count; j++) {
 		if (compar(&sort->array[i], &sort->array[j]) != 0) {
-			j++;
+			i++;
+			if (conflicting >= 10) {
+				cil_log(CIL_WARN, "  Only first 10 of %d conflicting rules shown\n", conflicting);
+			}
+			conflicting = 0;
 		} else {
 			removed++;
 			if (!db->multiple_decls ||
 			   concompar(&sort->array[i], &sort->array[j]) != 0) {
 				struct cil_list_item li;
 				int rc2;
-				cil_log(CIL_WARN, "Found conflicting %s rules\n",
-					flavor_str);
-				rc = SEPOL_ERR;
-				li.flavor = flavor;
-				li.data = sort->array[i];
-				rc2 = cil_tree_walk(db->ast->root,
-						    __cil_post_report_conflict,
-						    NULL, NULL, &li);
-				if (rc2 != SEPOL_OK) goto exit;
-				li.data = sort->array[j];
-				rc2 = cil_tree_walk(db->ast->root,
-						    __cil_post_report_conflict,
-						    NULL, NULL, &li);
-				if (rc2 != SEPOL_OK) goto exit;
+				conflicting++;
+				if (conflicting == 1) {
+					cil_log(CIL_WARN, "Found conflicting %s rules\n", flavor_str);
+					rc = SEPOL_ERR;
+					li.flavor = flavor;
+					li.data = sort->array[i];
+					rc2 = cil_tree_walk(db->ast->root, __cil_post_report_conflict,
+										NULL, NULL, &li);
+					if (rc2 != SEPOL_OK) goto exit;
+				}
+				if (conflicting < 10) {
+					li.data = sort->array[j];
+					rc2 = cil_tree_walk(db->ast->root, __cil_post_report_conflict,
+										NULL, NULL, &li);
+					if (rc2 != SEPOL_OK) goto exit;
+				}
 			}
 		}
-		if (i != j) {
-			sort->array[j] = sort->array[i];
+		if (i != j && !conflicting) {
+			sort->array[i] = sort->array[j];
 		}
 	}
-
 	sort->count = count - removed;
 
 exit:
-- 
2.31.1

Re: [PATCH 1/2] libsepol/cil: Limit the amount of reporting for neverallow violations

[bauen1]

Hi,

as a heavy user of neverallow / neverallowx, please don't limit this.

When adding a new neverallow rule there might quite a few types violating them, and having to rebuild the policy every 2 types would make fixing them incredibly annoying.

If you want to limit this, then please make it opt-in or add it as a command line option.

On 1/14/22 20:20, James Carter wrote:
> When there is a neverallow violation, a search is made for all of
> the rules that violate the neverallow. The violating rules as well
> as their parents are written out to make it easier to find these
> rules.
> 
> If there is a lot of rules that violate a neverallow, then this
> amount of reporting is too much. Instead, only print out the first
> two rules (with their parents) that match the violated neverallow
> rule along with the total number of rules that violate the
> neverallow.
> 
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>   libsepol/cil/src/cil_binary.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 4ac8ce8d..04a5d053 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -4640,6 +4640,8 @@ static int __cil_print_neverallow_failure(const struct cil_db *db, struct cil_tr
>   	char *neverallow_str;
>   	char *allow_str;
>   	enum cil_flavor avrule_flavor;
> +	int num_matching = 0;
> +	int count_matching = 0;
>   
>   	target.rule_kind = CIL_AVRULE_ALLOWED;
>   	target.is_extended = cil_rule->is_extended;
> @@ -4666,11 +4668,19 @@ static int __cil_print_neverallow_failure(const struct cil_db *db, struct cil_tr
>   		goto exit;
>   	}
>   
> +	cil_list_for_each(i2, matching) {
> +		num_matching++;
> +	}
>   	cil_list_for_each(i2, matching) {
>   		n2 = i2->data;
>   		r2 = n2->data;
>   		__cil_print_parents("    ", n2);
>   		__cil_print_rule("      ", allow_str, r2);
> +		count_matching++;
> +		if (count_matching >= 2) {
> +			cil_log(CIL_ERR, "    Only first 2 of %d matching rules shown\n", num_matching);
> +			break;
> +		}
>   	}
>   	cil_log(CIL_ERR,"\n");
>   	cil_list_destroy(&matching, CIL_FALSE);

-- 
bauen1
https://dn42.bauen1.xyz/

Re: [PATCH 1/2] libsepol/cil: Limit the amount of reporting for neverallow violations

[James Carter]

On Fri, Jan 14, 2022 at 2:44 PM bauen1 <j2468h@googlemail.com> wrote:
>
> Hi,
>
> as a heavy user of neverallow / neverallowx, please don't limit this.
>
> When adding a new neverallow rule there might quite a few types violating them, and having to rebuild the policy every 2 types would make fixing them incredibly annoying.
>
> If you want to limit this, then please make it opt-in or add it as a command line option.
>

I am trying to limit error messages because oss-fuzz seems to be good
at creating policies that generate a lot of error messages and
subsequently take a lot of time to process.

But I am not going to do that at the expense of people actually using secilc.

I was already thinking about making the amount of error reporting
depending on the verbosity level. What would think of it limiting it
to two by default, but unlimited at any higher verbosity level. I can
even add a message to use "-v" to see all of the errors.

Jim


> On 1/14/22 20:20, James Carter wrote:
> > When there is a neverallow violation, a search is made for all of
> > the rules that violate the neverallow. The violating rules as well
> > as their parents are written out to make it easier to find these
> > rules.
> >
> > If there is a lot of rules that violate a neverallow, then this
> > amount of reporting is too much. Instead, only print out the first
> > two rules (with their parents) that match the violated neverallow
> > rule along with the total number of rules that violate the
> > neverallow.
> >
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> >   libsepol/cil/src/cil_binary.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> > index 4ac8ce8d..04a5d053 100644
> > --- a/libsepol/cil/src/cil_binary.c
> > +++ b/libsepol/cil/src/cil_binary.c
> > @@ -4640,6 +4640,8 @@ static int __cil_print_neverallow_failure(const struct cil_db *db, struct cil_tr
> >       char *neverallow_str;
> >       char *allow_str;
> >       enum cil_flavor avrule_flavor;
> > +     int num_matching = 0;
> > +     int count_matching = 0;
> >
> >       target.rule_kind = CIL_AVRULE_ALLOWED;
> >       target.is_extended = cil_rule->is_extended;
> > @@ -4666,11 +4668,19 @@ static int __cil_print_neverallow_failure(const struct cil_db *db, struct cil_tr
> >               goto exit;
> >       }
> >
> > +     cil_list_for_each(i2, matching) {
> > +             num_matching++;
> > +     }
> >       cil_list_for_each(i2, matching) {
> >               n2 = i2->data;
> >               r2 = n2->data;
> >               __cil_print_parents("    ", n2);
> >               __cil_print_rule("      ", allow_str, r2);
> > +             count_matching++;
> > +             if (count_matching >= 2) {
> > +                     cil_log(CIL_ERR, "    Only first 2 of %d matching rules shown\n", num_matching);
> > +                     break;
> > +             }
> >       }
> >       cil_log(CIL_ERR,"\n");
> >       cil_list_destroy(&matching, CIL_FALSE);
>
> --
> bauen1
> https://dn42.bauen1.xyz/

Re: [PATCH 1/2] libsepol/cil: Limit the amount of reporting for neverallow violations

[bauen1]

On 1/18/22 16:48, James Carter wrote:
> On Fri, Jan 14, 2022 at 2:44 PM bauen1 <j2468h@googlemail.com> wrote:
>>
>> Hi,
>>
>> as a heavy user of neverallow / neverallowx, please don't limit this.
>>
>> When adding a new neverallow rule there might quite a few types violating them, and having to rebuild the policy every 2 types would make fixing them incredibly annoying.
>>
>> If you want to limit this, then please make it opt-in or add it as a command line option.
>>
> 
> I am trying to limit error messages because oss-fuzz seems to be good
> at creating policies that generate a lot of error messages and
> subsequently take a lot of time to process.
> 
> But I am not going to do that at the expense of people actually using secilc.
> 
> I was already thinking about making the amount of error reporting
> depending on the verbosity level. What would think of it limiting it
> to two by default, but unlimited at any higher verbosity level. I can
> even add a message to use "-v" to see all of the errors.

Thanks, something like that would be totally fine for me.

> Jim
> 
> 
>> On 1/14/22 20:20, James Carter wrote:
>>> When there is a neverallow violation, a search is made for all of
>>> the rules that violate the neverallow. The violating rules as well
>>> as their parents are written out to make it easier to find these
>>> rules.
>>>
>>> If there is a lot of rules that violate a neverallow, then this
>>> amount of reporting is too much. Instead, only print out the first
>>> two rules (with their parents) that match the violated neverallow
>>> rule along with the total number of rules that violate the
>>> neverallow.
>>>
>>> Signed-off-by: James Carter <jwcart2@gmail.com>
>>> ---
>>>    libsepol/cil/src/cil_binary.c | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
>>> index 4ac8ce8d..04a5d053 100644
>>> --- a/libsepol/cil/src/cil_binary.c
>>> +++ b/libsepol/cil/src/cil_binary.c
>>> @@ -4640,6 +4640,8 @@ static int __cil_print_neverallow_failure(const struct cil_db *db, struct cil_tr
>>>        char *neverallow_str;
>>>        char *allow_str;
>>>        enum cil_flavor avrule_flavor;
>>> +     int num_matching = 0;
>>> +     int count_matching = 0;
>>>
>>>        target.rule_kind = CIL_AVRULE_ALLOWED;
>>>        target.is_extended = cil_rule->is_extended;
>>> @@ -4666,11 +4668,19 @@ static int __cil_print_neverallow_failure(const struct cil_db *db, struct cil_tr
>>>                goto exit;
>>>        }
>>>
>>> +     cil_list_for_each(i2, matching) {
>>> +             num_matching++;
>>> +     }
>>>        cil_list_for_each(i2, matching) {
>>>                n2 = i2->data;
>>>                r2 = n2->data;
>>>                __cil_print_parents("    ", n2);
>>>                __cil_print_rule("      ", allow_str, r2);
>>> +             count_matching++;
>>> +             if (count_matching >= 2) {
>>> +                     cil_log(CIL_ERR, "    Only first 2 of %d matching rules shown\n", num_matching);
>>> +                     break;
>>> +             }
>>>        }
>>>        cil_log(CIL_ERR,"\n");
>>>        cil_list_destroy(&matching, CIL_FALSE);
>>
>> --
>> bauen1
>> https://dn42.bauen1.xyz/

-- 
bauen1
https://dn42.bauen1.xyz/

Re: [PATCH 1/2] libsepol/cil: Limit the amount of reporting for neverallow violations

[bauen1]

Hi,

On 1/19/22 14:04, bauen1 wrote:
> 
> On 1/18/22 16:48, James Carter wrote:
>> On Fri, Jan 14, 2022 at 2:44 PM bauen1 <j2468h@googlemail.com> wrote:
>>>
>>> Hi,
>>>
>>> as a heavy user of neverallow / neverallowx, please don't limit this.
>>>
>>> When adding a new neverallow rule there might quite a few types violating them, and having to rebuild the policy every 2 types would make fixing them incredibly annoying.
>>>
>>> If you want to limit this, then please make it opt-in or add it as a command line option.
>>>
>>
>> I am trying to limit error messages because oss-fuzz seems to be good
>> at creating policies that generate a lot of error messages and
>> subsequently take a lot of time to process.
>>
>> But I am not going to do that at the expense of people actually using secilc.
>>
>> I was already thinking about making the amount of error reporting
>> depending on the verbosity level. What would think of it limiting it
>> to two by default, but unlimited at any higher verbosity level. I can
>> even add a message to use "-v" to see all of the errors.
> 
> Thanks, something like that would be totally fine for me.
> 

I've also just noticed that typebounds will only print the first 2 violations.
So if you make this depend on the verbosity level you might want to change that too, just to be consistent.

-- 
bauen1
https://dn42.bauen1.xyz/

Re: [PATCH 1/2] libsepol/cil: Limit the amount of reporting for neverallow violations

[James Carter]

On Fri, Feb 11, 2022 at 8:03 PM bauen1 <j2468h@googlemail.com> wrote:
>
> Hi,
>
> On 1/19/22 14:04, bauen1 wrote:
> >
> > On 1/18/22 16:48, James Carter wrote:
> >> On Fri, Jan 14, 2022 at 2:44 PM bauen1 <j2468h@googlemail.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> as a heavy user of neverallow / neverallowx, please don't limit this.
> >>>
> >>> When adding a new neverallow rule there might quite a few types violating them, and having to rebuild the policy every 2 types would make fixing them incredibly annoying.
> >>>
> >>> If you want to limit this, then please make it opt-in or add it as a command line option.
> >>>
> >>
> >> I am trying to limit error messages because oss-fuzz seems to be good
> >> at creating policies that generate a lot of error messages and
> >> subsequently take a lot of time to process.
> >>
> >> But I am not going to do that at the expense of people actually using secilc.
> >>
> >> I was already thinking about making the amount of error reporting
> >> depending on the verbosity level. What would think of it limiting it
> >> to two by default, but unlimited at any higher verbosity level. I can
> >> even add a message to use "-v" to see all of the errors.
> >
> > Thanks, something like that would be totally fine for me.
> >
>
> I've also just noticed that typebounds will only print the first 2 violations.
> So if you make this depend on the verbosity level you might want to change that too, just to be consistent.
>

Yes, I did send out a v2 that changes the typebounds error reporting
to depend on the verbosity level as well. That patch set was sent out
on January 19th.

Thanks,
Jim


> --
> bauen1
> https://dn42.bauen1.xyz/