diff mbox series

libsepol/cil: Rewrite verification of map classes and classpermissionsets

Message ID 20200131202450.25665-1-jwcart2@tycho.nsa.gov (mailing list archive)
State Accepted
Headers show
Series libsepol/cil: Rewrite verification of map classes and classpermissionsets | expand

Commit Message

James Carter Jan. 31, 2020, 8:24 p.m. UTC
The classperms associated with each map class permission and with each
classpermissionset are verified in __cil_verify_classperms() which had
multiple problems with how it did the verification.

1) Verification was short-circuited when the first normal class is found.
  The second classpermissionset statement below would not have been
  verified.
    (classpermission cp1)
    (classpermissionset cp1 (CLASS (PERM)))
    (classpermissionset cp1 cp2)

2) The classperms of a map class permission and classpermissionset were
not checked for being NULL before the function recursively called itself.
This would result in a segfault if the missing map or set was referred to
before the classmap or classpermission occured. This error was reported by
Dominick Grift (dominick.grift@defensec.nl).
  These rules would cause a segfault.
    (classmap cm1 (mp1))
    (classmapping cm1 mp1 (cm2 (mp2)))
    (classmap cm2 (mp2))
  But an error would be produced for these rules.
    (classmap cm1 (mp1))
    (classmap cm2 (mp2))
    (classmapping cm2 mp2 (cm1 (mp1)))

3) The loop detection logic was incomplete and could only detect a loop
with a certain statement ordering.
  These rules would cause a stack overflow.
    (classmap cm1 (mp1))
    (classmapping cm1 mp1 (cm2 (mp2)))
    (classmap cm2 (mp2))
    (classmapping cm2 mp2 (cm3 (mp3)))
    (classmap cm3 (mp3))
    (classmapping cm3 mp3 (cm2 (mp2)))

Rewrote __cil_verify_classperms() to fix these errors.

Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
---
 libsepol/cil/src/cil_verify.c | 83 ++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 46 deletions(-)

Comments

Stephen Smalley Feb. 5, 2020, 3:44 p.m. UTC | #1
On 1/31/20 3:24 PM, James Carter wrote:
> The classperms associated with each map class permission and with each
> classpermissionset are verified in __cil_verify_classperms() which had
> multiple problems with how it did the verification.
> 
> 1) Verification was short-circuited when the first normal class is found.
>    The second classpermissionset statement below would not have been
>    verified.
>      (classpermission cp1)
>      (classpermissionset cp1 (CLASS (PERM)))
>      (classpermissionset cp1 cp2)
> 
> 2) The classperms of a map class permission and classpermissionset were
> not checked for being NULL before the function recursively called itself.
> This would result in a segfault if the missing map or set was referred to
> before the classmap or classpermission occured. This error was reported by
> Dominick Grift (dominick.grift@defensec.nl).
>    These rules would cause a segfault.
>      (classmap cm1 (mp1))
>      (classmapping cm1 mp1 (cm2 (mp2)))
>      (classmap cm2 (mp2))
>    But an error would be produced for these rules.
>      (classmap cm1 (mp1))
>      (classmap cm2 (mp2))
>      (classmapping cm2 mp2 (cm1 (mp1)))
> 
> 3) The loop detection logic was incomplete and could only detect a loop
> with a certain statement ordering.
>    These rules would cause a stack overflow.
>      (classmap cm1 (mp1))
>      (classmapping cm1 mp1 (cm2 (mp2)))
>      (classmap cm2 (mp2))
>      (classmapping cm2 mp2 (cm3 (mp3)))
>      (classmap cm3 (mp3))
>      (classmapping cm3 mp3 (cm2 (mp2)))
> 
> Rewrote __cil_verify_classperms() to fix these errors.
> 
> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   libsepol/cil/src/cil_verify.c | 83 ++++++++++++++++-------------------
>   1 file changed, 37 insertions(+), 46 deletions(-)

[...]
James Carter Feb. 6, 2020, 4:04 p.m. UTC | #2
On 2/5/20 10:44 AM, Stephen Smalley wrote:
> On 1/31/20 3:24 PM, James Carter wrote:
>> The classperms associated with each map class permission and with each
>> classpermissionset are verified in __cil_verify_classperms() which had
>> multiple problems with how it did the verification.
>>
>> 1) Verification was short-circuited when the first normal class is found.
>>    The second classpermissionset statement below would not have been
>>    verified.
>>      (classpermission cp1)
>>      (classpermissionset cp1 (CLASS (PERM)))
>>      (classpermissionset cp1 cp2)
>>
>> 2) The classperms of a map class permission and classpermissionset were
>> not checked for being NULL before the function recursively called itself.
>> This would result in a segfault if the missing map or set was referred to
>> before the classmap or classpermission occured. This error was reported by
>> Dominick Grift (dominick.grift@defensec.nl).
>>    These rules would cause a segfault.
>>      (classmap cm1 (mp1))
>>      (classmapping cm1 mp1 (cm2 (mp2)))
>>      (classmap cm2 (mp2))
>>    But an error would be produced for these rules.
>>      (classmap cm1 (mp1))
>>      (classmap cm2 (mp2))
>>      (classmapping cm2 mp2 (cm1 (mp1)))
>>
>> 3) The loop detection logic was incomplete and could only detect a loop
>> with a certain statement ordering.
>>    These rules would cause a stack overflow.
>>      (classmap cm1 (mp1))
>>      (classmapping cm1 mp1 (cm2 (mp2)))
>>      (classmap cm2 (mp2))
>>      (classmapping cm2 mp2 (cm3 (mp3)))
>>      (classmap cm3 (mp3))
>>      (classmapping cm3 mp3 (cm2 (mp2)))
>>
>> Rewrote __cil_verify_classperms() to fix these errors.
>>
>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
> 
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> 
>> ---
>>   libsepol/cil/src/cil_verify.c | 83 ++++++++++++++++-------------------
>>   1 file changed, 37 insertions(+), 46 deletions(-)
> 
> [...]
> 

Applied.
diff mbox series

Patch

diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
index 018514dc..0a098dde 100644
--- a/libsepol/cil/src/cil_verify.c
+++ b/libsepol/cil/src/cil_verify.c
@@ -1545,25 +1545,49 @@  exit:
 	return rc;
 }
 
-static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig)
+static int __cil_verify_classperms(struct cil_list *classperms,
+				   struct cil_symtab_datum *orig,
+				   struct cil_symtab_datum *parent,
+				   struct cil_symtab_datum *cur,
+				   enum cil_flavor flavor,
+				   unsigned steps, unsigned limit)
 {
 	int rc = SEPOL_ERR;
 	struct cil_list_item *curr;
 
+	if (classperms == NULL) {
+		if (flavor == CIL_MAP_PERM) {
+			cil_tree_log(NODE(cur), CIL_ERR, "Map class %s does not have a classmapping for %s", parent->name, cur->name);
+		} else {
+			cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", cur->name);
+		}
+		goto exit;
+	}
+
+	if (steps > 0 && orig == cur) {
+		if (flavor == CIL_MAP_PERM) {
+			cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the map class %s and permission %s", parent->name, cur->name);
+		} else {
+			cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the set %s", cur->name);
+		}
+		goto exit;
+	} else {
+		steps++;
+		if (steps > limit) {
+			steps = 1;
+			limit *= 2;
+			orig = cur;
+		}
+	}
+
 	cil_list_for_each(curr, classperms) {
 		if (curr->flavor == CIL_CLASSPERMS) {
 			struct cil_classperms *cp = curr->data;
-			if (FLAVOR(cp->class) == CIL_CLASS) {
-				return SEPOL_OK;
-			} else { /* MAP */
+			if (FLAVOR(cp->class) != CIL_CLASS) { /* MAP */
 				struct cil_list_item *i = NULL;
 				cil_list_for_each(i, cp->perms) {
 					struct cil_perm *cmp = i->data;
-					if (&cmp->datum == orig) {
-						rc = SEPOL_ERR;
-						goto exit;
-					}
-					rc = __cil_verify_classperms(cmp->classperms, orig);
+					rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit);
 					if (rc != SEPOL_OK) {
 						goto exit;
 					}
@@ -1572,11 +1596,7 @@  static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symta
 		} else { /* SET */
 			struct cil_classperms_set *cp_set = curr->data;
 			struct cil_classpermission *cp = cp_set->set;
-			if (&cp->datum == orig) {
-				rc = SEPOL_ERR;
-				goto exit;
-			}
-			rc = __cil_verify_classperms(cp->classperms, orig);
+			rc = __cil_verify_classperms(cp->classperms, orig, NULL, &cp->datum, CIL_CLASSPERMISSION, steps, limit);
 			if (rc != SEPOL_OK) {
 				goto exit;
 			}
@@ -1586,30 +1606,14 @@  static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symta
 	return SEPOL_OK;
 
 exit:
-	return rc;
+	return SEPOL_ERR;
 }
 
 static int __cil_verify_classpermission(struct cil_tree_node *node)
 {
-	int rc = SEPOL_ERR;
 	struct cil_classpermission *cp = node->data;
 
-	if (cp->classperms == NULL) {
-		cil_tree_log(node, CIL_ERR, "Classpermission %s does not have a classpermissionset", cp->datum.name);
-		rc = SEPOL_ERR;
-		goto exit;
-	}
-
-	rc = __cil_verify_classperms(cp->classperms, &cp->datum);
-	if (rc != SEPOL_OK) {
-		cil_tree_log(node, CIL_ERR, "Found circular class permissions involving the set %s",cp->datum.name);
-		goto exit;
-	}
-
-	rc = SEPOL_OK;
-
-exit:
-	return rc;
+	return __cil_verify_classperms(cp->classperms, &cp->datum, NULL, &cp->datum, CIL_CLASSPERMISSION, 0, 2);
 }
 
 struct cil_verify_map_args {
@@ -1620,24 +1624,11 @@  struct cil_verify_map_args {
 
 static int __verify_map_perm_classperms(__attribute__((unused)) hashtab_key_t k, hashtab_datum_t d, void *args)
 {
-	int rc = SEPOL_ERR;
 	struct cil_verify_map_args *map_args = args;
 	struct cil_perm *cmp = (struct cil_perm *)d;
 
-	if (cmp->classperms == NULL) {
-		cil_tree_log(map_args->node, CIL_ERR, "Map class %s does not have a classmapping for %s", map_args->class->datum.name, cmp->datum.name);
-		map_args->rc = SEPOL_ERR;
-		goto exit;
-	}
-
-	rc = __cil_verify_classperms(cmp->classperms, &cmp->datum);
-	if (rc != SEPOL_OK) {
-		cil_tree_log(map_args->node, CIL_ERR, "Found circular class permissions involving the map class %s and permission %s", map_args->class->datum.name, cmp->datum.name);
-		map_args->rc = SEPOL_ERR;
-		goto exit;
-	}
+	map_args->rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &map_args->class->datum, &cmp->datum, CIL_MAP_PERM, 0, 2);
 
-exit:
 	return SEPOL_OK;
 }