From e81c466bca9a06e2ada7d783fe31dd44c9e04432 Mon Sep 17 00:00:00 2001
From: James Carter <jwcart2@gmail.com>
Date: Thu, 20 Apr 2023 08:58:01 -0400
Subject: [PATCH] libsepol/cil: Fix class permission verification in CIL
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Before the CIL post processing phase (where expressions are evaluated,
various ebitmaps are set, etc) there is a pre-verification where
checks are made to find self references or loops in bounds, attribute
sets, and class permissions. The class permission checking is faulty
in two ways.

First, it does not check for the use of "all" in a permission expression
for a class that has no permissions. An error will still be generated
later and secilc will exit cleanly, but without an error message that
explains the problem.

Second, it does not properly handle lists in permission expressions.
For example, "(C ((P)))" is a legitimate class permission. The
permissions expression contains one item that is a list containing
one permission. This permission expression will be properly evaluated.
Unfortunately, the class permission verification assumes that each
item in the permission expression is either an operator or a
permission datum and a segmenation fault will occur.

Refactor the class permission checking to give a proper error when
"all" is used in a permission expression for a class that has no
permissions and so that it can handle lists in permission
expressions. Also, check for the actual flavor of each item in
the permission expression and return an error if an unexpected
flavor is found.

The failure to properly handle lists in permission expressions was
found by oss-fuzz (#58085).

Tested-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_verify.c | 167 +++++++++++++++++++++++-----------
 1 file changed, 114 insertions(+), 53 deletions(-)

diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
index 4640dc59de..3f58969df2 100644
--- a/libsepol/cil/src/cil_verify.c
+++ b/libsepol/cil/src/cil_verify.c
@@ -1700,31 +1700,109 @@ static int __add_perm_to_list(__attribute__((unused)) hashtab_key_t k, hashtab_d
 	return SEPOL_OK;
 }
 
-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)
+static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit);
+
+static int __cil_verify_map_perm(struct cil_class *class, struct cil_perm *perm, struct cil_symtab_datum *orig, unsigned steps, unsigned limit)
+{
+	int rc;
+
+	if (!perm->classperms) {
+		cil_tree_log(NODE(class), CIL_ERR, "No class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name);
+		goto exit;
+	}
+
+	rc = __cil_verify_classperms(perm->classperms, orig, &perm->datum, steps, limit);
+	if (rc != SEPOL_OK) {
+		cil_tree_log(NODE(class), CIL_ERR, "There was an error verifying class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name);
+		goto exit;
+	}
+
+	return SEPOL_OK;
+
+exit:
+	return SEPOL_ERR;
+}
+
+
+static int __cil_verify_perms(struct cil_class *class, struct cil_list *perms, struct cil_symtab_datum *orig, unsigned steps, unsigned limit)
 {
 	int rc = SEPOL_ERR;
-	struct cil_list_item *curr;
+	int count = 0;
+	struct cil_list_item *i = NULL;
 
-	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);
+	if (!perms) {
+		cil_tree_log(NODE(class), CIL_ERR, "No permissions for class %s in class permissions", DATUM(class)->name);
+		goto exit;
+	}
+
+	cil_list_for_each(i, perms) {
+		count++;
+		if (i->flavor == CIL_LIST) {
+			rc = __cil_verify_perms(class, i->data, orig, steps, limit);
+			if (rc != SEPOL_OK) {
+				goto exit;
+			}
+		} else if (i->flavor == CIL_DATUM) {
+			struct cil_perm *perm = i->data;
+			if (FLAVOR(perm) == CIL_MAP_PERM) {
+				rc = __cil_verify_map_perm(class, perm, orig, steps, limit);
+				if (rc != SEPOL_OK) {
+					goto exit;
+				}
+			}
+		} else if (i->flavor == CIL_OP) {
+			enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data;
+			if (op == CIL_ALL) {
+				struct cil_list *perm_list;
+				struct cil_list_item *j = NULL;
+				int count2 = 0;
+				cil_list_init(&perm_list, CIL_MAP_PERM);
+				cil_symtab_map(&class->perms, __add_perm_to_list, perm_list);
+				cil_list_for_each(j, perm_list) {
+					count2++;
+					struct cil_perm *perm = j->data;
+					if (FLAVOR(perm) == CIL_MAP_PERM) {
+						rc = __cil_verify_map_perm(class, perm, orig, steps, limit);
+						if (rc != SEPOL_OK) {
+							cil_list_destroy(&perm_list, CIL_FALSE);
+							goto exit;
+						}
+					}
+				}
+				cil_list_destroy(&perm_list, CIL_FALSE);
+				if (count2 == 0) {
+					cil_tree_log(NODE(class), CIL_ERR, "Operator \"all\" used for %s which has no permissions associated with it", DATUM(class)->name);
+					goto exit;
+				}
+			}
 		} else {
-			cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", cur->name);
+			cil_tree_log(NODE(class), CIL_ERR, "Permission list for %s has an unexpected flavor: %d", DATUM(class)->name, i->flavor);
+			goto exit;
 		}
+	}
+
+	if (count == 0) {
+		cil_tree_log(NODE(class), CIL_ERR, "Empty permissions list for class %s in class permissions", DATUM(class)->name);
+		goto exit;
+	}
+
+	return SEPOL_OK;
+
+exit:
+	return SEPOL_ERR;
+}
+
+static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit)
+{
+	int rc;
+	struct cil_list_item *i;
+
+	if (classperms == NULL) {
 		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);
-		}
+		cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving %s", cur->name);
 		goto exit;
 	} else {
 		steps++;
@@ -1735,44 +1813,20 @@ static int __cil_verify_classperms(struct cil_list *classperms,
 		}
 	}
 
-	cil_list_for_each(curr, classperms) {
-		if (curr->flavor == CIL_CLASSPERMS) {
-			struct cil_classperms *cp = curr->data;
-			if (FLAVOR(cp->class) != CIL_CLASS) { /* MAP */
-				struct cil_list_item *i = NULL;
-				cil_list_for_each(i, cp->perms) {
-					if (i->flavor != CIL_OP) {
-						struct cil_perm *cmp = i->data;
-						rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit);
-						if (rc != SEPOL_OK) {
-							goto exit;
-						}
-					} else {
-						enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data;
-						if (op == CIL_ALL) {
-							struct cil_class *mc = cp->class;
-							struct cil_list *perm_list;
-							struct cil_list_item *j = NULL;
-
-							cil_list_init(&perm_list, CIL_MAP_PERM);
-							cil_symtab_map(&mc->perms, __add_perm_to_list, perm_list);
-							cil_list_for_each(j, perm_list) {
-								struct cil_perm *cmp = j->data;
-								rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit);
-								if (rc != SEPOL_OK) {
-									cil_list_destroy(&perm_list, CIL_FALSE);
-									goto exit;
-								}
-							}
-							cil_list_destroy(&perm_list, CIL_FALSE);
-						}
-					}
-				}
+	cil_list_for_each(i, classperms) {
+		if (i->flavor == CIL_CLASSPERMS) {
+			struct cil_classperms *cp = i->data;
+			rc = __cil_verify_perms(cp->class, cp->perms, orig, steps, limit);
+			if (rc != SEPOL_OK) {
+				goto exit;
 			}
 		} else { /* SET */
-			struct cil_classperms_set *cp_set = curr->data;
+			struct cil_classperms_set *cp_set = i->data;
 			struct cil_classpermission *cp = cp_set->set;
-			rc = __cil_verify_classperms(cp->classperms, orig, NULL, &cp->datum, CIL_CLASSPERMISSION, steps, limit);
+			if (!cp->classperms) {
+				cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", DATUM(cp)->name);
+			}
+			rc = __cil_verify_classperms(cp->classperms, orig, &cp->datum, steps, limit);
 			if (rc != SEPOL_OK) {
 				goto exit;
 			}
@@ -1787,9 +1841,15 @@ static int __cil_verify_classperms(struct cil_list *classperms,
 
 static int __cil_verify_classpermission(struct cil_tree_node *node)
 {
+	int rc;
 	struct cil_classpermission *cp = node->data;
 
-	return __cil_verify_classperms(cp->classperms, &cp->datum, NULL, &cp->datum, CIL_CLASSPERMISSION, 0, 2);
+	rc = __cil_verify_classperms(cp->classperms, &cp->datum, &cp->datum, 0, 2);
+	if (rc != SEPOL_OK) {
+		cil_tree_log(node, CIL_ERR, "Error verifying class permissions for classpermission %s", DATUM(cp)->name);
+	}
+
+	return rc;
 }
 
 struct cil_verify_map_args {
@@ -1804,8 +1864,9 @@ static int __verify_map_perm_classperms(__attribute__((unused)) hashtab_key_t k,
 	struct cil_perm *cmp = (struct cil_perm *)d;
 	int rc;
 
-	rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &map_args->class->datum, &cmp->datum, CIL_MAP_PERM, 0, 2);
+	rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &cmp->datum, 0, 2);
 	if (rc != SEPOL_OK) {
+		cil_tree_log(NODE(cmp), CIL_ERR, "Error verifying class permissions for map class %s, permission %s", DATUM(map_args->class)->name, DATUM(cmp)->name);
 		map_args->rc = rc;
 	}