From 0e420cc6f6debc2050229ea537b592c963b81a7c Mon Sep 17 00:00:00 2001

From: James Carter <jwcart2@gmail.com>

Date: Tue, 30 Mar 2021 13:39:18 -0400

Subject: [PATCH] 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>

---

 libsepol/cil/src/cil_build_ast.c   | 32 ++++++++++++++++++++++++++++++

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

 2 files changed, 35 insertions(+), 1 deletion(-)

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

index 3a91be03..4f72884c 100644

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

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

@@ -52,6 +52,7 @@ struct cil_args_build {

 	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;

 };

@@ -6077,6 +6078,7 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f

 	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;

@@ -6127,6 +6129,18 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f

 		}

 	}

+	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 &&

@@ -6468,6 +6482,10 @@ int __cil_build_ast_first_child_helper(__attribute__((unused)) struct cil_tree_n

 		args->macro = ast;

 	}

+	if (ast->flavor == CIL_OPTIONAL) {

+		args->optional = ast;

+	}

+

 	if (ast->flavor == CIL_BOOLEANIF) {

 		args->boolif = ast;

 	}

@@ -6498,6 +6516,19 @@ int __cil_build_ast_last_child_helper(struct cil_tree_node *parse_current, void

 		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;

 	}

@@ -6526,6 +6557,7 @@ int cil_build_ast(struct cil_db *db, struct cil_tree_node *parse_tree, struct ci

 	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);

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

index 06b6ab48..8ffab438 100644

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

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

@@ -3723,8 +3723,10 @@ 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;

-- 

2.30.2