[PATCH 00/12] Update checks for invalid rules in blocks

[PATCH 00/12] Update checks for invalid rules in blocks

[James Carter]

Looking into a bug that OSS-Fuzz found led to patch 7, Check for
statements not allowed in optional blocks, which is the most important
patch in this series. Working on patch 7 led to fixing some other
problems with the checks for invalid rules, cleaning up some of the code,
and improving the CIL documentation.

Patches 1, 2, 4, 5, and 10 are doing various cleanups.
Patch 3 fixes a bug that prevents the first rule in a block from being checked.
Patches 6, 7, 8, and 9 update the checks for invalid rules.
Patch 11 fixes a bug that prevented some error messages from being displayed.
Patch 12 updates the CIL documentation.

There is still work to do in this area. I am not sure why sensitivity and
category statements are not allowed in blocks, but everything else is. That
is why I didn't add those checks when building the AST. It is not clear if
mls, handleunknown, defaultuser, defaultrole, defaulttype, defaultrange,
and policycap should be restricted to the global namespace.

James Carter (12):
  libsepol/cil: Reorder checks for invalid rules when building AST
  libsepol/cil: Cleanup build AST helper functions
  libsepol/cil: Create new first child helper function for building AST
  libsepol/cil: Use AST to track blocks and optionals when resolving
  libsepol/cil: Reorder checks for invalid rules when resolving AST
  libsepol/cil: Sync checks for invalid rules in booleanifs
  libsepol/cil: Check for statements not allowed in optional blocks
  libsepol/cil: Sync checks for invalid rules in macros
  libsepol/cil: Do not allow tunable declarations in in-statements
  libsepol/cil: Make invalid statement error messages consistent
  libsepol/cil: Use CIL_ERR for error messages in cil_compile()
  secilc/docs: Update the CIL documentation for various blocks

 libsepol/cil/src/cil.c                    |   8 +-
 libsepol/cil/src/cil_build_ast.c          | 193 ++++++++++++----------
 libsepol/cil/src/cil_resolve_ast.c        | 174 ++++++++-----------
 secilc/docs/cil_call_macro_statements.md  |   2 +
 secilc/docs/cil_conditional_statements.md |   6 +
 secilc/docs/cil_container_statements.md   |  28 ++--
 6 files changed, 205 insertions(+), 206 deletions(-)

-- 
2.26.3

[PATCH 01/12] libsepol/cil: Reorder checks for invalid rules when building AST

[James Carter]

Reorder checks for invalid rules in the blocks of tunableifs,
in-statements, macros, and booleanifs when building the AST for
consistency.

Order the checks in the same order the blocks will be resolved in,
so tuanbleif, in-statement, macro, booleanif, and then non-block
rules.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_build_ast.c | 100 +++++++++++++++----------------
 1 file changed, 50 insertions(+), 50 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 4e53f06a..1c530bbc 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -49,10 +49,10 @@
 struct cil_args_build {
 	struct cil_tree_node *ast;
 	struct cil_db *db;
-	struct cil_tree_node *macro;
-	struct cil_tree_node *boolif;
 	struct cil_tree_node *tunif;
 	struct cil_tree_node *in;
+	struct cil_tree_node *macro;
+	struct cil_tree_node *boolif;
 };
 
 int cil_fill_list(struct cil_tree_node *current, enum cil_flavor flavor, struct cil_list **list)
@@ -6097,10 +6097,10 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 	struct cil_tree_node *ast_current = NULL;
 	struct cil_db *db = NULL;
 	struct cil_tree_node *ast_node = NULL;
-	struct cil_tree_node *macro = NULL;
-	struct cil_tree_node *boolif = NULL;
 	struct cil_tree_node *tunif = NULL;
 	struct cil_tree_node *in = NULL;
+	struct cil_tree_node *macro = NULL;
+	struct cil_tree_node *boolif = NULL;
 	int rc = SEPOL_ERR;
 
 	if (parse_current == NULL || finished == NULL || extra_args == NULL) {
@@ -6110,10 +6110,10 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 	args = extra_args;
 	ast_current = args->ast;
 	db = args->db;
-	macro = args->macro;
-	boolif = args->boolif;
 	tunif = args->tunif;
 	in = args->in;
+	macro = args->macro;
+	boolif = args->boolif;
 
 	if (parse_current->parent->cl_head != parse_current) {
 		/* ignore anything that isn't following a parenthesis */
@@ -6130,13 +6130,31 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 		goto exit;
 	}
 
+	if (tunif != NULL) {
+		if (parse_current->data == CIL_KEY_TUNABLE) {
+			rc = SEPOL_ERR;
+			cil_tree_log(parse_current, CIL_ERR, "Found tunable");
+			cil_log(CIL_ERR, "Tunables cannot be defined within tunableif statement\n");
+			goto exit;
+		}
+	}
+
+	if (in != NULL) {
+		if (parse_current->data == CIL_KEY_IN) {
+			rc = SEPOL_ERR;
+			cil_tree_log(parse_current, CIL_ERR, "Found in-statement");
+			cil_log(CIL_ERR, "in-statements cannot be defined within in-statements\n");
+			goto exit;
+		}
+	}
+
 	if (macro != NULL) {
-		if (parse_current->data == CIL_KEY_MACRO ||
-			parse_current->data == CIL_KEY_TUNABLE ||
+		if (parse_current->data == CIL_KEY_TUNABLE ||
 			parse_current->data == CIL_KEY_IN ||
 			parse_current->data == CIL_KEY_BLOCK ||
 			parse_current->data == CIL_KEY_BLOCKINHERIT ||
-			parse_current->data == CIL_KEY_BLOCKABSTRACT) {
+			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 macros", (char *)parse_current->data);
 			goto exit;
@@ -6144,15 +6162,15 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 	}
 
 	if (boolif != NULL) {
-		if (parse_current->data != CIL_KEY_CONDTRUE &&
+		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 &&
-			parse_current->data != CIL_KEY_AUDITALLOW &&
-			parse_current->data != CIL_KEY_TUNABLEIF &&
 			parse_current->data != CIL_KEY_ALLOW &&
 			parse_current->data != CIL_KEY_DONTAUDIT &&
+			parse_current->data != CIL_KEY_AUDITALLOW &&
 			parse_current->data != CIL_KEY_TYPETRANSITION &&
-			parse_current->data != CIL_KEY_TYPECHANGE &&
-			parse_current->data != CIL_KEY_CALL) {
+			parse_current->data != CIL_KEY_TYPECHANGE) {
 			rc = SEPOL_ERR;
 			cil_tree_log(parse_current, CIL_ERR, "Found %s", (char*)parse_current->data);
 			if (((struct cil_booleanif*)boolif->data)->preserved_tunable) {
@@ -6166,24 +6184,6 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 		}
 	}
 
-	if (tunif != NULL) {
-		if (parse_current->data == CIL_KEY_TUNABLE) {
-			rc = SEPOL_ERR;
-			cil_tree_log(parse_current, CIL_ERR, "Found tunable");
-			cil_log(CIL_ERR, "Tunables cannot be defined within tunableif statement\n");
-			goto exit;
-		}
-	}
-
-	if (in != NULL) {
-		if (parse_current->data == CIL_KEY_IN) {
-			rc = SEPOL_ERR;
-			cil_tree_log(parse_current, CIL_ERR, "Found in-statement");
-			cil_log(CIL_ERR, "in-statements cannot be defined within in-statements\n");
-			goto exit;
-		}
-	}
-
 	cil_tree_node_init(&ast_node);
 
 	ast_node->parent = ast_current;
@@ -6469,14 +6469,6 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 
 	if (rc == SEPOL_OK) {
 		if (ast_current->cl_head == NULL) {
-			if (ast_current->flavor == CIL_MACRO) {
-				args->macro = ast_current;
-			}
-
-			if (ast_current->flavor == CIL_BOOLEANIF) {
-				args->boolif = ast_current;
-			}
-
 			if (ast_current->flavor == CIL_TUNABLEIF) {
 				args->tunif = ast_current;
 			}
@@ -6485,6 +6477,14 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 				args->in = ast_current;
 			}
 
+			if (ast_current->flavor == CIL_MACRO) {
+				args->macro = ast_current;
+			}
+
+			if (ast_current->flavor == CIL_BOOLEANIF) {
+				args->boolif = ast_current;
+			}
+
 			ast_current->cl_head = ast_node;
 		} else {
 			ast_current->cl_tail->next = ast_node;
@@ -6520,14 +6520,6 @@ int __cil_build_ast_last_child_helper(struct cil_tree_node *parse_current, void
 
 	args->ast = ast->parent;
 
-	if (ast->flavor == CIL_MACRO) {
-		args->macro = NULL;
-	}
-
-	if (ast->flavor == CIL_BOOLEANIF) {
-		args->boolif = NULL;
-	}
-
 	if (ast->flavor == CIL_TUNABLEIF) {
 		args->tunif = NULL;
 	}
@@ -6536,6 +6528,14 @@ int __cil_build_ast_last_child_helper(struct cil_tree_node *parse_current, void
 		args->in = NULL;
 	}
 
+	if (ast->flavor == CIL_MACRO) {
+		args->macro = NULL;
+	}
+
+	if (ast->flavor == CIL_BOOLEANIF) {
+		args->boolif = NULL;
+	}
+
 	// At this point we no longer have any need for parse_current or any of its
 	// siblings; they have all been converted to the appropriate AST node. The
 	// full parse tree will get deleted elsewhere, but in an attempt to
@@ -6560,10 +6560,10 @@ 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.macro = NULL;
-	extra_args.boolif = NULL;
 	extra_args.tunif = NULL;
 	extra_args.in = NULL;
+	extra_args.macro = NULL;
+	extra_args.boolif = NULL;
 
 	rc = cil_tree_walk(parse_tree, __cil_build_ast_node_helper, NULL, __cil_build_ast_last_child_helper, &extra_args);
 	if (rc != SEPOL_OK) {
-- 
2.26.3

[PATCH 02/12] libsepol/cil: Cleanup build AST helper functions

[James Carter]

Since parse_current, finished, and extra_args can never be NULL,
remove the useless check and directly assign local variables from
extra_args.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_build_ast.c | 44 ++++++++------------------------
 1 file changed, 10 insertions(+), 34 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 1c530bbc..dd57ad82 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -6093,28 +6093,16 @@ void cil_destroy_src_info(struct cil_src_info *info)
 
 int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *finished, void *extra_args)
 {
-	struct cil_args_build *args = NULL;
-	struct cil_tree_node *ast_current = NULL;
-	struct cil_db *db = NULL;
+	struct cil_args_build *args = extra_args;
+	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 *boolif = args->boolif;
 	struct cil_tree_node *ast_node = NULL;
-	struct cil_tree_node *tunif = NULL;
-	struct cil_tree_node *in = NULL;
-	struct cil_tree_node *macro = NULL;
-	struct cil_tree_node *boolif = NULL;
 	int rc = SEPOL_ERR;
 
-	if (parse_current == NULL || finished == NULL || extra_args == NULL) {
-		goto exit;
-	}
-
-	args = extra_args;
-	ast_current = args->ast;
-	db = args->db;
-	tunif = args->tunif;
-	in = args->in;
-	macro = args->macro;
-	boolif = args->boolif;
-
 	if (parse_current->parent->cl_head != parse_current) {
 		/* ignore anything that isn't following a parenthesis */
 		rc = SEPOL_OK;
@@ -6502,20 +6490,11 @@ exit:
 
 int __cil_build_ast_last_child_helper(struct cil_tree_node *parse_current, void *extra_args)
 {
-	int rc = SEPOL_ERR;
-	struct cil_tree_node *ast = NULL;
-	struct cil_args_build *args = NULL;
-
-	if (extra_args == NULL) {
-		goto exit;
-	}
-
-	args = extra_args;
-	ast = args->ast;
+	struct cil_args_build *args = extra_args;
+	struct cil_tree_node *ast = args->ast;
 
 	if (ast->flavor == CIL_ROOT) {
-		rc = SEPOL_OK;
-		goto exit;
+		return SEPOL_OK;
 	}
 
 	args->ast = ast->parent;
@@ -6544,9 +6523,6 @@ int __cil_build_ast_last_child_helper(struct cil_tree_node *parse_current, void
 	cil_tree_children_destroy(parse_current->parent);
 
 	return SEPOL_OK;
-
-exit:
-	return rc;
 }
 
 int cil_build_ast(struct cil_db *db, struct cil_tree_node *parse_tree, struct cil_tree_node *ast)
-- 
2.26.3

[PATCH 03/12] libsepol/cil: Create new first child helper function for building AST

[James Carter]

In order to find statements not allowed in tunableifs, in-statements,
macros, and booleanifs, there are tree node pointers that point to
each of these kinds of statements when its block is being parsed.
If the pointer is non-NULL, then the rule being parsed is in the block
of that kind of statement.

The tree node pointers were being updated at the wrong point which
prevented an invalid statement from being found if it was the first
statement in the block of a tunableif, in-statement, macro, or
booleanif.

Create a first child helper function for walking the parse tree and
in that function set the appropriate tree node pointer if the
current AST node is a tunableif, in-statement, macro, or booleanif.
This also makes the code symmetrical with the last child helper
where the tree node pointers are set to NULL.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_build_ast.c | 42 +++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index dd57ad82..c0783ba6 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -6457,22 +6457,6 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 
 	if (rc == SEPOL_OK) {
 		if (ast_current->cl_head == NULL) {
-			if (ast_current->flavor == CIL_TUNABLEIF) {
-				args->tunif = ast_current;
-			}
-
-			if (ast_current->flavor == CIL_IN) {
-				args->in = ast_current;
-			}
-
-			if (ast_current->flavor == CIL_MACRO) {
-				args->macro = ast_current;
-			}
-
-			if (ast_current->flavor == CIL_BOOLEANIF) {
-				args->boolif = ast_current;
-			}
-
 			ast_current->cl_head = ast_node;
 		} else {
 			ast_current->cl_tail->next = ast_node;
@@ -6488,6 +6472,30 @@ exit:
 	return rc;
 }
 
+int __cil_build_ast_first_child_helper(__attribute__((unused)) struct cil_tree_node *parse_current, void *extra_args)
+{
+	struct cil_args_build *args = extra_args;
+	struct cil_tree_node *ast = args->ast;
+
+	if (ast->flavor == CIL_TUNABLEIF) {
+		args->tunif = ast;
+	}
+
+	if (ast->flavor == CIL_IN) {
+		args->in = ast;
+	}
+
+	if (ast->flavor == CIL_MACRO) {
+		args->macro = ast;
+	}
+
+	if (ast->flavor == CIL_BOOLEANIF) {
+		args->boolif = ast;
+	}
+
+	return SEPOL_OK;
+}
+
 int __cil_build_ast_last_child_helper(struct cil_tree_node *parse_current, void *extra_args)
 {
 	struct cil_args_build *args = extra_args;
@@ -6541,7 +6549,7 @@ int cil_build_ast(struct cil_db *db, struct cil_tree_node *parse_tree, struct ci
 	extra_args.macro = NULL;
 	extra_args.boolif = NULL;
 
-	rc = cil_tree_walk(parse_tree, __cil_build_ast_node_helper, NULL, __cil_build_ast_last_child_helper, &extra_args);
+	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;
 	}
-- 
2.26.3

[PATCH 04/12] libsepol/cil: Use AST to track blocks and optionals when resolving

[James Carter]

When resolving the AST, block and optional stacks are used to
determine if the current rule being resolved is in a block or
an optional. There is no need to do this since the parent node
pointers can be used when exiting a block or an optional to
determine if resolution is still within a block or an optional.

When entering either a block or an optional, update the appropriate
tree node pointer. When finished with the last child of a block or
optional, set the appropriate pointer to NULL. If a parent of the
same kind is found when the parent node pointers are followed back
to the root node, then set the pointer to that tree node.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_resolve_ast.c | 107 +++++++++--------------------
 1 file changed, 32 insertions(+), 75 deletions(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 63beed92..a61462d0 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -52,10 +52,10 @@ struct cil_args_resolve {
 	enum cil_pass pass;
 	uint32_t *changed;
 	struct cil_list *disabled_optionals;
-	struct cil_tree_node *optstack;
+	struct cil_tree_node *optional;
 	struct cil_tree_node *boolif;
 	struct cil_tree_node *macro;
-	struct cil_tree_node *blockstack;
+	struct cil_tree_node *block;
 	struct cil_list *sidorder_lists;
 	struct cil_list *classorder_lists;
 	struct cil_list *unordered_classorder_lists;
@@ -3777,16 +3777,16 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
 	int rc = SEPOL_ERR;
 	struct cil_args_resolve *args = extra_args;
 	enum cil_pass pass = args->pass;
-	struct cil_tree_node *optstack = args->optstack;
+	struct cil_tree_node *optional = args->optional;
 	struct cil_tree_node *boolif = args->boolif;
-	struct cil_tree_node *blockstack = args->blockstack;
+	struct cil_tree_node *block = args->block;
 	struct cil_tree_node *macro = args->macro;
 
 	if (node == NULL) {
 		goto exit;
 	}
 
-	if (optstack != NULL) {
+	if (optional != NULL) {
 		if (node->flavor == CIL_TUNABLE || 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));
@@ -3795,7 +3795,7 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
 		}
 	}
 
-	if (blockstack != NULL) {
+	if (block != NULL) {
 		if (node->flavor == CIL_CAT || node->flavor == CIL_SENS) {
 			cil_tree_log(node, CIL_ERR, "%s statement is not allowed in blocks", cil_node_to_string(node));
 			rc = SEPOL_ERR;
@@ -3849,11 +3849,11 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
 	if (rc == SEPOL_ENOENT) {
 		enum cil_log_level lvl = CIL_ERR;
 
-		if (optstack != NULL) {
+		if (optional != NULL) {
 			lvl = CIL_INFO;
 
-			struct cil_optional *opt = (struct cil_optional *)optstack->data;
-			struct cil_tree_node *opt_node = opt->datum.nodes->head->data;
+			struct cil_optional *opt = (struct cil_optional *)optional->data;
+			struct cil_tree_node *opt_node = NODE(opt);;
 			/* disable an optional if something failed to resolve */
 			opt->enabled = CIL_FALSE;
 			cil_tree_log(node, lvl, "Failed to resolve %s statement", cil_node_to_string(node));
@@ -3876,39 +3876,18 @@ int __cil_resolve_ast_first_child_helper(struct cil_tree_node *current, void *ex
 {
 	int rc = SEPOL_ERR;
 	struct cil_args_resolve *args = extra_args;
-	struct cil_tree_node *optstack = NULL;
 	struct cil_tree_node *parent = NULL;
-	struct cil_tree_node *blockstack = NULL;
-	struct cil_tree_node *new = NULL;
 
 	if (current == NULL || extra_args == NULL) {
 		goto exit;
 	}
 
-	optstack = args->optstack;
 	parent = current->parent;
-	blockstack = args->blockstack;
 
-	if (parent->flavor == CIL_OPTIONAL || parent->flavor == CIL_BLOCK) {
-		/* push this node onto a stack */
-		cil_tree_node_init(&new);
-
-		new->data = parent->data;
-		new->flavor = parent->flavor;
-
-		if (parent->flavor == CIL_OPTIONAL) {
-			if (optstack != NULL) {
-				optstack->parent = new;
-				new->cl_head = optstack;
-			}
-			args->optstack = new;
-		} else if (parent->flavor == CIL_BLOCK) {
-			if (blockstack != NULL) {
-				blockstack->parent = new;
-				new->cl_head = blockstack;
-			}
-			args->blockstack = new;
-		}
+	if (parent->flavor == CIL_BLOCK) {
+		args->block = parent;
+	} else if (parent->flavor == CIL_OPTIONAL) {
+		args->optional = parent;
 	} else if (parent->flavor == CIL_BOOLEANIF) {
 		args->boolif = parent;
 	} else if (parent->flavor == CIL_MACRO) {
@@ -3927,7 +3906,6 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
 	int rc = SEPOL_ERR;
 	struct cil_args_resolve *args = extra_args;
 	struct cil_tree_node *parent = NULL;
-	struct cil_tree_node *blockstack = NULL;
 
 	if (current == NULL ||  extra_args == NULL) {
 		goto exit;
@@ -3938,30 +3916,31 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
 	if (parent->flavor == CIL_MACRO) {
 		args->macro = NULL;
 	} else if (parent->flavor == CIL_OPTIONAL) {
-		struct cil_tree_node *optstack;
-
+		struct cil_tree_node *n = parent->parent;
 		if (((struct cil_optional *)parent->data)->enabled == CIL_FALSE) {
 			*(args->changed) = CIL_TRUE;
 			cil_list_append(args->disabled_optionals, CIL_NODE, parent);
 		}
-
-		/* pop off the stack */
-		optstack = args->optstack;
-		args->optstack = optstack->cl_head;
-		if (optstack->cl_head) {
-			optstack->cl_head->parent = NULL;
+		args->optional = NULL;
+		while (n && n->flavor != CIL_ROOT) {
+			if (n->flavor == CIL_OPTIONAL) {
+				args->optional = n;
+				break;
+			}
+			n = n->parent;
 		}
-		free(optstack);
 	} else if (parent->flavor == CIL_BOOLEANIF) {
 		args->boolif = NULL;
 	} else if (parent->flavor == CIL_BLOCK) {
-		/* pop off the stack */
-		blockstack = args->blockstack;
-		args->blockstack = blockstack->cl_head;
-		if (blockstack->cl_head) {
-			blockstack->cl_head->parent = NULL;
+		struct cil_tree_node *n = parent->parent;
+		args->block = NULL;
+		while (n && n->flavor != CIL_ROOT) {
+			if (n->flavor == CIL_BLOCK) {
+				args->block = n;
+				break;
+			}
+			n = n->parent;
 		}
-		free(blockstack);
 	}
 
 	return SEPOL_OK;
@@ -3970,16 +3949,6 @@ exit:
 	return rc;
 }
 
-static void cil_destroy_tree_node_stack(struct cil_tree_node *curr)
-{
-	struct cil_tree_node *next;
-	while (curr != NULL) {
-		next = curr->cl_head;
-		free(curr);
-		curr = next;
-	}
-}
-
 int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
 {
 	int rc = SEPOL_ERR;
@@ -3994,7 +3963,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
 	extra_args.db = db;
 	extra_args.pass = pass;
 	extra_args.changed = &changed;
-	extra_args.optstack = NULL;
+	extra_args.block = NULL;
+	extra_args.optional = NULL;
 	extra_args.boolif= NULL;
 	extra_args.macro = NULL;
 	extra_args.sidorder_lists = NULL;
@@ -4003,7 +3973,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
 	extra_args.catorder_lists = NULL;
 	extra_args.sensitivityorder_lists = NULL;
 	extra_args.in_list = NULL;
-	extra_args.blockstack = NULL;
 
 	cil_list_init(&extra_args.disabled_optionals, CIL_NODE);
 	cil_list_init(&extra_args.sidorder_lists, CIL_LIST_ITEM);
@@ -4107,17 +4076,7 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
 			}
 			cil_list_destroy(&extra_args.disabled_optionals, CIL_FALSE);
 			cil_list_init(&extra_args.disabled_optionals, CIL_NODE);
-		}
-
-		/* reset the arguments */
-		changed = 0;
-		while (extra_args.optstack != NULL) {
-			cil_destroy_tree_node_stack(extra_args.optstack);
-			extra_args.optstack = NULL;
-		}
-		while (extra_args.blockstack!= NULL) {
-			cil_destroy_tree_node_stack(extra_args.blockstack);
-			extra_args.blockstack = NULL;
+			changed = 0;
 		}
 	}
 
@@ -4128,8 +4087,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
 
 	rc = SEPOL_OK;
 exit:
-	cil_destroy_tree_node_stack(extra_args.optstack);
-	cil_destroy_tree_node_stack(extra_args.blockstack);
 	__cil_ordered_lists_destroy(&extra_args.sidorder_lists);
 	__cil_ordered_lists_destroy(&extra_args.classorder_lists);
 	__cil_ordered_lists_destroy(&extra_args.catorder_lists);
-- 
2.26.3

[PATCH 05/12] libsepol/cil: Reorder checks for invalid rules when resolving AST

[James Carter]

Reorder checks for invalid rules in the blocks of tunableifs,
in-statements, macros, and booleanifs when resolving the AST for
consistency.

Order the checks in the same order the blocks will be resolved in,
so tuanbleif, in-statement, macro, booleanif, and then non-block
rules.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_resolve_ast.c | 76 +++++++++++++++---------------
 1 file changed, 39 insertions(+), 37 deletions(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index a61462d0..93fc0d63 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -52,10 +52,10 @@ struct cil_args_resolve {
 	enum cil_pass pass;
 	uint32_t *changed;
 	struct cil_list *disabled_optionals;
+	struct cil_tree_node *block;
+	struct cil_tree_node *macro;
 	struct cil_tree_node *optional;
 	struct cil_tree_node *boolif;
-	struct cil_tree_node *macro;
-	struct cil_tree_node *block;
 	struct cil_list *sidorder_lists;
 	struct cil_list *classorder_lists;
 	struct cil_list *unordered_classorder_lists;
@@ -3777,50 +3777,52 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
 	int rc = SEPOL_ERR;
 	struct cil_args_resolve *args = extra_args;
 	enum cil_pass pass = args->pass;
-	struct cil_tree_node *optional = args->optional;
-	struct cil_tree_node *boolif = args->boolif;
 	struct cil_tree_node *block = args->block;
 	struct cil_tree_node *macro = args->macro;
+	struct cil_tree_node *optional = args->optional;
+	struct cil_tree_node *boolif = args->boolif;
 
 	if (node == NULL) {
 		goto exit;
 	}
 
-	if (optional != NULL) {
-		if (node->flavor == CIL_TUNABLE || 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));
+	if (block != NULL) {
+		if (node->flavor == CIL_CAT ||
+		    node->flavor == CIL_SENS) {
+			cil_tree_log(node, CIL_ERR, "%s statement is not allowed in blocks", cil_node_to_string(node));
 			rc = SEPOL_ERR;
 			goto exit;
 		}
 	}
 
-	if (block != NULL) {
-		if (node->flavor == CIL_CAT || node->flavor == CIL_SENS) {
-			cil_tree_log(node, CIL_ERR, "%s statement is not allowed in blocks", cil_node_to_string(node));
+	if (macro != NULL) {
+		if (node->flavor == CIL_BLOCK ||
+		    node->flavor == CIL_BLOCKINHERIT ||
+		    node->flavor == CIL_BLOCKABSTRACT ||
+		    node->flavor == CIL_MACRO) {
+			cil_tree_log(node, CIL_ERR, "%s statement is not allowed in macros", cil_node_to_string(node));
 			rc = SEPOL_ERR;
 			goto exit;
 		}
 	}
 
-	if (macro != NULL) {
-		if (node->flavor == CIL_BLOCKINHERIT ||
-			node->flavor == CIL_BLOCK ||
-			node->flavor == CIL_BLOCKABSTRACT ||
-			node->flavor == CIL_MACRO) {
-			cil_tree_log(node, CIL_ERR, "%s statement is not allowed in macros", cil_node_to_string(node));
+	if (optional != NULL) {
+		if (node->flavor == CIL_TUNABLE ||
+		    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;
 		}
 	}
 
 	if (boolif != NULL) {
-		if (!(node->flavor == CIL_CONDBLOCK ||
-			node->flavor == CIL_AVRULE ||
-			node->flavor == CIL_TYPE_RULE ||
-			node->flavor == CIL_CALL ||
-			node->flavor == CIL_TUNABLEIF ||
-			node->flavor == CIL_NAMETYPETRANSITION)) {
+		if (!(node->flavor == CIL_TUNABLEIF ||
+		      node->flavor == CIL_CALL ||
+		      node->flavor == CIL_CONDBLOCK ||
+		      node->flavor == CIL_AVRULE ||
+		      node->flavor == CIL_TYPE_RULE ||
+		      node->flavor == CIL_NAMETYPETRANSITION)) {
 			if (((struct cil_booleanif*)boolif->data)->preserved_tunable) {
 				cil_tree_log(node, CIL_ERR, "%s statement is not allowed in booleanifs (tunableif treated as a booleanif)", cil_node_to_string(node));
 			} else {
@@ -3886,12 +3888,12 @@ int __cil_resolve_ast_first_child_helper(struct cil_tree_node *current, void *ex
 
 	if (parent->flavor == CIL_BLOCK) {
 		args->block = parent;
+	} else if (parent->flavor == CIL_MACRO) {
+		args->macro = parent;
 	} else if (parent->flavor == CIL_OPTIONAL) {
 		args->optional = parent;
 	} else if (parent->flavor == CIL_BOOLEANIF) {
 		args->boolif = parent;
-	} else if (parent->flavor == CIL_MACRO) {
-		args->macro = parent;
 	}
 
 	return SEPOL_OK;
@@ -3913,7 +3915,17 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
 
 	parent = current->parent;
 
-	if (parent->flavor == CIL_MACRO) {
+	if (parent->flavor == CIL_BLOCK) {
+		struct cil_tree_node *n = parent->parent;
+		args->block = NULL;
+		while (n && n->flavor != CIL_ROOT) {
+			if (n->flavor == CIL_BLOCK) {
+				args->block = n;
+				break;
+			}
+			n = n->parent;
+		}
+	} else if (parent->flavor == CIL_MACRO) {
 		args->macro = NULL;
 	} else if (parent->flavor == CIL_OPTIONAL) {
 		struct cil_tree_node *n = parent->parent;
@@ -3931,16 +3943,6 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
 		}
 	} else if (parent->flavor == CIL_BOOLEANIF) {
 		args->boolif = NULL;
-	} else if (parent->flavor == CIL_BLOCK) {
-		struct cil_tree_node *n = parent->parent;
-		args->block = NULL;
-		while (n && n->flavor != CIL_ROOT) {
-			if (n->flavor == CIL_BLOCK) {
-				args->block = n;
-				break;
-			}
-			n = n->parent;
-		}
 	}
 
 	return SEPOL_OK;
@@ -3964,9 +3966,9 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
 	extra_args.pass = pass;
 	extra_args.changed = &changed;
 	extra_args.block = NULL;
+	extra_args.macro = NULL;
 	extra_args.optional = NULL;
 	extra_args.boolif= NULL;
-	extra_args.macro = NULL;
 	extra_args.sidorder_lists = NULL;
 	extra_args.classorder_lists = NULL;
 	extra_args.unordered_classorder_lists = NULL;
-- 
2.26.3

[PATCH 06/12] libsepol/cil: Sync checks for invalid rules in booleanifs

[James Carter]

When building the AST, typemember rules in a booleanif block will
be incorrectly called invalid. They are allowed in the kernel
policy and should be allowed in CIL.

When resolving the AST, if a neverallow rule is copied into a
booleanif block, it will not be considered an invalid rule, even
though this is not allowed in the kernel policy.

Update the booleanif checks to allow typemember rules and to not
allow neverallow rules in booleanifs. Also use the same form of
conditional for the checks when building and resolving the AST.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_build_ast.c   |  3 ++-
 libsepol/cil/src/cil_resolve_ast.c | 23 +++++++++++++++--------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index c0783ba6..457d3ee7 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -6158,7 +6158,8 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 			parse_current->data != CIL_KEY_DONTAUDIT &&
 			parse_current->data != CIL_KEY_AUDITALLOW &&
 			parse_current->data != CIL_KEY_TYPETRANSITION &&
-			parse_current->data != CIL_KEY_TYPECHANGE) {
+			parse_current->data != CIL_KEY_TYPECHANGE &&
+			parse_current->data != CIL_KEY_TYPEMEMBER) {
 			rc = SEPOL_ERR;
 			cil_tree_log(parse_current, CIL_ERR, "Found %s", (char*)parse_current->data);
 			if (((struct cil_booleanif*)boolif->data)->preserved_tunable) {
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 93fc0d63..56295a04 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -3774,7 +3774,7 @@ exit:
 
 int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished, void *extra_args)
 {
-	int rc = SEPOL_ERR;
+	int rc = SEPOL_OK;
 	struct cil_args_resolve *args = extra_args;
 	enum cil_pass pass = args->pass;
 	struct cil_tree_node *block = args->block;
@@ -3817,18 +3817,25 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
 	}
 
 	if (boolif != NULL) {
-		if (!(node->flavor == CIL_TUNABLEIF ||
-		      node->flavor == CIL_CALL ||
-		      node->flavor == CIL_CONDBLOCK ||
-		      node->flavor == CIL_AVRULE ||
-		      node->flavor == CIL_TYPE_RULE ||
-		      node->flavor == CIL_NAMETYPETRANSITION)) {
+		if (node->flavor != CIL_TUNABLEIF &&
+			node->flavor != CIL_CALL &&
+			node->flavor != CIL_CONDBLOCK &&
+			node->flavor != CIL_AVRULE &&
+			node->flavor != CIL_TYPE_RULE &&
+			node->flavor != CIL_NAMETYPETRANSITION) {
+			rc = SEPOL_ERR;
+		} else if (node->flavor == CIL_AVRULE) {
+			struct cil_avrule *rule = node->data;
+			if (rule->rule_kind == CIL_AVRULE_NEVERALLOW) {
+				rc = SEPOL_ERR;
+			}
+		}
+		if (rc == SEPOL_ERR) {
 			if (((struct cil_booleanif*)boolif->data)->preserved_tunable) {
 				cil_tree_log(node, CIL_ERR, "%s statement is not allowed in booleanifs (tunableif treated as a booleanif)", cil_node_to_string(node));
 			} else {
 				cil_tree_log(node, CIL_ERR, "%s statement is not allowed in booleanifs", cil_node_to_string(node));
 			}
-			rc = SEPOL_ERR;
 			goto exit;
 		}
 	}
-- 
2.26.3

[PATCH 07/12] libsepol/cil: Check for statements not allowed in optional blocks

[James Carter]

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 457d3ee7..1fef25d4 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;
 };
 
@@ -6099,6 +6100,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;
@@ -6149,6 +6151,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 &&
@@ -6490,6 +6504,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;
 	}
@@ -6520,6 +6538,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;
 	}
@@ -6548,6 +6579,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 56295a04..efff0f2e 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -3808,8 +3808,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.26.3

[PATCH 08/12] libsepol/cil: Sync checks for invalid rules in macros

[James Carter]

When resolving the AST, tunable and in-statements are not considered
to be invalid in macros. This is inconsistent with the checks when
building the AST.

Add checks to make tunable and in-statments invalid in macros when
resolving the AST.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_resolve_ast.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index efff0f2e..7229a3b4 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -3796,7 +3796,9 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
 	}
 
 	if (macro != NULL) {
-		if (node->flavor == CIL_BLOCK ||
+		if (node->flavor == CIL_TUNABLE ||
+			node->flavor == CIL_IN ||
+			node->flavor == CIL_BLOCK ||
 		    node->flavor == CIL_BLOCKINHERIT ||
 		    node->flavor == CIL_BLOCKABSTRACT ||
 		    node->flavor == CIL_MACRO) {
-- 
2.26.3

[PATCH 09/12] libsepol/cil: Do not allow tunable declarations in in-statements

[James Carter]

Since tunableifs are resolved before in-statements, do not allow
tuanble declarations in in-statements.

Since in-statements are the first flavor of statement that causes
part of the AST to be copied to another part, there is no need to
check the in-statements when resolving the AST.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_build_ast.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 1fef25d4..df7bb950 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -6130,7 +6130,8 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 	}
 
 	if (in != NULL) {
-		if (parse_current->data == CIL_KEY_IN) {
+		if (parse_current->data == CIL_KEY_TUNABLE ||
+			parse_current->data == CIL_KEY_IN) {
 			rc = SEPOL_ERR;
 			cil_tree_log(parse_current, CIL_ERR, "Found in-statement");
 			cil_log(CIL_ERR, "in-statements cannot be defined within in-statements\n");
-- 
2.26.3

Re: [PATCH 00/12] Update checks for invalid rules in blocks

[James Carter]

On Tue, Mar 30, 2021 at 1:39 PM James Carter <jwcart2@gmail.com> wrote:
>
> Looking into a bug that OSS-Fuzz found led to patch 7, Check for
> statements not allowed in optional blocks, which is the most important
> patch in this series. Working on patch 7 led to fixing some other
> problems with the checks for invalid rules, cleaning up some of the code,
> and improving the CIL documentation.
>
> Patches 1, 2, 4, 5, and 10 are doing various cleanups.
> Patch 3 fixes a bug that prevents the first rule in a block from being checked.
> Patches 6, 7, 8, and 9 update the checks for invalid rules.
> Patch 11 fixes a bug that prevented some error messages from being displayed.
> Patch 12 updates the CIL documentation.
>
> There is still work to do in this area. I am not sure why sensitivity and
> category statements are not allowed in blocks, but everything else is. That
> is why I didn't add those checks when building the AST. It is not clear if
> mls, handleunknown, defaultuser, defaultrole, defaulttype, defaultrange,
> and policycap should be restricted to the global namespace.
>
> James Carter (12):
>   libsepol/cil: Reorder checks for invalid rules when building AST
>   libsepol/cil: Cleanup build AST helper functions
>   libsepol/cil: Create new first child helper function for building AST
>   libsepol/cil: Use AST to track blocks and optionals when resolving
>   libsepol/cil: Reorder checks for invalid rules when resolving AST
>   libsepol/cil: Sync checks for invalid rules in booleanifs
>   libsepol/cil: Check for statements not allowed in optional blocks
>   libsepol/cil: Sync checks for invalid rules in macros
>   libsepol/cil: Do not allow tunable declarations in in-statements
>   libsepol/cil: Make invalid statement error messages consistent
>   libsepol/cil: Use CIL_ERR for error messages in cil_compile()
>   secilc/docs: Update the CIL documentation for various blocks
>
>  libsepol/cil/src/cil.c                    |   8 +-
>  libsepol/cil/src/cil_build_ast.c          | 193 ++++++++++++----------
>  libsepol/cil/src/cil_resolve_ast.c        | 174 ++++++++-----------
>  secilc/docs/cil_call_macro_statements.md  |   2 +
>  secilc/docs/cil_conditional_statements.md |   6 +
>  secilc/docs/cil_container_statements.md   |  28 ++--
>  6 files changed, 205 insertions(+), 206 deletions(-)
>
> --
> 2.26.3
>

There hasn't been any comments on this patch set. I am planning on
merging it next week.

Jim

Re: [PATCH 00/12] Update checks for invalid rules in blocks

[James Carter]

On Tue, Mar 30, 2021 at 1:39 PM James Carter <jwcart2@gmail.com> wrote:
>
> Looking into a bug that OSS-Fuzz found led to patch 7, Check for
> statements not allowed in optional blocks, which is the most important
> patch in this series. Working on patch 7 led to fixing some other
> problems with the checks for invalid rules, cleaning up some of the code,
> and improving the CIL documentation.
>
> Patches 1, 2, 4, 5, and 10 are doing various cleanups.
> Patch 3 fixes a bug that prevents the first rule in a block from being checked.
> Patches 6, 7, 8, and 9 update the checks for invalid rules.
> Patch 11 fixes a bug that prevented some error messages from being displayed.
> Patch 12 updates the CIL documentation.
>
> There is still work to do in this area. I am not sure why sensitivity and
> category statements are not allowed in blocks, but everything else is. That
> is why I didn't add those checks when building the AST. It is not clear if
> mls, handleunknown, defaultuser, defaultrole, defaulttype, defaultrange,
> and policycap should be restricted to the global namespace.
>
> James Carter (12):
>   libsepol/cil: Reorder checks for invalid rules when building AST
>   libsepol/cil: Cleanup build AST helper functions
>   libsepol/cil: Create new first child helper function for building AST
>   libsepol/cil: Use AST to track blocks and optionals when resolving
>   libsepol/cil: Reorder checks for invalid rules when resolving AST
>   libsepol/cil: Sync checks for invalid rules in booleanifs
>   libsepol/cil: Check for statements not allowed in optional blocks
>   libsepol/cil: Sync checks for invalid rules in macros
>   libsepol/cil: Do not allow tunable declarations in in-statements
>   libsepol/cil: Make invalid statement error messages consistent
>   libsepol/cil: Use CIL_ERR for error messages in cil_compile()
>   secilc/docs: Update the CIL documentation for various blocks
>
>  libsepol/cil/src/cil.c                    |   8 +-
>  libsepol/cil/src/cil_build_ast.c          | 193 ++++++++++++----------
>  libsepol/cil/src/cil_resolve_ast.c        | 174 ++++++++-----------
>  secilc/docs/cil_call_macro_statements.md  |   2 +
>  secilc/docs/cil_conditional_statements.md |   6 +
>  secilc/docs/cil_container_statements.md   |  28 ++--
>  6 files changed, 205 insertions(+), 206 deletions(-)
>
> --
> 2.26.3
>

This has been applied (with the whitespace error in the last patch fixed).
Jim

Re: [PATCH 00/12] Update checks for invalid rules in blocks

[Nicolas Iooss]

On Mon, Apr 19, 2021 at 8:26 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, Mar 30, 2021 at 1:39 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > Looking into a bug that OSS-Fuzz found led to patch 7, Check for
> > statements not allowed in optional blocks, which is the most important
> > patch in this series. Working on patch 7 led to fixing some other
> > problems with the checks for invalid rules, cleaning up some of the code,
> > and improving the CIL documentation.
> >
> > Patches 1, 2, 4, 5, and 10 are doing various cleanups.
> > Patch 3 fixes a bug that prevents the first rule in a block from being checked.
> > Patches 6, 7, 8, and 9 update the checks for invalid rules.
> > Patch 11 fixes a bug that prevented some error messages from being displayed.
> > Patch 12 updates the CIL documentation.
> >
> > There is still work to do in this area. I am not sure why sensitivity and
> > category statements are not allowed in blocks, but everything else is. That
> > is why I didn't add those checks when building the AST. It is not clear if
> > mls, handleunknown, defaultuser, defaultrole, defaulttype, defaultrange,
> > and policycap should be restricted to the global namespace.
> >
> > James Carter (12):
> >   libsepol/cil: Reorder checks for invalid rules when building AST
> >   libsepol/cil: Cleanup build AST helper functions
> >   libsepol/cil: Create new first child helper function for building AST
> >   libsepol/cil: Use AST to track blocks and optionals when resolving
> >   libsepol/cil: Reorder checks for invalid rules when resolving AST
> >   libsepol/cil: Sync checks for invalid rules in booleanifs
> >   libsepol/cil: Check for statements not allowed in optional blocks
> >   libsepol/cil: Sync checks for invalid rules in macros
> >   libsepol/cil: Do not allow tunable declarations in in-statements
> >   libsepol/cil: Make invalid statement error messages consistent
> >   libsepol/cil: Use CIL_ERR for error messages in cil_compile()
> >   secilc/docs: Update the CIL documentation for various blocks
> >
> >  libsepol/cil/src/cil.c                    |   8 +-
> >  libsepol/cil/src/cil_build_ast.c          | 193 ++++++++++++----------
> >  libsepol/cil/src/cil_resolve_ast.c        | 174 ++++++++-----------
> >  secilc/docs/cil_call_macro_statements.md  |   2 +
> >  secilc/docs/cil_conditional_statements.md |   6 +
> >  secilc/docs/cil_container_statements.md   |  28 ++--
> >  6 files changed, 205 insertions(+), 206 deletions(-)
> >
> > --
> > 2.26.3
> >
>
> This has been applied (with the whitespace error in the last patch fixed).
> Jim

Hi,
Thanks for these patches! I was on holiday in the past few weeks and
just got back. The applied patches look good to me. Moreover when I
run secilc on all the OSSFuzz reproducers I have studied so far, there
is no longer any crash :)

Nicolas