[PATCH 0/4 v2] libsepol/cil: Limit certain error and warning reports

[PATCH 0/4 v2] libsepol/cil: Limit certain error and warning reports

[James Carter]

When reporting some errors or warnings, a search is made to find the
original or matching rule. Both neverallow and type bounds violations will
use cil_find_matching_avrule_in_ast() to find the rules in violation. For
context rules, the AST is walked to find the conflicting rule. If there are
a lot of errors or warnings, then this can take a lot of time. oss-fuzz has
generated policies that can abuse this reporting, so the desire is to limit
the reporting by default.

By using the new function, cil_get_log_level(), the error reporting for
neverallow and type bounds violations and the warnings for context rule
conflicts can be less by default while still allowing for everything to
be reported at higher log verbosity levels.


James Carter (4):
  libsepol/cil: Add cil_get_log_level() function
  libsepol/cil: Provide more control over reporting bounds failures
  libsepol/cil: Limit the neverallow violations reported
  libsepol/cil: Limit the amount of reporting for context rule conflicts

 libsepol/cil/src/cil_binary.c | 20 +++++++++---
 libsepol/cil/src/cil_log.c    |  5 +++
 libsepol/cil/src/cil_log.h    |  2 ++
 libsepol/cil/src/cil_post.c   | 57 ++++++++++++++++++++---------------
 4 files changed, 56 insertions(+), 28 deletions(-)

-- 
2.31.1

[PATCH 1/4 v2] libsepol/cil: Add cil_get_log_level() function

[James Carter]

Add the function cil_get_log_level() that returns the current log
level for CIL.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_log.c | 5 +++++
 libsepol/cil/src/cil_log.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/libsepol/cil/src/cil_log.c b/libsepol/cil/src/cil_log.c
index a8e4d2e9..a296929b 100644
--- a/libsepol/cil/src/cil_log.c
+++ b/libsepol/cil/src/cil_log.c
@@ -70,3 +70,8 @@ void cil_set_log_level(enum cil_log_level lvl)
 {
 	cil_log_level = lvl;
 }
+
+enum cil_log_level cil_get_log_level(void)
+{
+	return cil_log_level;
+}
diff --git a/libsepol/cil/src/cil_log.h b/libsepol/cil/src/cil_log.h
index 541569be..442781fb 100644
--- a/libsepol/cil/src/cil_log.h
+++ b/libsepol/cil/src/cil_log.h
@@ -38,4 +38,6 @@
 __attribute__ ((format(printf, 2, 0))) void cil_vlog(enum cil_log_level lvl, const char *msg, va_list args);
 __attribute__ ((format(printf, 2, 3))) void cil_log(enum cil_log_level lvl, const char *msg, ...);
 
+enum cil_log_level cil_get_log_level(void);
+
 #endif // CIL_LOG_H_
-- 
2.31.1

[PATCH 2/4 v2] libsepol/cil: Provide more control over reporting bounds failures

[James Carter]

Commit 4b2e2a248e48b2902ab1ef3cab86322a3c6ef055 (libsepol/cil: Limit
the amount of reporting for bounds failures) limited the number of
bounds failures that were reported to the first two matching rules
for the first two bad rules.

Instead, report the first two matching rules for the first four bad
rules at the default log level and report all matching rules for all
bad rules for higher verbosity levels.

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

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 4ac8ce8d..b7da8241 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -4863,6 +4863,7 @@ static int cil_check_type_bounds(const struct cil_db *db, policydb_t *pdb, void
 			struct cil_avrule target;
 			struct cil_tree_node *n1 = NULL;
 			int count_bad = 0;
+			enum cil_log_level log_level = cil_get_log_level();
 
 			*violation = CIL_TRUE;
 
@@ -4909,16 +4910,16 @@ static int cil_check_type_bounds(const struct cil_db *db, policydb_t *pdb, void
 						__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);
+					if (count_matching >= 2 && num_matching > 2 && log_level == CIL_ERR) {
+						cil_log(CIL_ERR, "    Only first 2 of %d matching rules shown (use \"-v\" to show all)\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);
+				if (count_bad >= 4 && numbad > 4 && log_level == CIL_ERR) {
+					cil_log(CIL_ERR, "  Only first 4 of %d bad rules shown (use \"-v\" to show all)\n", numbad);
 					break;
 				}
 			}
-- 
2.31.1

[PATCH 3/4 v2] libsepol/cil: Limit the neverallow violations reported

[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
four rules (with their parents) that match the violated neverallow
rule along with the total number of rules that violate the
neverallow at the default log level. Report all the violations when
at a higher verbosity level.

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

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index b7da8241..8b64b37a 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -4640,6 +4640,9 @@ 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;
+	enum cil_log_level log_level = cil_get_log_level();
 
 	target.rule_kind = CIL_AVRULE_ALLOWED;
 	target.is_extended = cil_rule->is_extended;
@@ -4666,11 +4669,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 >= 4 && num_matching > 4 && log_level == CIL_ERR) {
+			cil_log(CIL_ERR, "    Only first 4 of %d matching rules shown (use \"-v\" to show all)\n", num_matching);
+			break;
+		}
 	}
 	cil_log(CIL_ERR,"\n");
 	cil_list_destroy(&matching, CIL_FALSE);
-- 
2.31.1

[PATCH 4/4 v2] 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.

First, since these are warnings and not reported at the default log
verbosity level (which only reports errors), only search for the
locations of the conflicting rules when the verbosity level means
that the warnings will actually be reported.

Second, Report all the duplicate conflicting rules together.

Third, Report the first four conflicts of the same rule if when
the verbosity level is at CIL_WARN ("-v") and report all of them
when the verbosity level is at CIL_INFO or higher ("-v -v").

Fixes problem found by oss-fuzz (#39735)

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

diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
index 7e2c2b9a..09c02af9 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -2280,8 +2280,10 @@ 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;
+	enum cil_log_level log_level = cil_get_log_level();
 
 	if (count < 2) {
 		return SEPOL_OK;
@@ -2289,36 +2291,43 @@ 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 >= 4) {
+				/* 2 rules were written when conflicting == 1 */
+				cil_log(CIL_WARN, "  Only first 4 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;
+			if (!db->multiple_decls || concompar(&sort->array[i], &sort->array[j]) != 0) {
+				conflicting++;
+				if (log_level >= CIL_WARN) {
+					struct cil_list_item li;
+					int rc2;
+					li.flavor = flavor;
+					if (conflicting == 1) {
+						cil_log(CIL_WARN, "Found conflicting %s rules\n", flavor_str);
+						rc = SEPOL_ERR;
+						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 < 4 || log_level > CIL_WARN) {
+						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 0/4 v2] libsepol/cil: Limit certain error and warning reports

[James Carter]

I plan on merging this series next week.
Jim

On Wed, Jan 19, 2022 at 11:35 AM James Carter <jwcart2@gmail.com> wrote:
>
> When reporting some errors or warnings, a search is made to find the
> original or matching rule. Both neverallow and type bounds violations will
> use cil_find_matching_avrule_in_ast() to find the rules in violation. For
> context rules, the AST is walked to find the conflicting rule. If there are
> a lot of errors or warnings, then this can take a lot of time. oss-fuzz has
> generated policies that can abuse this reporting, so the desire is to limit
> the reporting by default.
>
> By using the new function, cil_get_log_level(), the error reporting for
> neverallow and type bounds violations and the warnings for context rule
> conflicts can be less by default while still allowing for everything to
> be reported at higher log verbosity levels.
>
>
> James Carter (4):
>   libsepol/cil: Add cil_get_log_level() function
>   libsepol/cil: Provide more control over reporting bounds failures
>   libsepol/cil: Limit the neverallow violations reported
>   libsepol/cil: Limit the amount of reporting for context rule conflicts
>
>  libsepol/cil/src/cil_binary.c | 20 +++++++++---
>  libsepol/cil/src/cil_log.c    |  5 +++
>  libsepol/cil/src/cil_log.h    |  2 ++
>  libsepol/cil/src/cil_post.c   | 57 ++++++++++++++++++++---------------
>  4 files changed, 56 insertions(+), 28 deletions(-)
>
> --
> 2.31.1
>

Re: [PATCH 0/4 v2] libsepol/cil: Limit certain error and warning reports

[James Carter]

On Fri, Feb 18, 2022 at 4:17 PM James Carter <jwcart2@gmail.com> wrote:
>
> I plan on merging this series next week.

This series has been merged.
Jim

> Jim
>
> On Wed, Jan 19, 2022 at 11:35 AM James Carter <jwcart2@gmail.com> wrote:
> >
> > When reporting some errors or warnings, a search is made to find the
> > original or matching rule. Both neverallow and type bounds violations will
> > use cil_find_matching_avrule_in_ast() to find the rules in violation. For
> > context rules, the AST is walked to find the conflicting rule. If there are
> > a lot of errors or warnings, then this can take a lot of time. oss-fuzz has
> > generated policies that can abuse this reporting, so the desire is to limit
> > the reporting by default.
> >
> > By using the new function, cil_get_log_level(), the error reporting for
> > neverallow and type bounds violations and the warnings for context rule
> > conflicts can be less by default while still allowing for everything to
> > be reported at higher log verbosity levels.
> >
> >
> > James Carter (4):
> >   libsepol/cil: Add cil_get_log_level() function
> >   libsepol/cil: Provide more control over reporting bounds failures
> >   libsepol/cil: Limit the neverallow violations reported
> >   libsepol/cil: Limit the amount of reporting for context rule conflicts
> >
> >  libsepol/cil/src/cil_binary.c | 20 +++++++++---
> >  libsepol/cil/src/cil_log.c    |  5 +++
> >  libsepol/cil/src/cil_log.h    |  2 ++
> >  libsepol/cil/src/cil_post.c   | 57 ++++++++++++++++++++---------------
> >  4 files changed, 56 insertions(+), 28 deletions(-)
> >
> > --
> > 2.31.1
> >