From 9af91692416d01814f4b2ac22e39d3b57993af4f Mon Sep 17 00:00:00 2001

From: James Carter <jwcart2@gmail.com>

Date: Wed, 30 Jun 2021 15:12:16 -0400

Subject: [PATCH] libsepol/cil: Improve checking for bad inheritance patterns

commits 37863b0b1444c85a1ddc6c333c8bfea0c678c592 (libsepol/cil:

Improve degenerate inheritance check) and

74d00a8decebf940d95064ff60042dcb2cbcc2c0 (libsepol/cil: Detect

degenerate inheritance and exit with an error) attempted to detect

and exit with an error when compiling policies that have degenerate

inheritances. These policies result in the exponential growth of memory

usage while copying the blocks that are inherited.

There were two problems with the previous attempts to detect this

bad inheritance problem. The first is that the quick check using

cil_possible_degenerate_inheritance() did not detect all patterns

of degenerate inheritance. The second problem is that the detection

of inheritance loops during the CIL_PASS_BLKIN_LINK pass did not

detect all inheritance loops which made it possible for the full

degenerate inheritance checking done with

cil_check_for_degenerate_inheritance() to have a stack overflow

when encountering the inheritance loops. Both the degenerate and

loop inheritance checks need to be done at the same time and done

after the CIL_PASS_BLKIN_LINK pass. Otherwise, if loops are being

detected first, then a degenerate policy can cause the consumption

of all system memory and if degenerate policy is being detected

first, then an inheritance loop can cause a stack overflow.

With the new approach, the quick check is eliminated and the full

check is always done after the CIL_PASS_BLKIN_LINK pass. Because

of this the "inheritance_check" field in struct cil_resolve_args

is not needed and removed and the functions

cil_print_recursive_blockinherit(), cil_check_recursive_blockinherit(),

and cil_possible_degenerate_inheritance() have been deleted. The

function cil_count_potential() is renamed cil_check_inheritances()

and has checks for both degenerate inheritance and inheritance loops.

The inheritance checking is improved and uses an approach similar

to commit c28525a26fa145cb5fd911fd2a3b9125a275677f (libsepol/cil:

Properly check for loops in sets).

As has been the case with these degenerate inheritance patches,

these issues were discovered by the secilc-fuzzer.

Signed-off-by: James Carter <jwcart2@gmail.com>

---

 libsepol/cil/src/cil_resolve_ast.c | 172 +++++++----------------------

 1 file changed, 42 insertions(+), 130 deletions(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c

index 9a02e3867659..145d4e7452dd 100644

--- a/libsepol/cil/src/cil_resolve_ast.c

+++ b/libsepol/cil/src/cil_resolve_ast.c

@@ -64,7 +64,6 @@ struct cil_args_resolve {

 	struct cil_list *sensitivityorder_lists;

 	struct cil_list *in_list;

 	struct cil_stack *disabled_optionals;

-	int *inheritance_check;

 };

 static struct cil_name * __cil_insert_name(struct cil_db *db, hashtab_key_t key, struct cil_tree_node *ast_node)

@@ -2309,100 +2308,8 @@ exit:

 	return rc;

 }

-static void cil_print_recursive_blockinherit(struct cil_tree_node *bi_node, struct cil_tree_node *terminating_node)

-{

-	struct cil_list *trace = NULL;

-	struct cil_list_item *item = NULL;

-	struct cil_tree_node *curr = NULL;

-

-	cil_list_init(&trace, CIL_NODE);

-

-	for (curr = bi_node; curr != terminating_node; curr = curr->parent) {

-		if (curr->flavor == CIL_BLOCK) {

-			cil_list_prepend(trace, CIL_NODE, curr);

-		} else if (curr->flavor == CIL_BLOCKINHERIT) {

-			if (curr != bi_node) {

-				cil_list_prepend(trace, CIL_NODE, NODE(((struct cil_blockinherit *)curr->data)->block));

-			}

-			cil_list_prepend(trace, CIL_NODE, curr);

-		} else {

-			cil_list_prepend(trace, CIL_NODE, curr);

-		}

-	}

-	cil_list_prepend(trace, CIL_NODE, terminating_node);

-

-	cil_list_for_each(item, trace) {

-		curr = item->data;

-		if (curr->flavor == CIL_BLOCK) {

-			cil_tree_log(curr, CIL_ERR, "block %s", DATUM(curr->data)->name);

-		} else if (curr->flavor == CIL_BLOCKINHERIT) {

-			cil_tree_log(curr, CIL_ERR, "blockinherit %s", ((struct cil_blockinherit *)curr->data)->block_str);

-		} else if (curr->flavor == CIL_OPTIONAL) {

-			cil_tree_log(curr, CIL_ERR, "optional %s", DATUM(curr->data)->name);

-		} else {

-			cil_tree_log(curr, CIL_ERR, "%s", cil_node_to_string(curr));

-		}

-	}

-

-	cil_list_destroy(&trace, CIL_FALSE);

-}

-

-static int cil_check_recursive_blockinherit(struct cil_tree_node *bi_node)

-{

-	struct cil_tree_node *curr = NULL;

-	struct cil_blockinherit *bi = NULL;

-	struct cil_block *block = NULL;

-	int rc = SEPOL_ERR;

-

-	bi = bi_node->data;

-

-	for (curr = bi_node->parent; curr != NULL; curr = curr->parent) {

-		if (curr->flavor != CIL_BLOCK) {

-			continue;

-		}

-

-		block = curr->data;

-

-		if (block != bi->block) {

-			continue;

-		}

-

-		cil_log(CIL_ERR, "Recursive blockinherit found:\n");

-		cil_print_recursive_blockinherit(bi_node, curr);

-

-		rc = SEPOL_ERR;

-		goto exit;

-	}

-

-	rc = SEPOL_OK;

-

-exit:

-	return rc;

-}

-

-static int cil_possible_degenerate_inheritance(struct cil_tree_node *node)

-{

-	unsigned depth = 1;

-

-	node = node->parent;

-	while (node && node->flavor != CIL_ROOT) {

-		if (node->flavor == CIL_BLOCK) {

-			if (((struct cil_block *)(node->data))->bi_nodes != NULL) {

-				depth++;

-				if (depth >= CIL_DEGENERATE_INHERITANCE_DEPTH) {

-					return CIL_TRUE;

-				}

-			}

-		}

-		node = node->parent;

-	}

-

-	return CIL_FALSE;

-}

-

 int cil_resolve_blockinherit_link(struct cil_tree_node *current, void *extra_args)

 {

-	struct cil_args_resolve *args = extra_args;

 	struct cil_blockinherit *inherit = current->data;

 	struct cil_symtab_datum *block_datum = NULL;

 	struct cil_tree_node *node = NULL;

@@ -2423,20 +2330,11 @@ int cil_resolve_blockinherit_link(struct cil_tree_node *current, void *extra_arg

 	inherit->block = (struct cil_block *)block_datum;

-	rc = cil_check_recursive_blockinherit(current);

-	if (rc != SEPOL_OK) {

-			goto exit;

-	}

-

 	if (inherit->block->bi_nodes == NULL) {

 		cil_list_init(&inherit->block->bi_nodes, CIL_NODE);

 	}

 	cil_list_append(inherit->block->bi_nodes, CIL_NODE, current);

-	if (*(args->inheritance_check) == CIL_FALSE) {

-		*(args->inheritance_check) = cil_possible_degenerate_inheritance(node);

-	}

-

 	return SEPOL_OK;

 exit:

@@ -2466,11 +2364,6 @@ int cil_resolve_blockinherit_copy(struct cil_tree_node *current, void *extra_arg

 	}

 	cil_list_for_each(item, block->bi_nodes) {

-		rc = cil_check_recursive_blockinherit(item->data);

-		if (rc != SEPOL_OK) {

-			goto exit;

-		}

-

 		rc = cil_copy_ast(db, current, item->data);

 		if (rc != SEPOL_OK) {

 			cil_log(CIL_ERR, "Failed to copy block contents into blockinherit\n");

@@ -3611,34 +3504,58 @@ static unsigned cil_count_actual(struct cil_tree_node *node)

 	return count;

 }

-static unsigned cil_count_potential(struct cil_tree_node *node, unsigned max)

+static int cil_check_inheritances(struct cil_tree_node *node, unsigned max, unsigned *count, struct cil_stack *stack, unsigned *loop)

 {

-	unsigned count = 0;

+	int rc;

 	if (node->flavor == CIL_BLOCKINHERIT) {

 		struct cil_blockinherit *bi = node->data;

-		count += 1;

+		*count += 1;

+		if (*count > max) {

+			cil_tree_log(node, CIL_ERR, "Degenerate inheritance detected");

+			return SEPOL_ERR;

+		}

 		if (bi->block) {

-			count += cil_count_potential(NODE(bi->block), max);

-			if (count > max) {

-				return count;

+			struct cil_tree_node *block_node = NODE(bi->block);

+			struct cil_stack_item *item;

+			int i = 0;

+			cil_stack_for_each(stack, i, item) {

+				if (block_node == (struct cil_tree_node *)item->data) {

+					*loop = CIL_TRUE;

+					cil_tree_log(block_node, CIL_ERR, "Block inheritance loop found");

+					cil_tree_log(node, CIL_ERR, "  blockinherit");

+					return SEPOL_ERR;

+				}

+			}

+			cil_stack_push(stack, CIL_BLOCK, block_node);

+			rc = cil_check_inheritances(block_node, max, count, stack, loop);

+			cil_stack_pop(stack);

+			if (rc != SEPOL_OK) {

+				if (*loop == CIL_TRUE) {

+					cil_tree_log(node, CIL_ERR, "  blockinherit");

+				}

+				return SEPOL_ERR;

 			}

 		}

 	}

 	for (node = node->cl_head; node; node = node->next) {

-		count += cil_count_potential(node, max);

-		if (count > max) {

-			return count;

+		rc = cil_check_inheritances(node, max, count, stack, loop);

+		if (rc != SEPOL_OK) {

+			return SEPOL_ERR;

 		}

 	}

-	return count;

+	return SEPOL_OK;

 }

-static int cil_check_for_degenerate_inheritance(struct cil_tree_node *node)

+static int cil_check_for_bad_inheritance(struct cil_tree_node *node)

 {

-	uint64_t num_actual, num_potential, max;

+	unsigned num_actual, max;

+	unsigned num_potential = 0;

+	unsigned loop = CIL_FALSE;

+	struct cil_stack *stack;

+	int rc;

 	num_actual = cil_count_actual(node);

@@ -3647,13 +3564,11 @@ static int cil_check_for_degenerate_inheritance(struct cil_tree_node *node)

 		max = CIL_DEGENERATE_INHERITANCE_MINIMUM;

 	}

-	num_potential = cil_count_potential(node, max);

+	cil_stack_init(&stack);

+	rc = cil_check_inheritances(node, max, &num_potential, stack, &loop);

+	cil_stack_destroy(&stack);

-	if (num_potential > max) {

-		return SEPOL_ERR;

-	}

-

-	return SEPOL_OK;

+	return rc;

 }

 int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)

@@ -4127,7 +4042,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)

 	struct cil_args_resolve extra_args;

 	enum cil_pass pass = CIL_PASS_TIF;

 	uint32_t changed = 0;

-	int inheritance_check = 0;

 	if (db == NULL || current == NULL) {

 		return rc;

@@ -4147,7 +4061,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)

 	extra_args.sensitivityorder_lists = NULL;

 	extra_args.in_list = NULL;

 	extra_args.disabled_optionals = NULL;

-	extra_args.inheritance_check = &inheritance_check;

 	cil_list_init(&extra_args.to_destroy, CIL_NODE);

 	cil_list_init(&extra_args.sidorder_lists, CIL_LIST_ITEM);

@@ -4174,10 +4087,9 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)

 			cil_list_destroy(&extra_args.in_list, CIL_FALSE);

 		}

-		if (pass == CIL_PASS_BLKIN_LINK && inheritance_check == CIL_TRUE) {

-			rc = cil_check_for_degenerate_inheritance(current);

+		if (pass == CIL_PASS_BLKIN_LINK) {

+			rc = cil_check_for_bad_inheritance(current);

 			if (rc != SEPOL_OK) {

-				cil_log(CIL_ERR, "Degenerate inheritance detected\n");

 				rc = SEPOL_ERR;

 				goto exit;

 			}

-- 

2.32.0