CVE-2021-36087: Out-of-bounds Read (CWE-125)
value: The CIL compiler in SELinux 3.2 has a heap-based buffer over-read in ebitmap_match_any (called indirectly from cil_check_neverallow). This occurs because there is sometimes a lack of checks for invalid statements in an optional block.
CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 3.3
Published: 2021-07-01T03:15Z
Repository: https://github.com/SELinuxProject/selinux
Language: C
Files changed: libsepol/cil/src/cil_build_ast.c, libsepol/cil/src/cil_resolve_ast.c
Commit date: 2021-04-19 14:05:14 -0400
Commit message: libsepol/cil: Check for statements not allowed in optional blocks

While there are some checks for invalid statements in an optional
block when resolving the AST, there are no checks when building the
AST.

OSS-Fuzz found the following policy which caused a null dereference
in cil_tree_get_next_path().
  (blockinherit b3)
  (sid SID)
  (sidorder(SID))
  (optional o
    (ibpkeycon :(1 0)s)
    (block b3
      (filecon""block())
      (filecon""block())))

The problem is that the blockinherit copies block b3 before
the optional block is disabled. When the optional is disabled,
block b3 is deleted along with everything else in the optional.
Later, when filecon statements with the same path are found an
error message is produced and in trying to find out where the block
was copied from, the reference to the deleted block is used. The
error handling code assumes (rightly) that if something was copied
from a block then that block should still exist.

It is clear that in-statements, blocks, and macros cannot be in an
optional, because that allows nodes to be copied from the optional
block to somewhere outside even though the optional could be disabled
later. When optionals are disabled the AST is reset and the
resolution is restarted at the point of resolving macro calls, so
anything resolved before macro calls will never be re-resolved.
This includes tunableifs, in-statements, blockinherits,
blockabstracts, and macro definitions. Tunable declarations also
cannot be in an optional block because they are needed to resolve
tunableifs. It should be fine to allow blockinherit statements in
an optional, because that is copying nodes from outside the optional
to the optional and if the optional is later disabled, everything
will be deleted anyway.

Check and quit with an error if a tunable declaration, in-statement,
block, blockabstract, or macro definition is found within an
optional when either building or resolving the AST.

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

Diff: diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 96c944975..882548585 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -50,10 +50,11 @@ struct cil_args_build {
 	struct cil_tree_node *ast;
 	struct cil_db *db;
 	struct cil_tree_node *tunif;
 	struct cil_tree_node *in;
 	struct cil_tree_node *macro;
+	struct cil_tree_node *optional;
 	struct cil_tree_node *boolif;
 };
 
 int cil_fill_list(struct cil_tree_node *current, enum cil_flavor flavor, struct cil_list **list)
 {
@@ -6069,10 +6070,11 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 	struct cil_db *db = args->db;
 	struct cil_tree_node *ast_current = args->ast;
 	struct cil_tree_node *tunif = args->tunif;
 	struct cil_tree_node *in = args->in;
 	struct cil_tree_node *macro = args->macro;
+	struct cil_tree_node *optional = args->optional;
 	struct cil_tree_node *boolif = args->boolif;
 	struct cil_tree_node *ast_node = NULL;
 	int rc = SEPOL_ERR;
 
 	if (parse_current->parent->cl_head != parse_current) {
@@ -6119,10 +6121,22 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 			cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in macros", (char *)parse_current->data);
 			goto exit;
 		}
 	}
 
+	if (optional != NULL) {
+		if (parse_current->data == CIL_KEY_TUNABLE ||
+			parse_current->data == CIL_KEY_IN ||
+			parse_current->data == CIL_KEY_BLOCK ||
+			parse_current->data == CIL_KEY_BLOCKABSTRACT ||
+			parse_current->data == CIL_KEY_MACRO) {
+			rc = SEPOL_ERR;
+			cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in optionals", (char *)parse_current->data);
+			goto exit;
+		}
+	}
+
 	if (boolif != NULL) {
 		if (parse_current->data != CIL_KEY_TUNABLEIF &&
 			parse_current->data != CIL_KEY_CALL &&
 			parse_current->data != CIL_KEY_CONDTRUE &&
 			parse_current->data != CIL_KEY_CONDFALSE &&
@@ -6460,10 +6474,14 @@ int __cil_build_ast_first_child_helper(__attribute__((unused)) struct cil_tree_n
 
 	if (ast->flavor == CIL_MACRO) {
 		args->macro = ast;
 	}
 
+	if (ast->flavor == CIL_OPTIONAL) {
+		args->optional = ast;
+	}
+
 	if (ast->flavor == CIL_BOOLEANIF) {
 		args->boolif = ast;
 	}
 
 	return SEPOL_OK;
@@ -6490,10 +6508,23 @@ int __cil_build_ast_last_child_helper(struct cil_tree_node *parse_current, void
 
 	if (ast->flavor == CIL_MACRO) {
 		args->macro = NULL;
 	}
 
+	if (ast->flavor == CIL_OPTIONAL) {
+		struct cil_tree_node *n = ast->parent;
+		args->optional = NULL;
+		/* Optionals can be nested */
+		while (n && n->flavor != CIL_ROOT) {
+			if (n->flavor == CIL_OPTIONAL) {
+				args->optional = n;
+				break;
+			}
+			n = n->parent;
+		}
+	}
+
 	if (ast->flavor == CIL_BOOLEANIF) {
 		args->boolif = NULL;
 	}
 
 	// At this point we no longer have any need for parse_current or any of its
@@ -6518,10 +6549,11 @@ int cil_build_ast(struct cil_db *db, struct cil_tree_node *parse_tree, struct ci
 	extra_args.ast = ast;
 	extra_args.db = db;
 	extra_args.tunif = NULL;
 	extra_args.in = NULL;
 	extra_args.macro = NULL;
+	extra_args.optional = NULL;
 	extra_args.boolif = NULL;
 
 	rc = cil_tree_walk(parse_tree, __cil_build_ast_node_helper, __cil_build_ast_first_child_helper, __cil_build_ast_last_child_helper, &extra_args);
 	if (rc != SEPOL_OK) {
 		goto exit;
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 56295a047..efff0f2ec 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -3806,12 +3806,14 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
 		}
 	}
 
 	if (optional != NULL) {
 		if (node->flavor == CIL_TUNABLE ||
+			node->flavor == CIL_IN ||
+			node->flavor == CIL_BLOCK ||
+			node->flavor == CIL_BLOCKABSTRACT ||
 		    node->flavor == CIL_MACRO) {
-			/* tuanbles and macros are not allowed in optionals*/
 			cil_tree_log(node, CIL_ERR, "%s statement is not allowed in optionals", cil_node_to_string(node));
 			rc = SEPOL_ERR;
 			goto exit;
 		}
 	}

Vulnerable code: 			/* tuanbles and macros are not allowed in optionals*/
Fixed code: 	struct cil_tree_node *optional;
	struct cil_tree_node *optional = args->optional;
	if (optional != NULL) {
		if (parse_current->data == CIL_KEY_TUNABLE ||
			parse_current->data == CIL_KEY_IN ||
			parse_current->data == CIL_KEY_BLOCK ||
			parse_current->data == CIL_KEY_BLOCKABSTRACT ||
			parse_current->data == CIL_KEY_MACRO) {
			rc = SEPOL_ERR;
			cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in optionals", (char *)parse_current->data);
			goto exit;
		}
	}

	if (ast->flavor == CIL_OPTIONAL) {
		args->optional = ast;
	}

	if (ast->flavor == CIL_OPTIONAL) {
		struct cil_tree_node *n = ast->parent;
		args->optional = NULL;
		/* Optionals can be nested */
		while (n && n->flavor != CIL_ROOT) {
			if (n->flavor == CIL_OPTIONAL) {
				args->optional = n;
				break;
			}
			n = n->parent;
		}
	}

	extra_args.optional = NULL;
			node->flavor == CIL_IN ||
			node->flavor == CIL_BLOCK ||
			node->flavor == CIL_BLOCKABSTRACT ||