[PATCH 00/16 v2] Refactor and fix assertion checking

[PATCH 00/16 v2] Refactor and fix assertion checking

[James Carter]

The first 13 patches refactor and cleanup the neverallow and
neverallowxperm checking code to make it easier to understand.

The last 3 patches fixes errors in the assertion checking code.

This series is to prepare for adding not-self support to assertion
checking.

The only change for version 2 is in patch 7 where target_type should
have been used instead of source_type.

James Carter (16):
  libsepol: Return an error if check_assertion() returns an error.
  libsepol: Change label in check_assertion_avtab_match()
  libsepol: Remove uneeded error messages in assertion checking
  libsepol: Check for error from check_assertion_extended_permissions()
  libsepol: Use consistent return checking style
  libsepol: Move check of target types to before check for self
  libsepol: Create function check_assertion_self_match() and use it
  libsepol: Use (rc < 0) instead of (rc) when calling ebitmap functions
  libsepol: Remove unnessesary check for matching class
  libsepol: Move assigning outer loop index out of inner loop
  libsepol: Make use of previously created ebitmap when checking self
  libsepol: Refactor match_any_class_permissions() to be clearer
  libsepol: Make return value clearer when reporting neverallowx errors
  libsepol: The src and tgt must be the same if neverallow uses self
  libsepol: Set args avtab pointer when reporting assertion violations
  libsepol: Fix two problems with neverallowxperm reporting

 libsepol/src/assertion.c | 193 +++++++++++++++++++++------------------
 1 file changed, 102 insertions(+), 91 deletions(-)

-- 
2.31.1

[PATCH 01/16 v2] libsepol: Return an error if check_assertion() returns an error.

[James Carter]

Instead of calling report_assertion_failures() and treating an
error like it was a neverallow violation, just return an error.

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

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index dd2749a0..ba4a204f 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -538,6 +538,10 @@ int check_assertions(sepol_handle_t * handle, policydb_t * p,
 		if (!(a->specified & (AVRULE_NEVERALLOW | AVRULE_XPERMS_NEVERALLOW)))
 			continue;
 		rc = check_assertion(p, a);
+		if (rc < 0) {
+			ERR(handle, "Error occurred while checking neverallows");
+			return -1;
+		}
 		if (rc) {
 			rc = report_assertion_failures(handle, p, a);
 			if (rc < 0) {
-- 
2.31.1

[PATCH 02/16 v2] libsepol: Change label in check_assertion_avtab_match()

[James Carter]

Change the label name from "exit" to "nomatch' to make it clearer
what is happening.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/src/assertion.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index ba4a204f..d716450f 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -454,14 +454,14 @@ static int check_assertion_avtab_match(avtab_key_t *k, avtab_datum_t *d, void *a
 	avtab_t *avtab = a->avtab;
 
 	if ((k->specified & AVTAB_ALLOWED) == 0)
-		goto exit;
+		goto nomatch;
 
 	if (!match_any_class_permissions(avrule->perms, k->target_class, d->data))
-		goto exit;
+		goto nomatch;
 
 	rc = ebitmap_match_any(&avrule->stypes.types, &p->attr_type_map[k->source_type - 1]);
 	if (rc == 0)
-		goto exit;
+		goto nomatch;
 
 	if (avrule->flags == RULE_SELF) {
 		/* If the neverallow uses SELF, then it is not enough that the
@@ -482,16 +482,16 @@ static int check_assertion_avtab_match(avtab_key_t *k, avtab_datum_t *d, void *a
 	/* neverallow may have tgts even if it uses SELF */
 	rc = ebitmap_match_any(&avrule->ttypes.types, &p->attr_type_map[k->target_type -1]);
 	if (rc == 0 && rc2 == 0)
-		goto exit;
+		goto nomatch;
 
 	if (avrule->specified == AVRULE_XPERMS_NEVERALLOW) {
 		rc = check_assertion_extended_permissions(avrule, avtab, k, p);
 		if (rc == 0)
-			goto exit;
+			goto nomatch;
 	}
 	return 1;
 
-exit:
+nomatch:
 	return 0;
 
 oom:
-- 
2.31.1

[PATCH 03/16 v2] libsepol: Remove uneeded error messages in assertion checking

[James Carter]

An out of memory condition is unlikely and the general message
that an error occured while checking neverallows is sufficient.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/src/assertion.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index d716450f..832d3749 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -278,11 +278,8 @@ static int report_assertion_avtab_matches(avtab_key_t *k, avtab_datum_t *d, void
 			}
 		}
 	}
-	goto exit;
 
 oom:
-	ERR(NULL, "Out of memory - unable to check neverallows");
-
 exit:
 	ebitmap_destroy(&src_matches);
 	ebitmap_destroy(&tgt_matches);
@@ -436,8 +433,6 @@ static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab
 	goto exit;
 
 oom:
-	ERR(NULL, "Out of memory - unable to check neverallows");
-
 exit:
 	ebitmap_destroy(&src_matches);
 	ebitmap_destroy(&tgt_matches);
@@ -495,7 +490,6 @@ nomatch:
 	return 0;
 
 oom:
-	ERR(NULL, "Out of memory - unable to check neverallows");
 	return rc;
 }
 
-- 
2.31.1

[PATCH 04/16 v2] libsepol: Check for error from check_assertion_extended_permissions()

[James Carter]

Return an error if check_assertion_extended_permissions() returns
an error instead of treating it as an assertion violation.

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

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 832d3749..a2cbb74d 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -481,6 +481,8 @@ static int check_assertion_avtab_match(avtab_key_t *k, avtab_datum_t *d, void *a
 
 	if (avrule->specified == AVRULE_XPERMS_NEVERALLOW) {
 		rc = check_assertion_extended_permissions(avrule, avtab, k, p);
+		if (rc < 0)
+			goto oom;
 		if (rc == 0)
 			goto nomatch;
 	}
-- 
2.31.1

[PATCH 05/16 v2] libsepol: Use consistent return checking style

[James Carter]

In check_assertion_avtab_match(), for the functions that do not return
an error, but only returns 0 or 1 depending on if a match is found,
call the function in an if statement.

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

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index a2cbb74d..bd0dc4ed 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -454,8 +454,7 @@ static int check_assertion_avtab_match(avtab_key_t *k, avtab_datum_t *d, void *a
 	if (!match_any_class_permissions(avrule->perms, k->target_class, d->data))
 		goto nomatch;
 
-	rc = ebitmap_match_any(&avrule->stypes.types, &p->attr_type_map[k->source_type - 1]);
-	if (rc == 0)
+	if (!ebitmap_match_any(&avrule->stypes.types, &p->attr_type_map[k->source_type - 1]))
 		goto nomatch;
 
 	if (avrule->flags == RULE_SELF) {
@@ -475,9 +474,10 @@ static int check_assertion_avtab_match(avtab_key_t *k, avtab_datum_t *d, void *a
 	}
 
 	/* neverallow may have tgts even if it uses SELF */
-	rc = ebitmap_match_any(&avrule->ttypes.types, &p->attr_type_map[k->target_type -1]);
-	if (rc == 0 && rc2 == 0)
-		goto nomatch;
+	if (!ebitmap_match_any(&avrule->ttypes.types, &p->attr_type_map[k->target_type -1])) {
+		if (rc2 == 0)
+			goto nomatch;
+	}
 
 	if (avrule->specified == AVRULE_XPERMS_NEVERALLOW) {
 		rc = check_assertion_extended_permissions(avrule, avtab, k, p);
-- 
2.31.1

[PATCH 06/16 v2] libsepol: Move check of target types to before check for self

[James Carter]

If a neverallow has target types as well as using self and a match
is found with the target types, then self does not even need to
be checked, since the rule is already in violation of the assertion.

So move the check for a match of the target types before dealing with
self.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/src/assertion.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index bd0dc4ed..7a1c4a5e 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -457,26 +457,28 @@ static int check_assertion_avtab_match(avtab_key_t *k, avtab_datum_t *d, void *a
 	if (!ebitmap_match_any(&avrule->stypes.types, &p->attr_type_map[k->source_type - 1]))
 		goto nomatch;
 
-	if (avrule->flags == RULE_SELF) {
-		/* If the neverallow uses SELF, then it is not enough that the
-		 * neverallow's source matches the src and tgt of the rule being checked.
-		 * It must match the same thing in the src and tgt, so AND the source
-		 * and target together and check for a match on the result.
-		 */
-		ebitmap_t match;
-		rc = ebitmap_and(&match, &p->attr_type_map[k->source_type - 1], &p->attr_type_map[k->target_type - 1] );
-		if (rc) {
-			ebitmap_destroy(&match);
-			goto oom;
-		}
-		rc2 = ebitmap_match_any(&avrule->stypes.types, &match);
-		ebitmap_destroy(&match);
-	}
-
 	/* neverallow may have tgts even if it uses SELF */
 	if (!ebitmap_match_any(&avrule->ttypes.types, &p->attr_type_map[k->target_type -1])) {
-		if (rc2 == 0)
+		if (avrule->flags == RULE_SELF) {
+			/* If the neverallow uses SELF, then it is not enough that the
+			 * neverallow's source matches the src and tgt of the rule being checked.
+			 * It must match the same thing in the src and tgt, so AND the source
+			 * and target together and check for a match on the result.
+			 */
+			ebitmap_t match;
+			rc = ebitmap_and(&match, &p->attr_type_map[k->source_type - 1], &p->attr_type_map[k->target_type - 1] );
+			if (rc) {
+				ebitmap_destroy(&match);
+				goto oom;
+			}
+			if (!ebitmap_match_any(&avrule->stypes.types, &match)) {
+				ebitmap_destroy(&match);
+				goto nomatch;
+			}
+			ebitmap_destroy(&match);
+		} else {
 			goto nomatch;
+		}
 	}
 
 	if (avrule->specified == AVRULE_XPERMS_NEVERALLOW) {
-- 
2.31.1

[PATCH 07/16 v2] libsepol: Create function check_assertion_self_match() and use it

[James Carter]

Create the function called check_assertion_self_match() and put the
self checking code into it.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
v2: Use k->target_type instead of k->source_type in call to ebitmap_match_any()

 libsepol/src/assertion.c | 45 ++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 7a1c4a5e..6881c5f6 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -440,9 +440,35 @@ exit:
 	return ret;
 }
 
+static int check_assertion_self_match(avtab_key_t *k, avrule_t *avrule, policydb_t *p)
+{
+	ebitmap_t src_matches;
+	int rc;
+
+	/* The key's target must match something in the matches of the avrule's source
+	 * and the key's source.
+	 */
+
+	rc = ebitmap_and(&src_matches, &avrule->stypes.types, &p->attr_type_map[k->source_type - 1]);
+	if (rc < 0)
+		goto oom;
+
+	if (!ebitmap_match_any(&src_matches, &p->attr_type_map[k->target_type - 1])) {
+		rc = 0;
+		goto nomatch;
+	}
+
+	rc = 1;
+
+oom:
+nomatch:
+	ebitmap_destroy(&src_matches);
+	return rc;
+}
+
 static int check_assertion_avtab_match(avtab_key_t *k, avtab_datum_t *d, void *args)
 {
-	int rc, rc2 = 0;
+	int rc;
 	struct avtab_match_args *a = (struct avtab_match_args *)args;
 	policydb_t *p = a->p;
 	avrule_t *avrule = a->avrule;
@@ -460,22 +486,11 @@ static int check_assertion_avtab_match(avtab_key_t *k, avtab_datum_t *d, void *a
 	/* neverallow may have tgts even if it uses SELF */
 	if (!ebitmap_match_any(&avrule->ttypes.types, &p->attr_type_map[k->target_type -1])) {
 		if (avrule->flags == RULE_SELF) {
-			/* If the neverallow uses SELF, then it is not enough that the
-			 * neverallow's source matches the src and tgt of the rule being checked.
-			 * It must match the same thing in the src and tgt, so AND the source
-			 * and target together and check for a match on the result.
-			 */
-			ebitmap_t match;
-			rc = ebitmap_and(&match, &p->attr_type_map[k->source_type - 1], &p->attr_type_map[k->target_type - 1] );
-			if (rc) {
-				ebitmap_destroy(&match);
+			rc = check_assertion_self_match(k, avrule, p);
+			if (rc < 0)
 				goto oom;
-			}
-			if (!ebitmap_match_any(&avrule->stypes.types, &match)) {
-				ebitmap_destroy(&match);
+			if (rc == 0)
 				goto nomatch;
-			}
-			ebitmap_destroy(&match);
 		} else {
 			goto nomatch;
 		}
-- 
2.31.1

[PATCH 08/16 v2] libsepol: Use (rc < 0) instead of (rc) when calling ebitmap functions

[James Carter]

Inorder to differentiate errors from matches, use "(rc < 0)" when
calling ebitmap_* functions while checking neverallow rules.

Also, just use rc instead of having a separate variable (ret) in
check_assertion_extended_permissions().

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

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 6881c5f6..b48169ef 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -231,27 +231,27 @@ static int report_assertion_avtab_matches(avtab_key_t *k, avtab_datum_t *d, void
 
 	rc = ebitmap_and(&src_matches, &avrule->stypes.types,
 			 &p->attr_type_map[k->source_type - 1]);
-	if (rc)
+	if (rc < 0)
 		goto oom;
 
 	if (ebitmap_is_empty(&src_matches))
 		goto exit;
 
 	rc = ebitmap_and(&tgt_matches, &avrule->ttypes.types, &p->attr_type_map[k->target_type -1]);
-	if (rc)
+	if (rc < 0)
 		goto oom;
 
 	if (avrule->flags == RULE_SELF) {
 		rc = ebitmap_and(&matches, &p->attr_type_map[k->source_type - 1], &p->attr_type_map[k->target_type - 1]);
-		if (rc)
+		if (rc < 0)
 			goto oom;
 		rc = ebitmap_and(&self_matches, &avrule->stypes.types, &matches);
-		if (rc)
+		if (rc < 0)
 			goto oom;
 
 		if (!ebitmap_is_empty(&self_matches)) {
 			rc = ebitmap_union(&tgt_matches, &self_matches);
-			if (rc)
+			if (rc < 0)
 				goto oom;
 		}
 	}
@@ -299,11 +299,11 @@ static int report_assertion_failures(sepol_handle_t *handle, policydb_t *p, avru
 	args.errors = 0;
 
 	rc = avtab_map(&p->te_avtab, report_assertion_avtab_matches, &args);
-	if (rc)
+	if (rc < 0)
 		goto oom;
 
 	rc = avtab_map(&p->te_cond_avtab, report_assertion_avtab_matches, &args);
-	if (rc)
+	if (rc < 0)
 		goto oom;
 
 	return args.errors;
@@ -379,7 +379,6 @@ static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab
 	ebitmap_node_t *snode, *tnode;
 	class_perm_node_t *cp;
 	int rc;
-	int ret = 1;
 
 	ebitmap_init(&src_matches);
 	ebitmap_init(&tgt_matches);
@@ -388,56 +387,61 @@ static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab
 
 	rc = ebitmap_and(&src_matches, &avrule->stypes.types,
 			 &p->attr_type_map[k->source_type - 1]);
-	if (rc)
+	if (rc < 0)
 		goto oom;
 
-	if (ebitmap_is_empty(&src_matches))
+	if (ebitmap_is_empty(&src_matches)) {
+		rc = 0;
 		goto exit;
+	}
 
 	rc = ebitmap_and(&tgt_matches, &avrule->ttypes.types,
 			 &p->attr_type_map[k->target_type -1]);
-	if (rc)
+	if (rc < 0)
 		goto oom;
 
 	if (avrule->flags == RULE_SELF) {
 		rc = ebitmap_and(&matches, &p->attr_type_map[k->source_type - 1],
 				&p->attr_type_map[k->target_type - 1]);
-		if (rc)
+		if (rc < 0)
 			goto oom;
 		rc = ebitmap_and(&self_matches, &avrule->stypes.types, &matches);
-		if (rc)
+		if (rc < 0)
 			goto oom;
 
 		if (!ebitmap_is_empty(&self_matches)) {
 			rc = ebitmap_union(&tgt_matches, &self_matches);
-			if (rc)
+			if (rc < 0)
 				goto oom;
 		}
 	}
 
-	if (ebitmap_is_empty(&tgt_matches))
+	if (ebitmap_is_empty(&tgt_matches)) {
+		rc = 0;
 		goto exit;
+	}
 
 	for (cp = avrule->perms; cp; cp = cp->next) {
 		if (cp->tclass != k->target_class)
 			continue;
 		ebitmap_for_each_positive_bit(&src_matches, snode, i) {
 			ebitmap_for_each_positive_bit(&tgt_matches, tnode, j) {
-				ret = check_assertion_extended_permissions_avtab(
-						avrule, avtab, i, j, k, p);
-				if (ret)
+				if (check_assertion_extended_permissions_avtab(avrule, avtab, i, j, k, p)) {
+					rc = 1;
 					goto exit;
+				}
 			}
 		}
 	}
-	goto exit;
+
+	rc = 0;
 
 oom:
 exit:
 	ebitmap_destroy(&src_matches);
 	ebitmap_destroy(&tgt_matches);
 	ebitmap_destroy(&matches);
-	return ret;
+	return rc;
 }
 
 static int check_assertion_self_match(avtab_key_t *k, avrule_t *avrule, policydb_t *p)
-- 
2.31.1

[PATCH 09/16 v2] libsepol: Remove unnessesary check for matching class

[James Carter]

When check_assertion_extended_permissions() is called, it has already
been determined that there is a match, and, since neither the class
nor the permissions are used, there is no need for the check.

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

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index b48169ef..42fa87d9 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -377,7 +377,6 @@ static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab
 	ebitmap_t src_matches, tgt_matches, self_matches, matches;
 	unsigned int i, j;
 	ebitmap_node_t *snode, *tnode;
-	class_perm_node_t *cp;
 	int rc;
 
 	ebitmap_init(&src_matches);
@@ -421,15 +420,11 @@ static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab
 		goto exit;
 	}
 
-	for (cp = avrule->perms; cp; cp = cp->next) {
-		if (cp->tclass != k->target_class)
-			continue;
-		ebitmap_for_each_positive_bit(&src_matches, snode, i) {
-			ebitmap_for_each_positive_bit(&tgt_matches, tnode, j) {
-				if (check_assertion_extended_permissions_avtab(avrule, avtab, i, j, k, p)) {
-					rc = 1;
-					goto exit;
-				}
+	ebitmap_for_each_positive_bit(&src_matches, snode, i) {
+		ebitmap_for_each_positive_bit(&tgt_matches, tnode, j) {
+			if (check_assertion_extended_permissions_avtab(avrule, avtab, i, j, k, p)) {
+				rc = 1;
+				goto exit;
 			}
 		}
 	}
-- 
2.31.1

[PATCH 10/16 v2] libsepol: Move assigning outer loop index out of inner loop

[James Carter]

Assign value based on outer loop index in the outer loop instead
of the inner loop.

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

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 42fa87d9..9c09eef3 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -158,8 +158,8 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
 	tmp_key.specified = AVTAB_XPERMS_ALLOWED;
 
 	ebitmap_for_each_positive_bit(sattr, snode, i) {
+		tmp_key.source_type = i + 1;
 		ebitmap_for_each_positive_bit(tattr, tnode, j) {
-			tmp_key.source_type = i + 1;
 			tmp_key.target_type = j + 1;
 			for (node = avtab_search_node(avtab, &tmp_key);
 			     node;
@@ -334,8 +334,8 @@ static int check_assertion_extended_permissions_avtab(avrule_t *avrule, avtab_t
 	tmp_key.specified = AVTAB_XPERMS_ALLOWED;
 
 	ebitmap_for_each_positive_bit(sattr, snode, i) {
+		tmp_key.source_type = i + 1;
 		ebitmap_for_each_positive_bit(tattr, tnode, j) {
-			tmp_key.source_type = i + 1;
 			tmp_key.target_type = j + 1;
 			for (node = avtab_search_node(avtab, &tmp_key);
 			     node;
-- 
2.31.1

[PATCH 11/16 v2] libsepol: Make use of previously created ebitmap when checking self

[James Carter]

In both check_assertion_extended_permissions() and
report_assertion_avtab_matches(), when checking for a match involving
a rule using self, the matches between the source and target of the
rule being checked are found using ebitmap_and() and then the matches
between that result and the source of the neverallow are found using
another ebitmap_and() call.

Since the matches between the sources of the rule being checked and
the neverallow have already been found, just find the matches between
that result and the target of the rule being checked. This only
requires one call to ebitmap_and() instead of two.

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

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 9c09eef3..71ee7815 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -214,7 +214,7 @@ static int report_assertion_avtab_matches(avtab_key_t *k, avtab_datum_t *d, void
 	avrule_t *avrule = a->avrule;
 	class_perm_node_t *cp;
 	uint32_t perms;
-	ebitmap_t src_matches, tgt_matches, self_matches, matches;
+	ebitmap_t src_matches, tgt_matches, self_matches;
 	ebitmap_node_t *snode, *tnode;
 	unsigned int i, j;
 
@@ -227,7 +227,6 @@ static int report_assertion_avtab_matches(avtab_key_t *k, avtab_datum_t *d, void
 	ebitmap_init(&src_matches);
 	ebitmap_init(&tgt_matches);
 	ebitmap_init(&self_matches);
-	ebitmap_init(&matches);
 
 	rc = ebitmap_and(&src_matches, &avrule->stypes.types,
 			 &p->attr_type_map[k->source_type - 1]);
@@ -242,10 +241,7 @@ static int report_assertion_avtab_matches(avtab_key_t *k, avtab_datum_t *d, void
 		goto oom;
 
 	if (avrule->flags == RULE_SELF) {
-		rc = ebitmap_and(&matches, &p->attr_type_map[k->source_type - 1], &p->attr_type_map[k->target_type - 1]);
-		if (rc < 0)
-			goto oom;
-		rc = ebitmap_and(&self_matches, &avrule->stypes.types, &matches);
+		rc = ebitmap_and(&self_matches, &src_matches, &p->attr_type_map[k->target_type - 1]);
 		if (rc < 0)
 			goto oom;
 
@@ -284,7 +280,6 @@ exit:
 	ebitmap_destroy(&src_matches);
 	ebitmap_destroy(&tgt_matches);
 	ebitmap_destroy(&self_matches);
-	ebitmap_destroy(&matches);
 	return rc;
 }
 
@@ -374,7 +369,7 @@ static int check_assertion_extended_permissions_avtab(avrule_t *avrule, avtab_t
 static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab,
 						avtab_key_t *k, policydb_t *p)
 {
-	ebitmap_t src_matches, tgt_matches, self_matches, matches;
+	ebitmap_t src_matches, tgt_matches, self_matches;
 	unsigned int i, j;
 	ebitmap_node_t *snode, *tnode;
 	int rc;
@@ -382,7 +377,6 @@ static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab
 	ebitmap_init(&src_matches);
 	ebitmap_init(&tgt_matches);
 	ebitmap_init(&self_matches);
-	ebitmap_init(&matches);
 
 	rc = ebitmap_and(&src_matches, &avrule->stypes.types,
 			 &p->attr_type_map[k->source_type - 1]);
@@ -400,11 +394,7 @@ static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab
 		goto oom;
 
 	if (avrule->flags == RULE_SELF) {
-		rc = ebitmap_and(&matches, &p->attr_type_map[k->source_type - 1],
-				&p->attr_type_map[k->target_type - 1]);
-		if (rc < 0)
-			goto oom;
-		rc = ebitmap_and(&self_matches, &avrule->stypes.types, &matches);
+		rc = ebitmap_and(&self_matches, &src_matches, &p->attr_type_map[k->target_type - 1]);
 		if (rc < 0)
 			goto oom;
 
@@ -435,7 +425,7 @@ oom:
 exit:
 	ebitmap_destroy(&src_matches);
 	ebitmap_destroy(&tgt_matches);
-	ebitmap_destroy(&matches);
+	ebitmap_destroy(&self_matches);
 	return rc;
 }
 
-- 
2.31.1

[PATCH 12/16 v2] libsepol: Refactor match_any_class_permissions() to be clearer

[James Carter]

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

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 71ee7815..26fa8d96 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -65,14 +65,11 @@ static void report_failure(sepol_handle_t *handle, policydb_t *p, const avrule_t
 static int match_any_class_permissions(class_perm_node_t *cp, uint32_t class, uint32_t data)
 {
 	for (; cp; cp = cp->next) {
-		if ((cp->tclass == class) && (cp->data & data)) {
-			break;
-		}
+		if ((cp->tclass == class) && (cp->data & data))
+			return 1;
 	}
-	if (!cp)
-		return 0;
 
-	return 1;
+	return 0;
 }
 
 static int extended_permissions_and(uint32_t *perms1, uint32_t *perms2) {
-- 
2.31.1

[PATCH 13/16 v2] libsepol: Make return value clearer when reporting neverallowx errors

[James Carter]

The value returned from report_assertion_extended_permissions() is
the nubmer of errors, so call it that instead of ret.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/src/assertion.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 26fa8d96..93c57061 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -148,8 +148,8 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
 	ebitmap_t *tattr = &p->type_attr_map[ttype];
 	ebitmap_node_t *snode, *tnode;
 	unsigned int i, j;
-	int rc = 1;
-	int ret = 0;
+	int rc;
+	int errors = 0;
 
 	memcpy(&tmp_key, k, sizeof(avtab_key_t));
 	tmp_key.specified = AVTAB_XPERMS_ALLOWED;
@@ -178,15 +178,14 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
 							p->p_class_val_to_name[curperm->tclass - 1],
 							sepol_extended_perms_to_string(&error));
 
-					rc = 0;
-					ret++;
+					errors++;
 				}
 			}
 		}
 	}
 
 	/* failure on the regular permissions */
-	if (rc) {
+	if (!errors) {
 		ERR(handle, "neverallowxperm on line %lu of %s (or line %lu of policy.conf) violated by\n"
 				"allow %s %s:%s {%s };",
 				avrule->source_line, avrule->source_filename, avrule->line,
@@ -194,11 +193,11 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
 				p->p_type_val_to_name[ttype],
 				p->p_class_val_to_name[curperm->tclass - 1],
 				sepol_av_to_string(p, curperm->tclass, perms));
-		ret++;
+		errors++;
 
 	}
 
-	return ret;
+	return errors;
 }
 
 static int report_assertion_avtab_matches(avtab_key_t *k, avtab_datum_t *d, void *args)
-- 
2.31.1

[PATCH 14/16 v2] libsepol: The src and tgt must be the same if neverallow uses self

[James Carter]

When checking for violations of neverallow rules, if the neverallow
uses self, then the src and tgt must be the same when checking
extended permissions and when reporting violations.

Example:
  allow attr attr : CLASS PERM;
  neverallow attr self : CLASS PERM;

If the types t1 and t2 have attribute attr, then the violations
that would be reported would be:
  allow t1 t1 : CLASS PERM;
  allow t1 t2 : CLASS PERM;
  allow t2 t1 : CLASS PERM;
  allow t2 t2 : CLASS PERM;
instead of:
  allow t1 t1 : CLASS PERM;
  allow t2 t2 : CLASS PERM;

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

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 93c57061..1c69f4d9 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -213,6 +213,7 @@ static int report_assertion_avtab_matches(avtab_key_t *k, avtab_datum_t *d, void
 	ebitmap_t src_matches, tgt_matches, self_matches;
 	ebitmap_node_t *snode, *tnode;
 	unsigned int i, j;
+	const int is_avrule_self = (avrule->flags & RULE_SELF) != 0;
 
 	if ((k->specified & AVTAB_ALLOWED) == 0)
 		return 0;
@@ -236,7 +237,7 @@ static int report_assertion_avtab_matches(avtab_key_t *k, avtab_datum_t *d, void
 	if (rc < 0)
 		goto oom;
 
-	if (avrule->flags == RULE_SELF) {
+	if (is_avrule_self) {
 		rc = ebitmap_and(&self_matches, &src_matches, &p->attr_type_map[k->target_type - 1]);
 		if (rc < 0)
 			goto oom;
@@ -260,6 +261,8 @@ static int report_assertion_avtab_matches(avtab_key_t *k, avtab_datum_t *d, void
 
 		ebitmap_for_each_positive_bit(&src_matches, snode, i) {
 			ebitmap_for_each_positive_bit(&tgt_matches, tnode, j) {
+				if (is_avrule_self && i != j)
+					continue;
 				if (avrule->specified == AVRULE_XPERMS_NEVERALLOW) {
 					a->errors += report_assertion_extended_permissions(handle,p, avrule,
 											i, j, cp, perms, k, avtab);
@@ -368,6 +371,7 @@ static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab
 	ebitmap_t src_matches, tgt_matches, self_matches;
 	unsigned int i, j;
 	ebitmap_node_t *snode, *tnode;
+	const int is_avrule_self = (avrule->flags & RULE_SELF) != 0;
 	int rc;
 
 	ebitmap_init(&src_matches);
@@ -389,7 +393,7 @@ static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab
 	if (rc < 0)
 		goto oom;
 
-	if (avrule->flags == RULE_SELF) {
+	if (is_avrule_self) {
 		rc = ebitmap_and(&self_matches, &src_matches, &p->attr_type_map[k->target_type - 1]);
 		if (rc < 0)
 			goto oom;
@@ -408,6 +412,8 @@ static int check_assertion_extended_permissions(avrule_t *avrule, avtab_t *avtab
 
 	ebitmap_for_each_positive_bit(&src_matches, snode, i) {
 		ebitmap_for_each_positive_bit(&tgt_matches, tnode, j) {
+			if (is_avrule_self && i != j)
+				continue;
 			if (check_assertion_extended_permissions_avtab(avrule, avtab, i, j, k, p)) {
 				rc = 1;
 				goto exit;
-- 
2.31.1

[PATCH 15/16 v2] libsepol: Set args avtab pointer when reporting assertion violations

[James Carter]

The changes are the same as in a patch sent by Christian Göttsche
<cgzones@googlemail.com> to support adding not-self to neverallowxperm
checking, but it is needed for normal neverallowxperm checking as well
and the following explanation reflects that.

When reporting neverallowxperm violations, the avtab is searched to
find the rule that violates the assertion. If the avtab pointer of
the args is not set, then it will report the error as if no extended
permissions existed for the source and target (so allowing the ioctl
permission at all violates the neverallowxperm).

Example (where t1 has attribute attr):
  allow attr attr:CLASS ioctl;
  allowxperm attr attr:CLASS ioctl 0x9411;
  neverallowxperm t1 self:CLASS ioctl 0x9411;
Would be reported as:
  neverallowxperm on line 3 of policy.conf (or line 3 of policy.conf)
  violated by
  allow t1 t1:CLASS { ioctl };
Instead of:
  neverallowxperm on line 3 of policy.conf (or line 3 of policy.conf)
  violated by
  allowxperm attr attr:CLASS ioctl { 0x9411 };

Reported-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/src/assertion.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 1c69f4d9..b21c83ba 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -292,10 +292,12 @@ static int report_assertion_failures(sepol_handle_t *handle, policydb_t *p, avru
 	args.avrule = avrule;
 	args.errors = 0;
 
+	args.avtab =  &p->te_avtab;
 	rc = avtab_map(&p->te_avtab, report_assertion_avtab_matches, &args);
 	if (rc < 0)
 		goto oom;
 
+	args.avtab =  &p->te_cond_avtab;
 	rc = avtab_map(&p->te_cond_avtab, report_assertion_avtab_matches, &args);
 	if (rc < 0)
 		goto oom;
-- 
2.31.1

[PATCH 16/16 v2] libsepol: Fix two problems with neverallowxperm reporting

[James Carter]

Not all violations of neverallowxperm rules were being reported.
In check_assertion_extended_permissions_avtab(), a break was
performed after finding a match rather than just returning right
away. This means that if other src and tgt pairs were checked
afterward that did not match, then no match would be reported.

Example:
 allow attr attr:CLASS ioctl;
 allowxperm attr attr:CLASS ioctl 0x9401;
 allowxperm t1 self:CLASS ioctl 0x9421;
 neverallowxperm attr self:CLASS ioctl 0x9421;
Would result in no assertion violations being found.

Another problem was that the reporting function did not properly
recognize when there was a valid allowxperm rule and falsely
reported additional violations that did not exist. (There had
to be at least one legitimate violation.)

Using the same example as above (and assuming t1 and t2 both have
attribute attr), the following would be reported as:
  neverallowxperm on line 4 of policy.conf (or line 4 of policy.conf)
  violated by
  allowxperm t1 t1:CLASS ioctl { 0x9421 };

  neverallowxperm on line 4 of policy.conf (or line 4 of policy.conf)
  violated by
  allow t2 t2:CLASS4 { ioctl };

There is no violation for t2 because there is a valid allowxperm
rule for it.

With this patch, only the first error message (which is the correct
one) is printed.

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

diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index b21c83ba..44c20362 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -149,6 +149,7 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
 	ebitmap_node_t *snode, *tnode;
 	unsigned int i, j;
 	int rc;
+	int found_xperm = 0;
 	int errors = 0;
 
 	memcpy(&tmp_key, k, sizeof(avtab_key_t));
@@ -165,7 +166,7 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
 				if ((xperms->specified != AVTAB_XPERMS_IOCTLFUNCTION)
 						&& (xperms->specified != AVTAB_XPERMS_IOCTLDRIVER))
 					continue;
-
+				found_xperm = 1;
 				rc = check_extended_permissions(avrule->xperms, xperms);
 				/* failure on the extended permission check_extended_permissions */
 				if (rc) {
@@ -185,7 +186,7 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
 	}
 
 	/* failure on the regular permissions */
-	if (!errors) {
+	if (!found_xperm) {
 		ERR(handle, "neverallowxperm on line %lu of %s (or line %lu of policy.conf) violated by\n"
 				"allow %s %s:%s {%s };",
 				avrule->source_line, avrule->source_filename, avrule->line,
@@ -343,7 +344,7 @@ static int check_assertion_extended_permissions_avtab(avrule_t *avrule, avtab_t
 					continue;
 				rc = check_extended_permissions(neverallow_xperms, xperms);
 				if (rc)
-					break;
+					return rc;
 			}
 		}
 	}
-- 
2.31.1

Re: [PATCH 00/16 v2] Refactor and fix assertion checking

[James Carter]

I plan on merging this series next week.
Jim


On Tue, Jan 11, 2022 at 4:54 PM James Carter <jwcart2@gmail.com> wrote:
>
> The first 13 patches refactor and cleanup the neverallow and
> neverallowxperm checking code to make it easier to understand.
>
> The last 3 patches fixes errors in the assertion checking code.
>
> This series is to prepare for adding not-self support to assertion
> checking.
>
> The only change for version 2 is in patch 7 where target_type should
> have been used instead of source_type.
>
> James Carter (16):
>   libsepol: Return an error if check_assertion() returns an error.
>   libsepol: Change label in check_assertion_avtab_match()
>   libsepol: Remove uneeded error messages in assertion checking
>   libsepol: Check for error from check_assertion_extended_permissions()
>   libsepol: Use consistent return checking style
>   libsepol: Move check of target types to before check for self
>   libsepol: Create function check_assertion_self_match() and use it
>   libsepol: Use (rc < 0) instead of (rc) when calling ebitmap functions
>   libsepol: Remove unnessesary check for matching class
>   libsepol: Move assigning outer loop index out of inner loop
>   libsepol: Make use of previously created ebitmap when checking self
>   libsepol: Refactor match_any_class_permissions() to be clearer
>   libsepol: Make return value clearer when reporting neverallowx errors
>   libsepol: The src and tgt must be the same if neverallow uses self
>   libsepol: Set args avtab pointer when reporting assertion violations
>   libsepol: Fix two problems with neverallowxperm reporting
>
>  libsepol/src/assertion.c | 193 +++++++++++++++++++++------------------
>  1 file changed, 102 insertions(+), 91 deletions(-)
>
> --
> 2.31.1
>

Re: [PATCH 00/16 v2] Refactor and fix assertion checking

[James Carter]

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

This series has been merged.
Jim

>
> On Tue, Jan 11, 2022 at 4:54 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > The first 13 patches refactor and cleanup the neverallow and
> > neverallowxperm checking code to make it easier to understand.
> >
> > The last 3 patches fixes errors in the assertion checking code.
> >
> > This series is to prepare for adding not-self support to assertion
> > checking.
> >
> > The only change for version 2 is in patch 7 where target_type should
> > have been used instead of source_type.
> >
> > James Carter (16):
> >   libsepol: Return an error if check_assertion() returns an error.
> >   libsepol: Change label in check_assertion_avtab_match()
> >   libsepol: Remove uneeded error messages in assertion checking
> >   libsepol: Check for error from check_assertion_extended_permissions()
> >   libsepol: Use consistent return checking style
> >   libsepol: Move check of target types to before check for self
> >   libsepol: Create function check_assertion_self_match() and use it
> >   libsepol: Use (rc < 0) instead of (rc) when calling ebitmap functions
> >   libsepol: Remove unnessesary check for matching class
> >   libsepol: Move assigning outer loop index out of inner loop
> >   libsepol: Make use of previously created ebitmap when checking self
> >   libsepol: Refactor match_any_class_permissions() to be clearer
> >   libsepol: Make return value clearer when reporting neverallowx errors
> >   libsepol: The src and tgt must be the same if neverallow uses self
> >   libsepol: Set args avtab pointer when reporting assertion violations
> >   libsepol: Fix two problems with neverallowxperm reporting
> >
> >  libsepol/src/assertion.c | 193 +++++++++++++++++++++------------------
> >  1 file changed, 102 insertions(+), 91 deletions(-)
> >
> > --
> > 2.31.1
> >