[PATCH] libsepol/cil: Allow some duplicate macro and block declarations

[PATCH] libsepol/cil: Allow some duplicate macro and block declarations

[James Carter]

The commit d155b410d4bbc90d28f361b966f0429598da8188 (libsepol/cil:
Check for duplicate blocks, optionals, and macros) added checks when
copying blocks, macros, and optionals so that a duplicate would cause
an exit with an error. Unfortunately, some policies exist that depend
on this behavior when using inheritance.

The behavior is as follows.

For macros only the first declared macro matters.
;
(macro m ((type ARG1))
  (allow ARG1 self (CLASS (PERM1)))
)
(block b
  (macro m ((type ARG1))
    (allow ARG1 self (CLASS (PERM2)))
  )
)
(blockinherit b)
(type t)
(call m (t))
;
For this policy segment, the macro m in block b will not be called.
Only the original macro m will be.

This behavior has been used to override macros that are going to
be inherited. Only the inherited macros that have not already been
declared in the destination namespace will be used.

Blocks seem to work fine even though there are two of them
;
(block b1
  (blockinherit b2)
  (block b
    (type t1)
    (allow t1 self (CLASS (PERM)))
  )
)
(block b2
  (block b
    (type t2)
    (allow t2 self (CLASS (PERM)))
  )
)
(blockinherit b1)
;
In this example, the blockinherit of b2 will cause there to be
two block b's in block b1. Note that if both block b's tried to
declare the same type, then that would be an error. The blockinherit
of b1 will copy both block b's.

This behavior has been used to allow the use of in-statements for
a block that is being inherited. Since the in-statements are resolved
before block inheritance, this only works if a block with the same
name as the block to be inherited is declared in the namespace.

To support the use of these two behaviors, allow duplicate blocks
and macros when they occur as the result of block inheritance. In
any other circumstances and error for a redeclaration will be given.

Since the duplicate macro is not going to be used it is just skipped.
The duplicate block will use the datum of the original block. In both
cases a warning message will be produced (it will only be seen if
"-v" is used when compiling the policy).

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

diff --git a/libsepol/cil/src/cil_copy_ast.c b/libsepol/cil/src/cil_copy_ast.c
index 9c0231f2..67be2ec8 100644
--- a/libsepol/cil/src/cil_copy_ast.c
+++ b/libsepol/cil/src/cil_copy_ast.c
@@ -43,6 +43,7 @@
 #include "cil_verify.h"
 
 struct cil_args_copy {
+	struct cil_tree_node *orig_dest;
 	struct cil_tree_node *dest;
 	struct cil_db *db;
 };
@@ -101,17 +102,23 @@ int cil_copy_block(__attribute__((unused)) struct cil_db *db, void *data, void *
 	struct cil_block *orig = data;
 	char *key = orig->datum.name;
 	struct cil_symtab_datum *datum = NULL;
-	struct cil_block *new;
 
 	cil_symtab_get_datum(symtab, key, &datum);
 	if (datum != NULL) {
-		cil_tree_log(NODE(datum), CIL_ERR, "Re-declaration of %s %s", cil_node_to_string(NODE(datum)), key);
-		return SEPOL_ERR;
+		if (FLAVOR(datum) != CIL_BLOCK) {
+			cil_tree_log(NODE(orig), CIL_ERR, "Block %s being copied", key);
+			cil_tree_log(NODE(datum), CIL_ERR, "  Conflicts with %s already declared", cil_node_to_string(NODE(datum)));
+			return SEPOL_ERR;
+		}
+		cil_tree_log(NODE(orig), CIL_WARN, "Block %s being copied", key);
+		cil_tree_log(NODE(datum), CIL_WARN, "  Previously declared", key);
+		*copy = datum;
+	} else {
+		struct cil_block *new;
+		cil_block_init(&new);
+		*copy = new;
 	}
 
-	cil_block_init(&new);
-	*copy = new;
-
 	return SEPOL_OK;
 }
 
@@ -1511,21 +1518,26 @@ int cil_copy_macro(__attribute__((unused)) struct cil_db *db, void *data, void *
 	struct cil_macro *orig = data;
 	char *key = orig->datum.name;
 	struct cil_symtab_datum *datum = NULL;
-	struct cil_macro *new;
 
 	cil_symtab_get_datum(symtab, key, &datum);
 	if (datum != NULL) {
-		cil_tree_log(NODE(datum), CIL_ERR, "Re-declaration of %s %s", cil_node_to_string(NODE(datum)), key);
-		return SEPOL_ERR;
-	}
-
-	cil_macro_init(&new);
-	if (orig->params != NULL) {
-		cil_copy_list(orig->params, &new->params);
+		if (FLAVOR(datum) != CIL_MACRO) {
+			cil_tree_log(NODE(orig), CIL_ERR, "Macro %s being copied", key);
+			cil_tree_log(NODE(datum), CIL_ERR, "  Conflicts with %s already declared", cil_node_to_string(NODE(datum)));
+			return SEPOL_ERR;
+		}
+		cil_tree_log(NODE(orig), CIL_WARN, "Skipping macro %s", key);
+		cil_tree_log(NODE(datum), CIL_WARN, "  Previously declared");
+		*copy = NULL;
+	} else {
+		struct cil_macro *new;
+		cil_macro_init(&new);
+		if (orig->params != NULL) {
+			cil_copy_list(orig->params, &new->params);
+		}
+		*copy = new;
 	}
 
-	*copy = new;
-
 	return SEPOL_OK;
 }
 
@@ -1700,7 +1712,7 @@ int cil_copy_src_info(__attribute__((unused)) struct cil_db *db, void *data, voi
 	return SEPOL_OK;
 }
 
-int __cil_copy_node_helper(struct cil_tree_node *orig, __attribute__((unused)) uint32_t *finished, void *extra_args)
+int __cil_copy_node_helper(struct cil_tree_node *orig, uint32_t *finished, void *extra_args)
 {
 	int rc = SEPOL_ERR;
 	struct cil_tree_node *parent = NULL;
@@ -2005,6 +2017,16 @@ int __cil_copy_node_helper(struct cil_tree_node *orig, __attribute__((unused)) u
 
 	rc = (*copy_func)(db, orig->data, &data, symtab);
 	if (rc == SEPOL_OK) {
+		if (orig->flavor == CIL_MACRO && data == NULL) {
+			/* Skipping macro re-declaration */
+			if (args->orig_dest->flavor != CIL_BLOCKINHERIT) {
+				cil_log(CIL_ERR, "  Re-declaration of macro is only allowed when inheriting a block\n");
+				return SEPOL_ERR;
+			}
+			*finished = CIL_TREE_SKIP_HEAD;
+			return SEPOL_OK;
+		}
+
 		cil_tree_node_init(&new);
 
 		new->parent = parent;
@@ -2013,7 +2035,15 @@ int __cil_copy_node_helper(struct cil_tree_node *orig, __attribute__((unused)) u
 		new->flavor = orig->flavor;
 		new->data = data;
 
-		if (orig->flavor >= CIL_MIN_DECLARATIVE) {
+		if (orig->flavor == CIL_BLOCK && DATUM(data)->nodes->head != NULL) {
+			/* Duplicate block */
+			if (args->orig_dest->flavor != CIL_BLOCKINHERIT) {
+				cil_log(CIL_ERR, "  Re-declaration of block is only allowed when inheriting a block\n");
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			cil_list_append(DATUM(new->data)->nodes, CIL_NODE, new);
+		} else if (orig->flavor >= CIL_MIN_DECLARATIVE) {
 			/* Check the flavor of data if was found in the destination symtab */
 			if (DATUM(data)->nodes->head && FLAVOR(data) != orig->flavor) {
 				cil_tree_log(orig, CIL_ERR, "Incompatible flavor when trying to copy %s", DATUM(data)->name);
@@ -2098,12 +2128,13 @@ int cil_copy_ast(struct cil_db *db, struct cil_tree_node *orig, struct cil_tree_
 	int rc = SEPOL_ERR;
 	struct cil_args_copy extra_args;
 
+	extra_args.orig_dest = dest;
 	extra_args.dest = dest;
 	extra_args.db = db;
 
 	rc = cil_tree_walk(orig, __cil_copy_node_helper, NULL,  __cil_copy_last_child_helper, &extra_args);
 	if (rc != SEPOL_OK) {
-		cil_log(CIL_INFO, "cil_tree_walk failed, rc: %d\n", rc);
+		cil_tree_log(dest, CIL_ERR, "Failed to copy %s to %s", cil_node_to_string(orig), cil_node_to_string(dest));
 		goto exit;
 	}
 
-- 
2.31.1

[PATCH 1/2] libsepol/cil: Improve in-statement to allow use after inheritance

[James Carter]

CIL's in-statement is resolved before block inheritance. This has
the advantage of allowing an in-statement to add rules to a base
block (say for a new permission) and having those rules also be
added everywhere that base block is inherited. But the disadvantage
of this behavior is that it is not possible to use an in-statement
on a block that is inherited for the simple reason that that block
does not exist when the in-statment is resolved.

Change the syntax of the in-statement to allow specifying whether
the rules should be added before or after inheritance. If neither
is specified, then the behavior remains the same. All current
in-statements will work as before.

Either the old syntax
  (in container_id
      cil_statement
      ...
  )
or the new syntax
  (in before|after container_id
      cil_statement
      ...
  )
may be used for in-statements. But only "(in after ..." will have
the new behavior. Using "(in before ..." will give the same
behavior as before.

Macro Example
;
(block b1
  (macro m1 ((type ARG1))
    (allow ARG1 self (C1 (P1a)))
  )
)
(in after b1.m1
  (allow ARG1 self (C1 (P1c)))
)
(type t1a)
(call b1.m1 (t1a))
(blockinherit b1)
(in after m1
  (allow ARG1 self (C1 (P1b)))
)
(type t1b)
(call m1 (t1b))
;
This results in the following rules:
  (allow t1a self (C1 (P1a)))
  (allow t1a self (C1 (P1c)))
  (allow t1b self (C1 (P1a)))
  (allow t1b self (C1 (P1b)))

Block Example
;
(block b2
  (block b
    (type ta)
    (allow ta self (C2 (P2a)))
  )
)
(in before b2.b
  (type tb)
  (allow tb self (C2 (P2b)))
)
(block c2
  (blockinherit b2)
  (in after b
    (type tc)
    (allow tc self (C2 (P2c)))
  )
)
;
This results in the following rules:
  (allow b2.b.ta self (C2 (P2a)))
  (allow b2.b.tb self (C2 (P2b)))
  (allow c2.b.ta self (C2 (P2a)))
  (allow c2.b.tb self (C2 (P2b)))
  (allow c2.b.tc self (C2 (P2c)))

Using in-statements on optionals also works as expected.

One additional change is that blockabstract and blockinherit rules
are not allowed when using an after in-statement. This is because
both of those are resolved before an after in-statement would be
resolved.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil.c             |  5 +++
 libsepol/cil/src/cil_build_ast.c   | 31 +++++++++++++++++--
 libsepol/cil/src/cil_internal.h    |  6 +++-
 libsepol/cil/src/cil_resolve_ast.c | 49 +++++++++++++++++++++---------
 4 files changed, 72 insertions(+), 19 deletions(-)

diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
index d24c81c8..672342b5 100644
--- a/libsepol/cil/src/cil.c
+++ b/libsepol/cil/src/cil.c
@@ -142,6 +142,8 @@ char *CIL_KEY_HANDLEUNKNOWN_DENY;
 char *CIL_KEY_HANDLEUNKNOWN_REJECT;
 char *CIL_KEY_MACRO;
 char *CIL_KEY_IN;
+char *CIL_KEY_IN_BEFORE;
+char *CIL_KEY_IN_AFTER;
 char *CIL_KEY_MLS;
 char *CIL_KEY_DEFAULTRANGE;
 char *CIL_KEY_BLOCKINHERIT;
@@ -353,6 +355,8 @@ static void cil_init_keys(void)
 	CIL_KEY_DEFAULTTYPE = cil_strpool_add("defaulttype");
 	CIL_KEY_MACRO = cil_strpool_add("macro");
 	CIL_KEY_IN = cil_strpool_add("in");
+	CIL_KEY_IN_BEFORE = cil_strpool_add("before");
+	CIL_KEY_IN_AFTER = cil_strpool_add("after");
 	CIL_KEY_MLS = cil_strpool_add("mls");
 	CIL_KEY_DEFAULTRANGE = cil_strpool_add("defaultrange");
 	CIL_KEY_GLOB = cil_strpool_add("*");
@@ -2121,6 +2125,7 @@ void cil_in_init(struct cil_in **in)
 	*in = cil_malloc(sizeof(**in));
 
 	cil_symtab_array_init((*in)->symtab, cil_sym_sizes[CIL_SYM_ARRAY_IN]);
+	(*in)->is_after = CIL_FALSE;
 	(*in)->block_str = NULL;
 }
 
diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 9da90883..4a87e212 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -380,7 +380,8 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
 	enum cil_syntax syntax[] = {
 		CIL_SYN_STRING,
 		CIL_SYN_STRING,
-		CIL_SYN_N_LISTS,
+		CIL_SYN_STRING | CIL_SYN_N_LISTS,
+		CIL_SYN_N_LISTS | CIL_SYN_END,
 		CIL_SYN_END
 	};
 	int syntax_len = sizeof(syntax)/sizeof(*syntax);
@@ -403,14 +404,29 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
 
 	cil_in_init(&in);
 
-	in->block_str = parse_current->next->data;
+	if (parse_current->next->next->data) {
+		char *is_after_str = parse_current->next->data;
+		if (is_after_str == CIL_KEY_IN_BEFORE) {
+			in->is_after = CIL_FALSE;
+		} else if (is_after_str == CIL_KEY_IN_AFTER) {
+			in->is_after = CIL_TRUE;
+		} else {
+			cil_log(CIL_ERR, "Value must be either \'before\' or \'after\'\n");
+			rc = SEPOL_ERR;
+			goto exit;
+		}
+		in->block_str = parse_current->next->next->data;
+	} else {
+		in->is_after = CIL_FALSE;
+		in->block_str = parse_current->next->data;
+	}
 
 	ast_node->data = in;
 	ast_node->flavor = CIL_IN;
 
 	return SEPOL_OK;
 exit:
-	cil_tree_log(parse_current, CIL_ERR, "Bad in statement");
+	cil_tree_log(parse_current, CIL_ERR, "Bad in-statement");
 	cil_destroy_in(in);
 	return rc;
 }
@@ -6136,12 +6152,21 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
 	}
 
 	if (in != NULL) {
+		struct cil_in *in_block = in->data;
 		if (parse_current->data == CIL_KEY_TUNABLE ||
 			parse_current->data == CIL_KEY_IN) {
 			rc = SEPOL_ERR;
 			cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in in-statement", (char *)parse_current->data);
 			goto exit;
 		}
+		if (in_block->is_after == CIL_TRUE) {
+			if (parse_current->data == CIL_KEY_BLOCKINHERIT ||
+				parse_current->data == CIL_KEY_BLOCKABSTRACT) {
+				rc = SEPOL_ERR;
+				cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in an after in-statement", (char *)parse_current->data);
+				goto exit;
+			}
+		}
 	}
 
 	if (macro != NULL) {
diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
index 98e303d1..d33c66bc 100644
--- a/libsepol/cil/src/cil_internal.h
+++ b/libsepol/cil/src/cil_internal.h
@@ -56,10 +56,11 @@ enum cil_pass {
 	CIL_PASS_INIT = 0,
 
 	CIL_PASS_TIF,
-	CIL_PASS_IN,
+	CIL_PASS_IN_BEFORE,
 	CIL_PASS_BLKIN_LINK,
 	CIL_PASS_BLKIN_COPY,
 	CIL_PASS_BLKABS,
+	CIL_PASS_IN_AFTER,
 	CIL_PASS_CALL1,
 	CIL_PASS_CALL2,
 	CIL_PASS_ALIAS1,
@@ -158,6 +159,8 @@ extern char *CIL_KEY_HANDLEUNKNOWN_DENY;
 extern char *CIL_KEY_HANDLEUNKNOWN_REJECT;
 extern char *CIL_KEY_MACRO;
 extern char *CIL_KEY_IN;
+extern char *CIL_KEY_IN_BEFORE;
+extern char *CIL_KEY_IN_AFTER;
 extern char *CIL_KEY_MLS;
 extern char *CIL_KEY_DEFAULTRANGE;
 extern char *CIL_KEY_BLOCKINHERIT;
@@ -355,6 +358,7 @@ struct cil_blockabstract {
 
 struct cil_in {
 	symtab_t symtab[CIL_SYM_NUM];
+	int is_after;
 	char *block_str;
 };
 
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 18007324..77e0d402 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -62,7 +62,8 @@ struct cil_args_resolve {
 	struct cil_list *unordered_classorder_lists;
 	struct cil_list *catorder_lists;
 	struct cil_list *sensitivityorder_lists;
-	struct cil_list *in_list;
+	struct cil_list *in_list_before;
+	struct cil_list *in_list_after;
 	struct cil_stack *disabled_optionals;
 };
 
@@ -2449,10 +2450,8 @@ exit:
 	return rc;
 }
 
-int cil_resolve_in_list(void *extra_args)
+int cil_resolve_in_list(struct cil_list *in_list, void *extra_args)
 {
-	struct cil_args_resolve *args = extra_args;
-	struct cil_list *ins = args->in_list;
 	struct cil_list_item *curr = NULL;
 	struct cil_tree_node *node = NULL;
 	struct cil_tree_node *last_failed_node = NULL;
@@ -2466,7 +2465,7 @@ int cil_resolve_in_list(void *extra_args)
 		resolved = 0;
 		unresolved = 0;
 
-		cil_list_for_each(curr, ins) {
+		cil_list_for_each(curr, in_list) {
 			if (curr->flavor != CIL_NODE) {
 				continue;
 			}
@@ -3590,12 +3589,10 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
 	int rc = SEPOL_OK;
 	struct cil_args_resolve *args = extra_args;
 	enum cil_pass pass = 0;
-	struct cil_list *ins;
 
 	if (node == NULL || args == NULL) {
 		goto exit;
 	}
-	ins = args->in_list;
 
 	pass = args->pass;
 	switch (pass) {
@@ -3604,11 +3601,14 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
 			rc = cil_resolve_tunif(node, args);
 		}
 		break;
-	case CIL_PASS_IN:
+	case CIL_PASS_IN_BEFORE:
 		if (node->flavor == CIL_IN) {
 			// due to ordering issues, in statements are just gathered here and
 			// resolved together in cil_resolve_in_list once all are found
-			cil_list_prepend(ins, CIL_NODE, node);
+			struct cil_in *in = node->data;
+			if (in->is_after == CIL_FALSE) {
+				cil_list_prepend(args->in_list_before, CIL_NODE, node);
+			}
 		}
 		break;
 	case CIL_PASS_BLKIN_LINK:
@@ -3626,6 +3626,16 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
 			rc = cil_resolve_blockabstract(node, args);
 		}
 		break;
+	case CIL_PASS_IN_AFTER:
+		if (node->flavor == CIL_IN) {
+			// due to ordering issues, in statements are just gathered here and
+			// resolved together in cil_resolve_in_list once all are found
+			struct cil_in *in = node->data;
+			if (in->is_after == CIL_TRUE) {
+				cil_list_prepend(args->in_list_after, CIL_NODE, node);
+			}
+		}
+		break;
 	case CIL_PASS_CALL1:
 		if (node->flavor == CIL_CALL && args->macro == NULL) {
 			rc = cil_resolve_call(node, args);
@@ -4073,7 +4083,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
 	extra_args.unordered_classorder_lists = NULL;
 	extra_args.catorder_lists = NULL;
 	extra_args.sensitivityorder_lists = NULL;
-	extra_args.in_list = NULL;
+	extra_args.in_list_before = NULL;
+	extra_args.in_list_after = NULL;
 	extra_args.disabled_optionals = NULL;
 
 	cil_list_init(&extra_args.to_destroy, CIL_NODE);
@@ -4082,7 +4093,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
 	cil_list_init(&extra_args.unordered_classorder_lists, CIL_LIST_ITEM);
 	cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
 	cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
-	cil_list_init(&extra_args.in_list, CIL_IN);
+	cil_list_init(&extra_args.in_list_before, CIL_IN);
+	cil_list_init(&extra_args.in_list_after, CIL_IN);
 	cil_stack_init(&extra_args.disabled_optionals);
 
 	for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
@@ -4093,12 +4105,18 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
 			goto exit;
 		}
 
-		if (pass == CIL_PASS_IN) {
-			rc = cil_resolve_in_list(&extra_args);
+		if (pass == CIL_PASS_IN_BEFORE) {
+			rc = cil_resolve_in_list(extra_args.in_list_before, &extra_args);
+			if (rc != SEPOL_OK) {
+				goto exit;
+			}
+			cil_list_destroy(&extra_args.in_list_before, CIL_FALSE);
+		} else if (pass == CIL_PASS_IN_AFTER) {
+			rc = cil_resolve_in_list(extra_args.in_list_after, &extra_args);
 			if (rc != SEPOL_OK) {
 				goto exit;
 			}
-			cil_list_destroy(&extra_args.in_list, CIL_FALSE);
+			cil_list_destroy(&extra_args.in_list_after, CIL_FALSE);
 		}
 
 		if (pass == CIL_PASS_BLKIN_LINK) {
@@ -4217,7 +4235,8 @@ exit:
 	__cil_ordered_lists_destroy(&extra_args.sensitivityorder_lists);
 	__cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
 	cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
-	cil_list_destroy(&extra_args.in_list, CIL_FALSE);
+	cil_list_destroy(&extra_args.in_list_before, CIL_FALSE);
+	cil_list_destroy(&extra_args.in_list_after, CIL_FALSE);
 	cil_stack_destroy(&extra_args.disabled_optionals);
 
 	return rc;
-- 
2.31.1

Re: [PATCH 1/2] libsepol/cil: Improve in-statement to allow use after inheritance

[Dominick Grift]

James Carter <jwcart2@gmail.com> writes:

> CIL's in-statement is resolved before block inheritance. This has
> the advantage of allowing an in-statement to add rules to a base
> block (say for a new permission) and having those rules also be
> added everywhere that base block is inherited. But the disadvantage
> of this behavior is that it is not possible to use an in-statement
> on a block that is inherited for the simple reason that that block
> does not exist when the in-statment is resolved.
>
> Change the syntax of the in-statement to allow specifying whether
> the rules should be added before or after inheritance. If neither
> is specified, then the behavior remains the same. All current
> in-statements will work as before.

Nice, thanks. I tested it here and it works as advertised.

>
> Either the old syntax
>   (in container_id
>       cil_statement
>       ...
>   )
> or the new syntax
>   (in before|after container_id
>       cil_statement
>       ...
>   )
> may be used for in-statements. But only "(in after ..." will have
> the new behavior. Using "(in before ..." will give the same
> behavior as before.
>
> Macro Example
> ;
> (block b1
>   (macro m1 ((type ARG1))
>     (allow ARG1 self (C1 (P1a)))
>   )
> )
> (in after b1.m1
>   (allow ARG1 self (C1 (P1c)))
> )
> (type t1a)
> (call b1.m1 (t1a))
> (blockinherit b1)
> (in after m1
>   (allow ARG1 self (C1 (P1b)))
> )
> (type t1b)
> (call m1 (t1b))
> ;
> This results in the following rules:
>   (allow t1a self (C1 (P1a)))
>   (allow t1a self (C1 (P1c)))
>   (allow t1b self (C1 (P1a)))
>   (allow t1b self (C1 (P1b)))
>
> Block Example
> ;
> (block b2
>   (block b
>     (type ta)
>     (allow ta self (C2 (P2a)))
>   )
> )
> (in before b2.b
>   (type tb)
>   (allow tb self (C2 (P2b)))
> )
> (block c2
>   (blockinherit b2)
>   (in after b
>     (type tc)
>     (allow tc self (C2 (P2c)))
>   )
> )
> ;
> This results in the following rules:
>   (allow b2.b.ta self (C2 (P2a)))
>   (allow b2.b.tb self (C2 (P2b)))
>   (allow c2.b.ta self (C2 (P2a)))
>   (allow c2.b.tb self (C2 (P2b)))
>   (allow c2.b.tc self (C2 (P2c)))
>
> Using in-statements on optionals also works as expected.
>
> One additional change is that blockabstract and blockinherit rules
> are not allowed when using an after in-statement. This is because
> both of those are resolved before an after in-statement would be
> resolved.
>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/src/cil.c             |  5 +++
>  libsepol/cil/src/cil_build_ast.c   | 31 +++++++++++++++++--
>  libsepol/cil/src/cil_internal.h    |  6 +++-
>  libsepol/cil/src/cil_resolve_ast.c | 49 +++++++++++++++++++++---------
>  4 files changed, 72 insertions(+), 19 deletions(-)
>
> diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
> index d24c81c8..672342b5 100644
> --- a/libsepol/cil/src/cil.c
> +++ b/libsepol/cil/src/cil.c
> @@ -142,6 +142,8 @@ char *CIL_KEY_HANDLEUNKNOWN_DENY;
>  char *CIL_KEY_HANDLEUNKNOWN_REJECT;
>  char *CIL_KEY_MACRO;
>  char *CIL_KEY_IN;
> +char *CIL_KEY_IN_BEFORE;
> +char *CIL_KEY_IN_AFTER;
>  char *CIL_KEY_MLS;
>  char *CIL_KEY_DEFAULTRANGE;
>  char *CIL_KEY_BLOCKINHERIT;
> @@ -353,6 +355,8 @@ static void cil_init_keys(void)
>  	CIL_KEY_DEFAULTTYPE = cil_strpool_add("defaulttype");
>  	CIL_KEY_MACRO = cil_strpool_add("macro");
>  	CIL_KEY_IN = cil_strpool_add("in");
> +	CIL_KEY_IN_BEFORE = cil_strpool_add("before");
> +	CIL_KEY_IN_AFTER = cil_strpool_add("after");
>  	CIL_KEY_MLS = cil_strpool_add("mls");
>  	CIL_KEY_DEFAULTRANGE = cil_strpool_add("defaultrange");
>  	CIL_KEY_GLOB = cil_strpool_add("*");
> @@ -2121,6 +2125,7 @@ void cil_in_init(struct cil_in **in)
>  	*in = cil_malloc(sizeof(**in));
>  
>  	cil_symtab_array_init((*in)->symtab, cil_sym_sizes[CIL_SYM_ARRAY_IN]);
> +	(*in)->is_after = CIL_FALSE;
>  	(*in)->block_str = NULL;
>  }
>  
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index 9da90883..4a87e212 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -380,7 +380,8 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
>  	enum cil_syntax syntax[] = {
>  		CIL_SYN_STRING,
>  		CIL_SYN_STRING,
> -		CIL_SYN_N_LISTS,
> +		CIL_SYN_STRING | CIL_SYN_N_LISTS,
> +		CIL_SYN_N_LISTS | CIL_SYN_END,
>  		CIL_SYN_END
>  	};
>  	int syntax_len = sizeof(syntax)/sizeof(*syntax);
> @@ -403,14 +404,29 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
>  
>  	cil_in_init(&in);
>  
> -	in->block_str = parse_current->next->data;
> +	if (parse_current->next->next->data) {
> +		char *is_after_str = parse_current->next->data;
> +		if (is_after_str == CIL_KEY_IN_BEFORE) {
> +			in->is_after = CIL_FALSE;
> +		} else if (is_after_str == CIL_KEY_IN_AFTER) {
> +			in->is_after = CIL_TRUE;
> +		} else {
> +			cil_log(CIL_ERR, "Value must be either \'before\' or \'after\'\n");
> +			rc = SEPOL_ERR;
> +			goto exit;
> +		}
> +		in->block_str = parse_current->next->next->data;
> +	} else {
> +		in->is_after = CIL_FALSE;
> +		in->block_str = parse_current->next->data;
> +	}
>  
>  	ast_node->data = in;
>  	ast_node->flavor = CIL_IN;
>  
>  	return SEPOL_OK;
>  exit:
> -	cil_tree_log(parse_current, CIL_ERR, "Bad in statement");
> +	cil_tree_log(parse_current, CIL_ERR, "Bad in-statement");
>  	cil_destroy_in(in);
>  	return rc;
>  }
> @@ -6136,12 +6152,21 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
>  	}
>  
>  	if (in != NULL) {
> +		struct cil_in *in_block = in->data;
>  		if (parse_current->data == CIL_KEY_TUNABLE ||
>  			parse_current->data == CIL_KEY_IN) {
>  			rc = SEPOL_ERR;
>  			cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in in-statement", (char *)parse_current->data);
>  			goto exit;
>  		}
> +		if (in_block->is_after == CIL_TRUE) {
> +			if (parse_current->data == CIL_KEY_BLOCKINHERIT ||
> +				parse_current->data == CIL_KEY_BLOCKABSTRACT) {
> +				rc = SEPOL_ERR;
> +				cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in an after in-statement", (char *)parse_current->data);
> +				goto exit;
> +			}
> +		}
>  	}
>  
>  	if (macro != NULL) {
> diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
> index 98e303d1..d33c66bc 100644
> --- a/libsepol/cil/src/cil_internal.h
> +++ b/libsepol/cil/src/cil_internal.h
> @@ -56,10 +56,11 @@ enum cil_pass {
>  	CIL_PASS_INIT = 0,
>  
>  	CIL_PASS_TIF,
> -	CIL_PASS_IN,
> +	CIL_PASS_IN_BEFORE,
>  	CIL_PASS_BLKIN_LINK,
>  	CIL_PASS_BLKIN_COPY,
>  	CIL_PASS_BLKABS,
> +	CIL_PASS_IN_AFTER,
>  	CIL_PASS_CALL1,
>  	CIL_PASS_CALL2,
>  	CIL_PASS_ALIAS1,
> @@ -158,6 +159,8 @@ extern char *CIL_KEY_HANDLEUNKNOWN_DENY;
>  extern char *CIL_KEY_HANDLEUNKNOWN_REJECT;
>  extern char *CIL_KEY_MACRO;
>  extern char *CIL_KEY_IN;
> +extern char *CIL_KEY_IN_BEFORE;
> +extern char *CIL_KEY_IN_AFTER;
>  extern char *CIL_KEY_MLS;
>  extern char *CIL_KEY_DEFAULTRANGE;
>  extern char *CIL_KEY_BLOCKINHERIT;
> @@ -355,6 +358,7 @@ struct cil_blockabstract {
>  
>  struct cil_in {
>  	symtab_t symtab[CIL_SYM_NUM];
> +	int is_after;
>  	char *block_str;
>  };
>  
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 18007324..77e0d402 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -62,7 +62,8 @@ struct cil_args_resolve {
>  	struct cil_list *unordered_classorder_lists;
>  	struct cil_list *catorder_lists;
>  	struct cil_list *sensitivityorder_lists;
> -	struct cil_list *in_list;
> +	struct cil_list *in_list_before;
> +	struct cil_list *in_list_after;
>  	struct cil_stack *disabled_optionals;
>  };
>  
> @@ -2449,10 +2450,8 @@ exit:
>  	return rc;
>  }
>  
> -int cil_resolve_in_list(void *extra_args)
> +int cil_resolve_in_list(struct cil_list *in_list, void *extra_args)
>  {
> -	struct cil_args_resolve *args = extra_args;
> -	struct cil_list *ins = args->in_list;
>  	struct cil_list_item *curr = NULL;
>  	struct cil_tree_node *node = NULL;
>  	struct cil_tree_node *last_failed_node = NULL;
> @@ -2466,7 +2465,7 @@ int cil_resolve_in_list(void *extra_args)
>  		resolved = 0;
>  		unresolved = 0;
>  
> -		cil_list_for_each(curr, ins) {
> +		cil_list_for_each(curr, in_list) {
>  			if (curr->flavor != CIL_NODE) {
>  				continue;
>  			}
> @@ -3590,12 +3589,10 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
>  	int rc = SEPOL_OK;
>  	struct cil_args_resolve *args = extra_args;
>  	enum cil_pass pass = 0;
> -	struct cil_list *ins;
>  
>  	if (node == NULL || args == NULL) {
>  		goto exit;
>  	}
> -	ins = args->in_list;
>  
>  	pass = args->pass;
>  	switch (pass) {
> @@ -3604,11 +3601,14 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
>  			rc = cil_resolve_tunif(node, args);
>  		}
>  		break;
> -	case CIL_PASS_IN:
> +	case CIL_PASS_IN_BEFORE:
>  		if (node->flavor == CIL_IN) {
>  			// due to ordering issues, in statements are just gathered here and
>  			// resolved together in cil_resolve_in_list once all are found
> -			cil_list_prepend(ins, CIL_NODE, node);
> +			struct cil_in *in = node->data;
> +			if (in->is_after == CIL_FALSE) {
> +				cil_list_prepend(args->in_list_before, CIL_NODE, node);
> +			}
>  		}
>  		break;
>  	case CIL_PASS_BLKIN_LINK:
> @@ -3626,6 +3626,16 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
>  			rc = cil_resolve_blockabstract(node, args);
>  		}
>  		break;
> +	case CIL_PASS_IN_AFTER:
> +		if (node->flavor == CIL_IN) {
> +			// due to ordering issues, in statements are just gathered here and
> +			// resolved together in cil_resolve_in_list once all are found
> +			struct cil_in *in = node->data;
> +			if (in->is_after == CIL_TRUE) {
> +				cil_list_prepend(args->in_list_after, CIL_NODE, node);
> +			}
> +		}
> +		break;
>  	case CIL_PASS_CALL1:
>  		if (node->flavor == CIL_CALL && args->macro == NULL) {
>  			rc = cil_resolve_call(node, args);
> @@ -4073,7 +4083,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
>  	extra_args.unordered_classorder_lists = NULL;
>  	extra_args.catorder_lists = NULL;
>  	extra_args.sensitivityorder_lists = NULL;
> -	extra_args.in_list = NULL;
> +	extra_args.in_list_before = NULL;
> +	extra_args.in_list_after = NULL;
>  	extra_args.disabled_optionals = NULL;
>  
>  	cil_list_init(&extra_args.to_destroy, CIL_NODE);
> @@ -4082,7 +4093,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
>  	cil_list_init(&extra_args.unordered_classorder_lists, CIL_LIST_ITEM);
>  	cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
>  	cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
> -	cil_list_init(&extra_args.in_list, CIL_IN);
> +	cil_list_init(&extra_args.in_list_before, CIL_IN);
> +	cil_list_init(&extra_args.in_list_after, CIL_IN);
>  	cil_stack_init(&extra_args.disabled_optionals);
>  
>  	for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
> @@ -4093,12 +4105,18 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
>  			goto exit;
>  		}
>  
> -		if (pass == CIL_PASS_IN) {
> -			rc = cil_resolve_in_list(&extra_args);
> +		if (pass == CIL_PASS_IN_BEFORE) {
> +			rc = cil_resolve_in_list(extra_args.in_list_before, &extra_args);
> +			if (rc != SEPOL_OK) {
> +				goto exit;
> +			}
> +			cil_list_destroy(&extra_args.in_list_before, CIL_FALSE);
> +		} else if (pass == CIL_PASS_IN_AFTER) {
> +			rc = cil_resolve_in_list(extra_args.in_list_after, &extra_args);
>  			if (rc != SEPOL_OK) {
>  				goto exit;
>  			}
> -			cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> +			cil_list_destroy(&extra_args.in_list_after, CIL_FALSE);
>  		}
>  
>  		if (pass == CIL_PASS_BLKIN_LINK) {
> @@ -4217,7 +4235,8 @@ exit:
>  	__cil_ordered_lists_destroy(&extra_args.sensitivityorder_lists);
>  	__cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
>  	cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
> -	cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> +	cil_list_destroy(&extra_args.in_list_before, CIL_FALSE);
> +	cil_list_destroy(&extra_args.in_list_after, CIL_FALSE);
>  	cil_stack_destroy(&extra_args.disabled_optionals);
>  
>  	return rc;

-- 
gpg --locate-keys dominick.grift@defensec.nl
Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
https://sks-keyservers.net/pks/lookup?op=get&search=0xDA7E521F10F64098
Dominick Grift

Re: [PATCH 1/2] libsepol/cil: Improve in-statement to allow use after inheritance

[Nicolas Iooss]

 On Fri, Aug 13, 2021 at 1:52 PM James Carter <jwcart2@gmail.com> wrote:
>
> CIL's in-statement is resolved before block inheritance. This has
> the advantage of allowing an in-statement to add rules to a base
> block (say for a new permission) and having those rules also be
> added everywhere that base block is inherited. But the disadvantage
> of this behavior is that it is not possible to use an in-statement
> on a block that is inherited for the simple reason that that block
> does not exist when the in-statment is resolved.
>
> Change the syntax of the in-statement to allow specifying whether
> the rules should be added before or after inheritance. If neither
> is specified, then the behavior remains the same. All current
> in-statements will work as before.
>
> Either the old syntax
>   (in container_id
>       cil_statement
>       ...
>   )
> or the new syntax
>   (in before|after container_id
>       cil_statement
>       ...
>   )
> may be used for in-statements. But only "(in after ..." will have
> the new behavior. Using "(in before ..." will give the same
> behavior as before.
>
> Macro Example
> ;
> (block b1
>   (macro m1 ((type ARG1))
>     (allow ARG1 self (C1 (P1a)))
>   )
> )
> (in after b1.m1
>   (allow ARG1 self (C1 (P1c)))
> )
> (type t1a)
> (call b1.m1 (t1a))
> (blockinherit b1)
> (in after m1
>   (allow ARG1 self (C1 (P1b)))
> )
> (type t1b)
> (call m1 (t1b))
> ;
> This results in the following rules:
>   (allow t1a self (C1 (P1a)))
>   (allow t1a self (C1 (P1c)))
>   (allow t1b self (C1 (P1a)))
>   (allow t1b self (C1 (P1b)))
>
> Block Example
> ;
> (block b2
>   (block b
>     (type ta)
>     (allow ta self (C2 (P2a)))
>   )
> )
> (in before b2.b
>   (type tb)
>   (allow tb self (C2 (P2b)))
> )
> (block c2
>   (blockinherit b2)
>   (in after b
>     (type tc)
>     (allow tc self (C2 (P2c)))
>   )
> )
> ;
> This results in the following rules:
>   (allow b2.b.ta self (C2 (P2a)))
>   (allow b2.b.tb self (C2 (P2b)))
>   (allow c2.b.ta self (C2 (P2a)))
>   (allow c2.b.tb self (C2 (P2b)))
>   (allow c2.b.tc self (C2 (P2c)))
>
> Using in-statements on optionals also works as expected.
>
> One additional change is that blockabstract and blockinherit rules
> are not allowed when using an after in-statement. This is because
> both of those are resolved before an after in-statement would be
> resolved.
>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/src/cil.c             |  5 +++
>  libsepol/cil/src/cil_build_ast.c   | 31 +++++++++++++++++--
>  libsepol/cil/src/cil_internal.h    |  6 +++-
>  libsepol/cil/src/cil_resolve_ast.c | 49 +++++++++++++++++++++---------
>  4 files changed, 72 insertions(+), 19 deletions(-)
>
> diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
> index d24c81c8..672342b5 100644
> --- a/libsepol/cil/src/cil.c
> +++ b/libsepol/cil/src/cil.c
> @@ -142,6 +142,8 @@ char *CIL_KEY_HANDLEUNKNOWN_DENY;
>  char *CIL_KEY_HANDLEUNKNOWN_REJECT;
>  char *CIL_KEY_MACRO;
>  char *CIL_KEY_IN;
> +char *CIL_KEY_IN_BEFORE;
> +char *CIL_KEY_IN_AFTER;
>  char *CIL_KEY_MLS;
>  char *CIL_KEY_DEFAULTRANGE;
>  char *CIL_KEY_BLOCKINHERIT;
> @@ -353,6 +355,8 @@ static void cil_init_keys(void)
>         CIL_KEY_DEFAULTTYPE = cil_strpool_add("defaulttype");
>         CIL_KEY_MACRO = cil_strpool_add("macro");
>         CIL_KEY_IN = cil_strpool_add("in");
> +       CIL_KEY_IN_BEFORE = cil_strpool_add("before");
> +       CIL_KEY_IN_AFTER = cil_strpool_add("after");
>         CIL_KEY_MLS = cil_strpool_add("mls");
>         CIL_KEY_DEFAULTRANGE = cil_strpool_add("defaultrange");
>         CIL_KEY_GLOB = cil_strpool_add("*");
> @@ -2121,6 +2125,7 @@ void cil_in_init(struct cil_in **in)
>         *in = cil_malloc(sizeof(**in));
>
>         cil_symtab_array_init((*in)->symtab, cil_sym_sizes[CIL_SYM_ARRAY_IN]);
> +       (*in)->is_after = CIL_FALSE;
>         (*in)->block_str = NULL;
>  }
>
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index 9da90883..4a87e212 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -380,7 +380,8 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
>         enum cil_syntax syntax[] = {
>                 CIL_SYN_STRING,
>                 CIL_SYN_STRING,
> -               CIL_SYN_N_LISTS,
> +               CIL_SYN_STRING | CIL_SYN_N_LISTS,
> +               CIL_SYN_N_LISTS | CIL_SYN_END,
>                 CIL_SYN_END
>         };

Hello,
I am unfamiliar with __cil_verify_syntax, but when I see this
definition of syntax, it makes me think that the syntax allows {...,
CIL_SYN_N_LISTS, CIL_SYN_N_LISTS, CIL_SYN_END}, which is not supposed
to be allowed. Nevertheless when this happens,
parse_current->next->next->data should be NULL (if I understand
correctly what __cil_verify_syntax does with CIL_SYN_N_LISTS) so the
in statement is treated as a current in statement (with no
before/after keywords).

After more digging in __cil_verify_syntax, it seems that this first
interpretation is partially unsound ("two successive CIL_SYN_N_LISTS"
does not make sense) but in fact using "CIL_SYN_STRING |
CIL_SYN_N_LISTS" could cause issues, because this can match a list
(i.e. a sequence of nodes such that c->data == NULL && c->cl_head !=
NULL) ended with a string (i.e. c->data != NULL && c->cl_head ==
NULL). So I guess that it is possible to craft a CIL policy with "(in
container_id (cil_statements) some_string (other_cil_statements))",
and the current code could behave unexpectedly on "some_string".
This could in fact be a bug in __cil_verify_syntax, which should
verify that either the current node is a string or that the current
and its successors are all list nodes, but not a mix of "a list of
list nodes ended with a string", when CIL_SYN_STRING | CIL_SYN_N_LISTS
is used.

I do not currently have time to check that there is actually a bug
(and will not have before the end of August). So I am sharing my
thoughts to make you aware of this. If you see something that I missed
(and this is likely), please do not hesitate to reply that I am wrong.

Thanks,
Nicolas

>         int syntax_len = sizeof(syntax)/sizeof(*syntax);
> @@ -403,14 +404,29 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
>
>         cil_in_init(&in);
>
> -       in->block_str = parse_current->next->data;
> +       if (parse_current->next->next->data) {
> +               char *is_after_str = parse_current->next->data;
> +               if (is_after_str == CIL_KEY_IN_BEFORE) {
> +                       in->is_after = CIL_FALSE;
> +               } else if (is_after_str == CIL_KEY_IN_AFTER) {
> +                       in->is_after = CIL_TRUE;
> +               } else {
> +                       cil_log(CIL_ERR, "Value must be either \'before\' or \'after\'\n");
> +                       rc = SEPOL_ERR;
> +                       goto exit;
> +               }
> +               in->block_str = parse_current->next->next->data;
> +       } else {
> +               in->is_after = CIL_FALSE;
> +               in->block_str = parse_current->next->data;
> +       }
>
>         ast_node->data = in;
>         ast_node->flavor = CIL_IN;
>
>         return SEPOL_OK;
>  exit:
> -       cil_tree_log(parse_current, CIL_ERR, "Bad in statement");
> +       cil_tree_log(parse_current, CIL_ERR, "Bad in-statement");
>         cil_destroy_in(in);
>         return rc;
>  }
> @@ -6136,12 +6152,21 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
>         }
>
>         if (in != NULL) {
> +               struct cil_in *in_block = in->data;
>                 if (parse_current->data == CIL_KEY_TUNABLE ||
>                         parse_current->data == CIL_KEY_IN) {
>                         rc = SEPOL_ERR;
>                         cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in in-statement", (char *)parse_current->data);
>                         goto exit;
>                 }
> +               if (in_block->is_after == CIL_TRUE) {
> +                       if (parse_current->data == CIL_KEY_BLOCKINHERIT ||
> +                               parse_current->data == CIL_KEY_BLOCKABSTRACT) {
> +                               rc = SEPOL_ERR;
> +                               cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in an after in-statement", (char *)parse_current->data);
> +                               goto exit;
> +                       }
> +               }
>         }
>
>         if (macro != NULL) {
> diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
> index 98e303d1..d33c66bc 100644
> --- a/libsepol/cil/src/cil_internal.h
> +++ b/libsepol/cil/src/cil_internal.h
> @@ -56,10 +56,11 @@ enum cil_pass {
>         CIL_PASS_INIT = 0,
>
>         CIL_PASS_TIF,
> -       CIL_PASS_IN,
> +       CIL_PASS_IN_BEFORE,
>         CIL_PASS_BLKIN_LINK,
>         CIL_PASS_BLKIN_COPY,
>         CIL_PASS_BLKABS,
> +       CIL_PASS_IN_AFTER,
>         CIL_PASS_CALL1,
>         CIL_PASS_CALL2,
>         CIL_PASS_ALIAS1,
> @@ -158,6 +159,8 @@ extern char *CIL_KEY_HANDLEUNKNOWN_DENY;
>  extern char *CIL_KEY_HANDLEUNKNOWN_REJECT;
>  extern char *CIL_KEY_MACRO;
>  extern char *CIL_KEY_IN;
> +extern char *CIL_KEY_IN_BEFORE;
> +extern char *CIL_KEY_IN_AFTER;
>  extern char *CIL_KEY_MLS;
>  extern char *CIL_KEY_DEFAULTRANGE;
>  extern char *CIL_KEY_BLOCKINHERIT;
> @@ -355,6 +358,7 @@ struct cil_blockabstract {
>
>  struct cil_in {
>         symtab_t symtab[CIL_SYM_NUM];
> +       int is_after;
>         char *block_str;
>  };
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 18007324..77e0d402 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -62,7 +62,8 @@ struct cil_args_resolve {
>         struct cil_list *unordered_classorder_lists;
>         struct cil_list *catorder_lists;
>         struct cil_list *sensitivityorder_lists;
> -       struct cil_list *in_list;
> +       struct cil_list *in_list_before;
> +       struct cil_list *in_list_after;
>         struct cil_stack *disabled_optionals;
>  };
>
> @@ -2449,10 +2450,8 @@ exit:
>         return rc;
>  }
>
> -int cil_resolve_in_list(void *extra_args)
> +int cil_resolve_in_list(struct cil_list *in_list, void *extra_args)
>  {
> -       struct cil_args_resolve *args = extra_args;
> -       struct cil_list *ins = args->in_list;
>         struct cil_list_item *curr = NULL;
>         struct cil_tree_node *node = NULL;
>         struct cil_tree_node *last_failed_node = NULL;
> @@ -2466,7 +2465,7 @@ int cil_resolve_in_list(void *extra_args)
>                 resolved = 0;
>                 unresolved = 0;
>
> -               cil_list_for_each(curr, ins) {
> +               cil_list_for_each(curr, in_list) {
>                         if (curr->flavor != CIL_NODE) {
>                                 continue;
>                         }
> @@ -3590,12 +3589,10 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
>         int rc = SEPOL_OK;
>         struct cil_args_resolve *args = extra_args;
>         enum cil_pass pass = 0;
> -       struct cil_list *ins;
>
>         if (node == NULL || args == NULL) {
>                 goto exit;
>         }
> -       ins = args->in_list;
>
>         pass = args->pass;
>         switch (pass) {
> @@ -3604,11 +3601,14 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
>                         rc = cil_resolve_tunif(node, args);
>                 }
>                 break;
> -       case CIL_PASS_IN:
> +       case CIL_PASS_IN_BEFORE:
>                 if (node->flavor == CIL_IN) {
>                         // due to ordering issues, in statements are just gathered here and
>                         // resolved together in cil_resolve_in_list once all are found
> -                       cil_list_prepend(ins, CIL_NODE, node);
> +                       struct cil_in *in = node->data;
> +                       if (in->is_after == CIL_FALSE) {
> +                               cil_list_prepend(args->in_list_before, CIL_NODE, node);
> +                       }
>                 }
>                 break;
>         case CIL_PASS_BLKIN_LINK:
> @@ -3626,6 +3626,16 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
>                         rc = cil_resolve_blockabstract(node, args);
>                 }
>                 break;
> +       case CIL_PASS_IN_AFTER:
> +               if (node->flavor == CIL_IN) {
> +                       // due to ordering issues, in statements are just gathered here and
> +                       // resolved together in cil_resolve_in_list once all are found
> +                       struct cil_in *in = node->data;
> +                       if (in->is_after == CIL_TRUE) {
> +                               cil_list_prepend(args->in_list_after, CIL_NODE, node);
> +                       }
> +               }
> +               break;
>         case CIL_PASS_CALL1:
>                 if (node->flavor == CIL_CALL && args->macro == NULL) {
>                         rc = cil_resolve_call(node, args);
> @@ -4073,7 +4083,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
>         extra_args.unordered_classorder_lists = NULL;
>         extra_args.catorder_lists = NULL;
>         extra_args.sensitivityorder_lists = NULL;
> -       extra_args.in_list = NULL;
> +       extra_args.in_list_before = NULL;
> +       extra_args.in_list_after = NULL;
>         extra_args.disabled_optionals = NULL;
>
>         cil_list_init(&extra_args.to_destroy, CIL_NODE);
> @@ -4082,7 +4093,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
>         cil_list_init(&extra_args.unordered_classorder_lists, CIL_LIST_ITEM);
>         cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
>         cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
> -       cil_list_init(&extra_args.in_list, CIL_IN);
> +       cil_list_init(&extra_args.in_list_before, CIL_IN);
> +       cil_list_init(&extra_args.in_list_after, CIL_IN);
>         cil_stack_init(&extra_args.disabled_optionals);
>
>         for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
> @@ -4093,12 +4105,18 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
>                         goto exit;
>                 }
>
> -               if (pass == CIL_PASS_IN) {
> -                       rc = cil_resolve_in_list(&extra_args);
> +               if (pass == CIL_PASS_IN_BEFORE) {
> +                       rc = cil_resolve_in_list(extra_args.in_list_before, &extra_args);
> +                       if (rc != SEPOL_OK) {
> +                               goto exit;
> +                       }
> +                       cil_list_destroy(&extra_args.in_list_before, CIL_FALSE);
> +               } else if (pass == CIL_PASS_IN_AFTER) {
> +                       rc = cil_resolve_in_list(extra_args.in_list_after, &extra_args);
>                         if (rc != SEPOL_OK) {
>                                 goto exit;
>                         }
> -                       cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> +                       cil_list_destroy(&extra_args.in_list_after, CIL_FALSE);
>                 }
>
>                 if (pass == CIL_PASS_BLKIN_LINK) {
> @@ -4217,7 +4235,8 @@ exit:
>         __cil_ordered_lists_destroy(&extra_args.sensitivityorder_lists);
>         __cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
>         cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
> -       cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> +       cil_list_destroy(&extra_args.in_list_before, CIL_FALSE);
> +       cil_list_destroy(&extra_args.in_list_after, CIL_FALSE);
>         cil_stack_destroy(&extra_args.disabled_optionals);
>
>         return rc;
> --
> 2.31.1
>

Re: [PATCH 1/2] libsepol/cil: Improve in-statement to allow use after inheritance

[James Carter]

On Tue, Aug 17, 2021 at 4:31 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
>  On Fri, Aug 13, 2021 at 1:52 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > CIL's in-statement is resolved before block inheritance. This has
> > the advantage of allowing an in-statement to add rules to a base
> > block (say for a new permission) and having those rules also be
> > added everywhere that base block is inherited. But the disadvantage
> > of this behavior is that it is not possible to use an in-statement
> > on a block that is inherited for the simple reason that that block
> > does not exist when the in-statment is resolved.
> >
> > Change the syntax of the in-statement to allow specifying whether
> > the rules should be added before or after inheritance. If neither
> > is specified, then the behavior remains the same. All current
> > in-statements will work as before.
> >
> > Either the old syntax
> >   (in container_id
> >       cil_statement
> >       ...
> >   )
> > or the new syntax
> >   (in before|after container_id
> >       cil_statement
> >       ...
> >   )
> > may be used for in-statements. But only "(in after ..." will have
> > the new behavior. Using "(in before ..." will give the same
> > behavior as before.
> >
> > Macro Example
> > ;
> > (block b1
> >   (macro m1 ((type ARG1))
> >     (allow ARG1 self (C1 (P1a)))
> >   )
> > )
> > (in after b1.m1
> >   (allow ARG1 self (C1 (P1c)))
> > )
> > (type t1a)
> > (call b1.m1 (t1a))
> > (blockinherit b1)
> > (in after m1
> >   (allow ARG1 self (C1 (P1b)))
> > )
> > (type t1b)
> > (call m1 (t1b))
> > ;
> > This results in the following rules:
> >   (allow t1a self (C1 (P1a)))
> >   (allow t1a self (C1 (P1c)))
> >   (allow t1b self (C1 (P1a)))
> >   (allow t1b self (C1 (P1b)))
> >
> > Block Example
> > ;
> > (block b2
> >   (block b
> >     (type ta)
> >     (allow ta self (C2 (P2a)))
> >   )
> > )
> > (in before b2.b
> >   (type tb)
> >   (allow tb self (C2 (P2b)))
> > )
> > (block c2
> >   (blockinherit b2)
> >   (in after b
> >     (type tc)
> >     (allow tc self (C2 (P2c)))
> >   )
> > )
> > ;
> > This results in the following rules:
> >   (allow b2.b.ta self (C2 (P2a)))
> >   (allow b2.b.tb self (C2 (P2b)))
> >   (allow c2.b.ta self (C2 (P2a)))
> >   (allow c2.b.tb self (C2 (P2b)))
> >   (allow c2.b.tc self (C2 (P2c)))
> >
> > Using in-statements on optionals also works as expected.
> >
> > One additional change is that blockabstract and blockinherit rules
> > are not allowed when using an after in-statement. This is because
> > both of those are resolved before an after in-statement would be
> > resolved.
> >
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> >  libsepol/cil/src/cil.c             |  5 +++
> >  libsepol/cil/src/cil_build_ast.c   | 31 +++++++++++++++++--
> >  libsepol/cil/src/cil_internal.h    |  6 +++-
> >  libsepol/cil/src/cil_resolve_ast.c | 49 +++++++++++++++++++++---------
> >  4 files changed, 72 insertions(+), 19 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
> > index d24c81c8..672342b5 100644
> > --- a/libsepol/cil/src/cil.c
> > +++ b/libsepol/cil/src/cil.c
> > @@ -142,6 +142,8 @@ char *CIL_KEY_HANDLEUNKNOWN_DENY;
> >  char *CIL_KEY_HANDLEUNKNOWN_REJECT;
> >  char *CIL_KEY_MACRO;
> >  char *CIL_KEY_IN;
> > +char *CIL_KEY_IN_BEFORE;
> > +char *CIL_KEY_IN_AFTER;
> >  char *CIL_KEY_MLS;
> >  char *CIL_KEY_DEFAULTRANGE;
> >  char *CIL_KEY_BLOCKINHERIT;
> > @@ -353,6 +355,8 @@ static void cil_init_keys(void)
> >         CIL_KEY_DEFAULTTYPE = cil_strpool_add("defaulttype");
> >         CIL_KEY_MACRO = cil_strpool_add("macro");
> >         CIL_KEY_IN = cil_strpool_add("in");
> > +       CIL_KEY_IN_BEFORE = cil_strpool_add("before");
> > +       CIL_KEY_IN_AFTER = cil_strpool_add("after");
> >         CIL_KEY_MLS = cil_strpool_add("mls");
> >         CIL_KEY_DEFAULTRANGE = cil_strpool_add("defaultrange");
> >         CIL_KEY_GLOB = cil_strpool_add("*");
> > @@ -2121,6 +2125,7 @@ void cil_in_init(struct cil_in **in)
> >         *in = cil_malloc(sizeof(**in));
> >
> >         cil_symtab_array_init((*in)->symtab, cil_sym_sizes[CIL_SYM_ARRAY_IN]);
> > +       (*in)->is_after = CIL_FALSE;
> >         (*in)->block_str = NULL;
> >  }
> >
> > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> > index 9da90883..4a87e212 100644
> > --- a/libsepol/cil/src/cil_build_ast.c
> > +++ b/libsepol/cil/src/cil_build_ast.c
> > @@ -380,7 +380,8 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
> >         enum cil_syntax syntax[] = {
> >                 CIL_SYN_STRING,
> >                 CIL_SYN_STRING,
> > -               CIL_SYN_N_LISTS,
> > +               CIL_SYN_STRING | CIL_SYN_N_LISTS,
> > +               CIL_SYN_N_LISTS | CIL_SYN_END,
> >                 CIL_SYN_END
> >         };
>
> Hello,
> I am unfamiliar with __cil_verify_syntax, but when I see this
> definition of syntax, it makes me think that the syntax allows {...,
> CIL_SYN_N_LISTS, CIL_SYN_N_LISTS, CIL_SYN_END}, which is not supposed
> to be allowed. Nevertheless when this happens,
> parse_current->next->next->data should be NULL (if I understand
> correctly what __cil_verify_syntax does with CIL_SYN_N_LISTS) so the
> in statement is treated as a current in statement (with no
> before/after keywords).
>
> After more digging in __cil_verify_syntax, it seems that this first
> interpretation is partially unsound ("two successive CIL_SYN_N_LISTS"
> does not make sense) but in fact using "CIL_SYN_STRING |
> CIL_SYN_N_LISTS" could cause issues, because this can match a list
> (i.e. a sequence of nodes such that c->data == NULL && c->cl_head !=
> NULL) ended with a string (i.e. c->data != NULL && c->cl_head ==
> NULL). So I guess that it is possible to craft a CIL policy with "(in
> container_id (cil_statements) some_string (other_cil_statements))",
> and the current code could behave unexpectedly on "some_string".
> This could in fact be a bug in __cil_verify_syntax, which should
> verify that either the current node is a string or that the current
> and its successors are all list nodes, but not a mix of "a list of
> list nodes ended with a string", when CIL_SYN_STRING | CIL_SYN_N_LISTS
> is used.
>

I think that your analysis might be correct here. I will take a look
at this. I think that __cil_verify_syntax() is limited and might not
be capable of catching everything. So at the very least, I probably
need to do more checking in cil_gen_in().

> I do not currently have time to check that there is actually a bug
> (and will not have before the end of August). So I am sharing my
> thoughts to make you aware of this. If you see something that I missed
> (and this is likely), please do not hesitate to reply that I am wrong.
>

As always, thanks for your comments,
Jim

> Thanks,
> Nicolas
>
> >         int syntax_len = sizeof(syntax)/sizeof(*syntax);
> > @@ -403,14 +404,29 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
> >
> >         cil_in_init(&in);
> >
> > -       in->block_str = parse_current->next->data;
> > +       if (parse_current->next->next->data) {
> > +               char *is_after_str = parse_current->next->data;
> > +               if (is_after_str == CIL_KEY_IN_BEFORE) {
> > +                       in->is_after = CIL_FALSE;
> > +               } else if (is_after_str == CIL_KEY_IN_AFTER) {
> > +                       in->is_after = CIL_TRUE;
> > +               } else {
> > +                       cil_log(CIL_ERR, "Value must be either \'before\' or \'after\'\n");
> > +                       rc = SEPOL_ERR;
> > +                       goto exit;
> > +               }
> > +               in->block_str = parse_current->next->next->data;
> > +       } else {
> > +               in->is_after = CIL_FALSE;
> > +               in->block_str = parse_current->next->data;
> > +       }
> >
> >         ast_node->data = in;
> >         ast_node->flavor = CIL_IN;
> >
> >         return SEPOL_OK;
> >  exit:
> > -       cil_tree_log(parse_current, CIL_ERR, "Bad in statement");
> > +       cil_tree_log(parse_current, CIL_ERR, "Bad in-statement");
> >         cil_destroy_in(in);
> >         return rc;
> >  }
> > @@ -6136,12 +6152,21 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
> >         }
> >
> >         if (in != NULL) {
> > +               struct cil_in *in_block = in->data;
> >                 if (parse_current->data == CIL_KEY_TUNABLE ||
> >                         parse_current->data == CIL_KEY_IN) {
> >                         rc = SEPOL_ERR;
> >                         cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in in-statement", (char *)parse_current->data);
> >                         goto exit;
> >                 }
> > +               if (in_block->is_after == CIL_TRUE) {
> > +                       if (parse_current->data == CIL_KEY_BLOCKINHERIT ||
> > +                               parse_current->data == CIL_KEY_BLOCKABSTRACT) {
> > +                               rc = SEPOL_ERR;
> > +                               cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in an after in-statement", (char *)parse_current->data);
> > +                               goto exit;
> > +                       }
> > +               }
> >         }
> >
> >         if (macro != NULL) {
> > diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
> > index 98e303d1..d33c66bc 100644
> > --- a/libsepol/cil/src/cil_internal.h
> > +++ b/libsepol/cil/src/cil_internal.h
> > @@ -56,10 +56,11 @@ enum cil_pass {
> >         CIL_PASS_INIT = 0,
> >
> >         CIL_PASS_TIF,
> > -       CIL_PASS_IN,
> > +       CIL_PASS_IN_BEFORE,
> >         CIL_PASS_BLKIN_LINK,
> >         CIL_PASS_BLKIN_COPY,
> >         CIL_PASS_BLKABS,
> > +       CIL_PASS_IN_AFTER,
> >         CIL_PASS_CALL1,
> >         CIL_PASS_CALL2,
> >         CIL_PASS_ALIAS1,
> > @@ -158,6 +159,8 @@ extern char *CIL_KEY_HANDLEUNKNOWN_DENY;
> >  extern char *CIL_KEY_HANDLEUNKNOWN_REJECT;
> >  extern char *CIL_KEY_MACRO;
> >  extern char *CIL_KEY_IN;
> > +extern char *CIL_KEY_IN_BEFORE;
> > +extern char *CIL_KEY_IN_AFTER;
> >  extern char *CIL_KEY_MLS;
> >  extern char *CIL_KEY_DEFAULTRANGE;
> >  extern char *CIL_KEY_BLOCKINHERIT;
> > @@ -355,6 +358,7 @@ struct cil_blockabstract {
> >
> >  struct cil_in {
> >         symtab_t symtab[CIL_SYM_NUM];
> > +       int is_after;
> >         char *block_str;
> >  };
> >
> > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > index 18007324..77e0d402 100644
> > --- a/libsepol/cil/src/cil_resolve_ast.c
> > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > @@ -62,7 +62,8 @@ struct cil_args_resolve {
> >         struct cil_list *unordered_classorder_lists;
> >         struct cil_list *catorder_lists;
> >         struct cil_list *sensitivityorder_lists;
> > -       struct cil_list *in_list;
> > +       struct cil_list *in_list_before;
> > +       struct cil_list *in_list_after;
> >         struct cil_stack *disabled_optionals;
> >  };
> >
> > @@ -2449,10 +2450,8 @@ exit:
> >         return rc;
> >  }
> >
> > -int cil_resolve_in_list(void *extra_args)
> > +int cil_resolve_in_list(struct cil_list *in_list, void *extra_args)
> >  {
> > -       struct cil_args_resolve *args = extra_args;
> > -       struct cil_list *ins = args->in_list;
> >         struct cil_list_item *curr = NULL;
> >         struct cil_tree_node *node = NULL;
> >         struct cil_tree_node *last_failed_node = NULL;
> > @@ -2466,7 +2465,7 @@ int cil_resolve_in_list(void *extra_args)
> >                 resolved = 0;
> >                 unresolved = 0;
> >
> > -               cil_list_for_each(curr, ins) {
> > +               cil_list_for_each(curr, in_list) {
> >                         if (curr->flavor != CIL_NODE) {
> >                                 continue;
> >                         }
> > @@ -3590,12 +3589,10 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
> >         int rc = SEPOL_OK;
> >         struct cil_args_resolve *args = extra_args;
> >         enum cil_pass pass = 0;
> > -       struct cil_list *ins;
> >
> >         if (node == NULL || args == NULL) {
> >                 goto exit;
> >         }
> > -       ins = args->in_list;
> >
> >         pass = args->pass;
> >         switch (pass) {
> > @@ -3604,11 +3601,14 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
> >                         rc = cil_resolve_tunif(node, args);
> >                 }
> >                 break;
> > -       case CIL_PASS_IN:
> > +       case CIL_PASS_IN_BEFORE:
> >                 if (node->flavor == CIL_IN) {
> >                         // due to ordering issues, in statements are just gathered here and
> >                         // resolved together in cil_resolve_in_list once all are found
> > -                       cil_list_prepend(ins, CIL_NODE, node);
> > +                       struct cil_in *in = node->data;
> > +                       if (in->is_after == CIL_FALSE) {
> > +                               cil_list_prepend(args->in_list_before, CIL_NODE, node);
> > +                       }
> >                 }
> >                 break;
> >         case CIL_PASS_BLKIN_LINK:
> > @@ -3626,6 +3626,16 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
> >                         rc = cil_resolve_blockabstract(node, args);
> >                 }
> >                 break;
> > +       case CIL_PASS_IN_AFTER:
> > +               if (node->flavor == CIL_IN) {
> > +                       // due to ordering issues, in statements are just gathered here and
> > +                       // resolved together in cil_resolve_in_list once all are found
> > +                       struct cil_in *in = node->data;
> > +                       if (in->is_after == CIL_TRUE) {
> > +                               cil_list_prepend(args->in_list_after, CIL_NODE, node);
> > +                       }
> > +               }
> > +               break;
> >         case CIL_PASS_CALL1:
> >                 if (node->flavor == CIL_CALL && args->macro == NULL) {
> >                         rc = cil_resolve_call(node, args);
> > @@ -4073,7 +4083,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> >         extra_args.unordered_classorder_lists = NULL;
> >         extra_args.catorder_lists = NULL;
> >         extra_args.sensitivityorder_lists = NULL;
> > -       extra_args.in_list = NULL;
> > +       extra_args.in_list_before = NULL;
> > +       extra_args.in_list_after = NULL;
> >         extra_args.disabled_optionals = NULL;
> >
> >         cil_list_init(&extra_args.to_destroy, CIL_NODE);
> > @@ -4082,7 +4093,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> >         cil_list_init(&extra_args.unordered_classorder_lists, CIL_LIST_ITEM);
> >         cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
> >         cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
> > -       cil_list_init(&extra_args.in_list, CIL_IN);
> > +       cil_list_init(&extra_args.in_list_before, CIL_IN);
> > +       cil_list_init(&extra_args.in_list_after, CIL_IN);
> >         cil_stack_init(&extra_args.disabled_optionals);
> >
> >         for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
> > @@ -4093,12 +4105,18 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> >                         goto exit;
> >                 }
> >
> > -               if (pass == CIL_PASS_IN) {
> > -                       rc = cil_resolve_in_list(&extra_args);
> > +               if (pass == CIL_PASS_IN_BEFORE) {
> > +                       rc = cil_resolve_in_list(extra_args.in_list_before, &extra_args);
> > +                       if (rc != SEPOL_OK) {
> > +                               goto exit;
> > +                       }
> > +                       cil_list_destroy(&extra_args.in_list_before, CIL_FALSE);
> > +               } else if (pass == CIL_PASS_IN_AFTER) {
> > +                       rc = cil_resolve_in_list(extra_args.in_list_after, &extra_args);
> >                         if (rc != SEPOL_OK) {
> >                                 goto exit;
> >                         }
> > -                       cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> > +                       cil_list_destroy(&extra_args.in_list_after, CIL_FALSE);
> >                 }
> >
> >                 if (pass == CIL_PASS_BLKIN_LINK) {
> > @@ -4217,7 +4235,8 @@ exit:
> >         __cil_ordered_lists_destroy(&extra_args.sensitivityorder_lists);
> >         __cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
> >         cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
> > -       cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> > +       cil_list_destroy(&extra_args.in_list_before, CIL_FALSE);
> > +       cil_list_destroy(&extra_args.in_list_after, CIL_FALSE);
> >         cil_stack_destroy(&extra_args.disabled_optionals);
> >
> >         return rc;
> > --
> > 2.31.1
> >
>

Re: [PATCH 1/2] libsepol/cil: Improve in-statement to allow use after inheritance

[James Carter]

On Tue, Aug 17, 2021 at 5:05 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, Aug 17, 2021 at 4:31 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> >  On Fri, Aug 13, 2021 at 1:52 PM James Carter <jwcart2@gmail.com> wrote:
> > >
> > > CIL's in-statement is resolved before block inheritance. This has
> > > the advantage of allowing an in-statement to add rules to a base
> > > block (say for a new permission) and having those rules also be
> > > added everywhere that base block is inherited. But the disadvantage
> > > of this behavior is that it is not possible to use an in-statement
> > > on a block that is inherited for the simple reason that that block
> > > does not exist when the in-statment is resolved.
> > >
> > > Change the syntax of the in-statement to allow specifying whether
> > > the rules should be added before or after inheritance. If neither
> > > is specified, then the behavior remains the same. All current
> > > in-statements will work as before.
> > >
> > > Either the old syntax
> > >   (in container_id
> > >       cil_statement
> > >       ...
> > >   )
> > > or the new syntax
> > >   (in before|after container_id
> > >       cil_statement
> > >       ...
> > >   )
> > > may be used for in-statements. But only "(in after ..." will have
> > > the new behavior. Using "(in before ..." will give the same
> > > behavior as before.
> > >
> > > Macro Example
> > > ;
> > > (block b1
> > >   (macro m1 ((type ARG1))
> > >     (allow ARG1 self (C1 (P1a)))
> > >   )
> > > )
> > > (in after b1.m1
> > >   (allow ARG1 self (C1 (P1c)))
> > > )
> > > (type t1a)
> > > (call b1.m1 (t1a))
> > > (blockinherit b1)
> > > (in after m1
> > >   (allow ARG1 self (C1 (P1b)))
> > > )
> > > (type t1b)
> > > (call m1 (t1b))
> > > ;
> > > This results in the following rules:
> > >   (allow t1a self (C1 (P1a)))
> > >   (allow t1a self (C1 (P1c)))
> > >   (allow t1b self (C1 (P1a)))
> > >   (allow t1b self (C1 (P1b)))
> > >
> > > Block Example
> > > ;
> > > (block b2
> > >   (block b
> > >     (type ta)
> > >     (allow ta self (C2 (P2a)))
> > >   )
> > > )
> > > (in before b2.b
> > >   (type tb)
> > >   (allow tb self (C2 (P2b)))
> > > )
> > > (block c2
> > >   (blockinherit b2)
> > >   (in after b
> > >     (type tc)
> > >     (allow tc self (C2 (P2c)))
> > >   )
> > > )
> > > ;
> > > This results in the following rules:
> > >   (allow b2.b.ta self (C2 (P2a)))
> > >   (allow b2.b.tb self (C2 (P2b)))
> > >   (allow c2.b.ta self (C2 (P2a)))
> > >   (allow c2.b.tb self (C2 (P2b)))
> > >   (allow c2.b.tc self (C2 (P2c)))
> > >
> > > Using in-statements on optionals also works as expected.
> > >
> > > One additional change is that blockabstract and blockinherit rules
> > > are not allowed when using an after in-statement. This is because
> > > both of those are resolved before an after in-statement would be
> > > resolved.
> > >
> > > Signed-off-by: James Carter <jwcart2@gmail.com>

These two patches have been merged.
Jim

> > > ---
> > >  libsepol/cil/src/cil.c             |  5 +++
> > >  libsepol/cil/src/cil_build_ast.c   | 31 +++++++++++++++++--
> > >  libsepol/cil/src/cil_internal.h    |  6 +++-
> > >  libsepol/cil/src/cil_resolve_ast.c | 49 +++++++++++++++++++++---------
> > >  4 files changed, 72 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
> > > index d24c81c8..672342b5 100644
> > > --- a/libsepol/cil/src/cil.c
> > > +++ b/libsepol/cil/src/cil.c
> > > @@ -142,6 +142,8 @@ char *CIL_KEY_HANDLEUNKNOWN_DENY;
> > >  char *CIL_KEY_HANDLEUNKNOWN_REJECT;
> > >  char *CIL_KEY_MACRO;
> > >  char *CIL_KEY_IN;
> > > +char *CIL_KEY_IN_BEFORE;
> > > +char *CIL_KEY_IN_AFTER;
> > >  char *CIL_KEY_MLS;
> > >  char *CIL_KEY_DEFAULTRANGE;
> > >  char *CIL_KEY_BLOCKINHERIT;
> > > @@ -353,6 +355,8 @@ static void cil_init_keys(void)
> > >         CIL_KEY_DEFAULTTYPE = cil_strpool_add("defaulttype");
> > >         CIL_KEY_MACRO = cil_strpool_add("macro");
> > >         CIL_KEY_IN = cil_strpool_add("in");
> > > +       CIL_KEY_IN_BEFORE = cil_strpool_add("before");
> > > +       CIL_KEY_IN_AFTER = cil_strpool_add("after");
> > >         CIL_KEY_MLS = cil_strpool_add("mls");
> > >         CIL_KEY_DEFAULTRANGE = cil_strpool_add("defaultrange");
> > >         CIL_KEY_GLOB = cil_strpool_add("*");
> > > @@ -2121,6 +2125,7 @@ void cil_in_init(struct cil_in **in)
> > >         *in = cil_malloc(sizeof(**in));
> > >
> > >         cil_symtab_array_init((*in)->symtab, cil_sym_sizes[CIL_SYM_ARRAY_IN]);
> > > +       (*in)->is_after = CIL_FALSE;
> > >         (*in)->block_str = NULL;
> > >  }
> > >
> > > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> > > index 9da90883..4a87e212 100644
> > > --- a/libsepol/cil/src/cil_build_ast.c
> > > +++ b/libsepol/cil/src/cil_build_ast.c
> > > @@ -380,7 +380,8 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
> > >         enum cil_syntax syntax[] = {
> > >                 CIL_SYN_STRING,
> > >                 CIL_SYN_STRING,
> > > -               CIL_SYN_N_LISTS,
> > > +               CIL_SYN_STRING | CIL_SYN_N_LISTS,
> > > +               CIL_SYN_N_LISTS | CIL_SYN_END,
> > >                 CIL_SYN_END
> > >         };
> >
> > Hello,
> > I am unfamiliar with __cil_verify_syntax, but when I see this
> > definition of syntax, it makes me think that the syntax allows {...,
> > CIL_SYN_N_LISTS, CIL_SYN_N_LISTS, CIL_SYN_END}, which is not supposed
> > to be allowed. Nevertheless when this happens,
> > parse_current->next->next->data should be NULL (if I understand
> > correctly what __cil_verify_syntax does with CIL_SYN_N_LISTS) so the
> > in statement is treated as a current in statement (with no
> > before/after keywords).
> >
> > After more digging in __cil_verify_syntax, it seems that this first
> > interpretation is partially unsound ("two successive CIL_SYN_N_LISTS"
> > does not make sense) but in fact using "CIL_SYN_STRING |
> > CIL_SYN_N_LISTS" could cause issues, because this can match a list
> > (i.e. a sequence of nodes such that c->data == NULL && c->cl_head !=
> > NULL) ended with a string (i.e. c->data != NULL && c->cl_head ==
> > NULL). So I guess that it is possible to craft a CIL policy with "(in
> > container_id (cil_statements) some_string (other_cil_statements))",
> > and the current code could behave unexpectedly on "some_string".
> > This could in fact be a bug in __cil_verify_syntax, which should
> > verify that either the current node is a string or that the current
> > and its successors are all list nodes, but not a mix of "a list of
> > list nodes ended with a string", when CIL_SYN_STRING | CIL_SYN_N_LISTS
> > is used.
> >
>
> I think that your analysis might be correct here. I will take a look
> at this. I think that __cil_verify_syntax() is limited and might not
> be capable of catching everything. So at the very least, I probably
> need to do more checking in cil_gen_in().
>
> > I do not currently have time to check that there is actually a bug
> > (and will not have before the end of August). So I am sharing my
> > thoughts to make you aware of this. If you see something that I missed
> > (and this is likely), please do not hesitate to reply that I am wrong.
> >
>
> As always, thanks for your comments,
> Jim
>
> > Thanks,
> > Nicolas
> >
> > >         int syntax_len = sizeof(syntax)/sizeof(*syntax);
> > > @@ -403,14 +404,29 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
> > >
> > >         cil_in_init(&in);
> > >
> > > -       in->block_str = parse_current->next->data;
> > > +       if (parse_current->next->next->data) {
> > > +               char *is_after_str = parse_current->next->data;
> > > +               if (is_after_str == CIL_KEY_IN_BEFORE) {
> > > +                       in->is_after = CIL_FALSE;
> > > +               } else if (is_after_str == CIL_KEY_IN_AFTER) {
> > > +                       in->is_after = CIL_TRUE;
> > > +               } else {
> > > +                       cil_log(CIL_ERR, "Value must be either \'before\' or \'after\'\n");
> > > +                       rc = SEPOL_ERR;
> > > +                       goto exit;
> > > +               }
> > > +               in->block_str = parse_current->next->next->data;
> > > +       } else {
> > > +               in->is_after = CIL_FALSE;
> > > +               in->block_str = parse_current->next->data;
> > > +       }
> > >
> > >         ast_node->data = in;
> > >         ast_node->flavor = CIL_IN;
> > >
> > >         return SEPOL_OK;
> > >  exit:
> > > -       cil_tree_log(parse_current, CIL_ERR, "Bad in statement");
> > > +       cil_tree_log(parse_current, CIL_ERR, "Bad in-statement");
> > >         cil_destroy_in(in);
> > >         return rc;
> > >  }
> > > @@ -6136,12 +6152,21 @@ int __cil_build_ast_node_helper(struct cil_tree_node *parse_current, uint32_t *f
> > >         }
> > >
> > >         if (in != NULL) {
> > > +               struct cil_in *in_block = in->data;
> > >                 if (parse_current->data == CIL_KEY_TUNABLE ||
> > >                         parse_current->data == CIL_KEY_IN) {
> > >                         rc = SEPOL_ERR;
> > >                         cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in in-statement", (char *)parse_current->data);
> > >                         goto exit;
> > >                 }
> > > +               if (in_block->is_after == CIL_TRUE) {
> > > +                       if (parse_current->data == CIL_KEY_BLOCKINHERIT ||
> > > +                               parse_current->data == CIL_KEY_BLOCKABSTRACT) {
> > > +                               rc = SEPOL_ERR;
> > > +                               cil_tree_log(parse_current, CIL_ERR, "%s is not allowed in an after in-statement", (char *)parse_current->data);
> > > +                               goto exit;
> > > +                       }
> > > +               }
> > >         }
> > >
> > >         if (macro != NULL) {
> > > diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
> > > index 98e303d1..d33c66bc 100644
> > > --- a/libsepol/cil/src/cil_internal.h
> > > +++ b/libsepol/cil/src/cil_internal.h
> > > @@ -56,10 +56,11 @@ enum cil_pass {
> > >         CIL_PASS_INIT = 0,
> > >
> > >         CIL_PASS_TIF,
> > > -       CIL_PASS_IN,
> > > +       CIL_PASS_IN_BEFORE,
> > >         CIL_PASS_BLKIN_LINK,
> > >         CIL_PASS_BLKIN_COPY,
> > >         CIL_PASS_BLKABS,
> > > +       CIL_PASS_IN_AFTER,
> > >         CIL_PASS_CALL1,
> > >         CIL_PASS_CALL2,
> > >         CIL_PASS_ALIAS1,
> > > @@ -158,6 +159,8 @@ extern char *CIL_KEY_HANDLEUNKNOWN_DENY;
> > >  extern char *CIL_KEY_HANDLEUNKNOWN_REJECT;
> > >  extern char *CIL_KEY_MACRO;
> > >  extern char *CIL_KEY_IN;
> > > +extern char *CIL_KEY_IN_BEFORE;
> > > +extern char *CIL_KEY_IN_AFTER;
> > >  extern char *CIL_KEY_MLS;
> > >  extern char *CIL_KEY_DEFAULTRANGE;
> > >  extern char *CIL_KEY_BLOCKINHERIT;
> > > @@ -355,6 +358,7 @@ struct cil_blockabstract {
> > >
> > >  struct cil_in {
> > >         symtab_t symtab[CIL_SYM_NUM];
> > > +       int is_after;
> > >         char *block_str;
> > >  };
> > >
> > > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > > index 18007324..77e0d402 100644
> > > --- a/libsepol/cil/src/cil_resolve_ast.c
> > > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > > @@ -62,7 +62,8 @@ struct cil_args_resolve {
> > >         struct cil_list *unordered_classorder_lists;
> > >         struct cil_list *catorder_lists;
> > >         struct cil_list *sensitivityorder_lists;
> > > -       struct cil_list *in_list;
> > > +       struct cil_list *in_list_before;
> > > +       struct cil_list *in_list_after;
> > >         struct cil_stack *disabled_optionals;
> > >  };
> > >
> > > @@ -2449,10 +2450,8 @@ exit:
> > >         return rc;
> > >  }
> > >
> > > -int cil_resolve_in_list(void *extra_args)
> > > +int cil_resolve_in_list(struct cil_list *in_list, void *extra_args)
> > >  {
> > > -       struct cil_args_resolve *args = extra_args;
> > > -       struct cil_list *ins = args->in_list;
> > >         struct cil_list_item *curr = NULL;
> > >         struct cil_tree_node *node = NULL;
> > >         struct cil_tree_node *last_failed_node = NULL;
> > > @@ -2466,7 +2465,7 @@ int cil_resolve_in_list(void *extra_args)
> > >                 resolved = 0;
> > >                 unresolved = 0;
> > >
> > > -               cil_list_for_each(curr, ins) {
> > > +               cil_list_for_each(curr, in_list) {
> > >                         if (curr->flavor != CIL_NODE) {
> > >                                 continue;
> > >                         }
> > > @@ -3590,12 +3589,10 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
> > >         int rc = SEPOL_OK;
> > >         struct cil_args_resolve *args = extra_args;
> > >         enum cil_pass pass = 0;
> > > -       struct cil_list *ins;
> > >
> > >         if (node == NULL || args == NULL) {
> > >                 goto exit;
> > >         }
> > > -       ins = args->in_list;
> > >
> > >         pass = args->pass;
> > >         switch (pass) {
> > > @@ -3604,11 +3601,14 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
> > >                         rc = cil_resolve_tunif(node, args);
> > >                 }
> > >                 break;
> > > -       case CIL_PASS_IN:
> > > +       case CIL_PASS_IN_BEFORE:
> > >                 if (node->flavor == CIL_IN) {
> > >                         // due to ordering issues, in statements are just gathered here and
> > >                         // resolved together in cil_resolve_in_list once all are found
> > > -                       cil_list_prepend(ins, CIL_NODE, node);
> > > +                       struct cil_in *in = node->data;
> > > +                       if (in->is_after == CIL_FALSE) {
> > > +                               cil_list_prepend(args->in_list_before, CIL_NODE, node);
> > > +                       }
> > >                 }
> > >                 break;
> > >         case CIL_PASS_BLKIN_LINK:
> > > @@ -3626,6 +3626,16 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
> > >                         rc = cil_resolve_blockabstract(node, args);
> > >                 }
> > >                 break;
> > > +       case CIL_PASS_IN_AFTER:
> > > +               if (node->flavor == CIL_IN) {
> > > +                       // due to ordering issues, in statements are just gathered here and
> > > +                       // resolved together in cil_resolve_in_list once all are found
> > > +                       struct cil_in *in = node->data;
> > > +                       if (in->is_after == CIL_TRUE) {
> > > +                               cil_list_prepend(args->in_list_after, CIL_NODE, node);
> > > +                       }
> > > +               }
> > > +               break;
> > >         case CIL_PASS_CALL1:
> > >                 if (node->flavor == CIL_CALL && args->macro == NULL) {
> > >                         rc = cil_resolve_call(node, args);
> > > @@ -4073,7 +4083,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > >         extra_args.unordered_classorder_lists = NULL;
> > >         extra_args.catorder_lists = NULL;
> > >         extra_args.sensitivityorder_lists = NULL;
> > > -       extra_args.in_list = NULL;
> > > +       extra_args.in_list_before = NULL;
> > > +       extra_args.in_list_after = NULL;
> > >         extra_args.disabled_optionals = NULL;
> > >
> > >         cil_list_init(&extra_args.to_destroy, CIL_NODE);
> > > @@ -4082,7 +4093,8 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > >         cil_list_init(&extra_args.unordered_classorder_lists, CIL_LIST_ITEM);
> > >         cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
> > >         cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
> > > -       cil_list_init(&extra_args.in_list, CIL_IN);
> > > +       cil_list_init(&extra_args.in_list_before, CIL_IN);
> > > +       cil_list_init(&extra_args.in_list_after, CIL_IN);
> > >         cil_stack_init(&extra_args.disabled_optionals);
> > >
> > >         for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
> > > @@ -4093,12 +4105,18 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > >                         goto exit;
> > >                 }
> > >
> > > -               if (pass == CIL_PASS_IN) {
> > > -                       rc = cil_resolve_in_list(&extra_args);
> > > +               if (pass == CIL_PASS_IN_BEFORE) {
> > > +                       rc = cil_resolve_in_list(extra_args.in_list_before, &extra_args);
> > > +                       if (rc != SEPOL_OK) {
> > > +                               goto exit;
> > > +                       }
> > > +                       cil_list_destroy(&extra_args.in_list_before, CIL_FALSE);
> > > +               } else if (pass == CIL_PASS_IN_AFTER) {
> > > +                       rc = cil_resolve_in_list(extra_args.in_list_after, &extra_args);
> > >                         if (rc != SEPOL_OK) {
> > >                                 goto exit;
> > >                         }
> > > -                       cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> > > +                       cil_list_destroy(&extra_args.in_list_after, CIL_FALSE);
> > >                 }
> > >
> > >                 if (pass == CIL_PASS_BLKIN_LINK) {
> > > @@ -4217,7 +4235,8 @@ exit:
> > >         __cil_ordered_lists_destroy(&extra_args.sensitivityorder_lists);
> > >         __cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
> > >         cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
> > > -       cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> > > +       cil_list_destroy(&extra_args.in_list_before, CIL_FALSE);
> > > +       cil_list_destroy(&extra_args.in_list_after, CIL_FALSE);
> > >         cil_stack_destroy(&extra_args.disabled_optionals);
> > >
> > >         return rc;
> > > --
> > > 2.31.1
> > >
> >

[PATCH 2/2] libsepol/secilc/docs: Update the CIL documentation

[James Carter]

Update the CIL documentation for the in-statement processing and
duplicate macro and block declarations with block inheritance.

Duplicate macro and block declarations are allowed if they occur as
the result of block inheritance. Document the fact that inherited
macros are overridden by any macros already declared in a
namespace and that declaring a block in a namespace that will
inherit a block with the same name can be used to allow in-statements
to be used on the block.

The new in-statement syntax still supports the old syntax but adds
the ability to specify whether the in-statement should be resolved
before or after block inheritance is resolved.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 secilc/docs/cil_call_macro_statements.md |  2 ++
 secilc/docs/cil_container_statements.md  | 12 +++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/secilc/docs/cil_call_macro_statements.md b/secilc/docs/cil_call_macro_statements.md
index 3cc14bf8..dcc6139f 100644
--- a/secilc/docs/cil_call_macro_statements.md
+++ b/secilc/docs/cil_call_macro_statements.md
@@ -60,6 +60,8 @@ Declare a macro in the current namespace with its associated parameters. The mac
 
 [`tunable`](cil_conditional_statements.md#tunable), [`in`](cil_container_statements.md#in), [`block`](cil_container_statements.md#block), [`blockinherit`](cil_container_statements.md#blockinherit), [`blockabstract`](cil_container_statements.md#blockabstract), and other [`macro`](cil_call_macro_statements.md#macro) statements are not allowed in [`macro`](cil_call_macro_statements.md#macro) blocks.
 
+Duplicate [`macro`](cil_call_macro_statements.md#macro) declarations in the same namespace will normally cause an error, but inheriting a macro into a namespace (with [`blockinherit`](cil_container_statements.md#blockinherit)) that already has a macro with the same name will only result in a warning message and not cause an error. This behavior allows inherited macros to be overridden with local ones.
+
 **Statement definition:**
 
 ```secil
diff --git a/secilc/docs/cil_container_statements.md b/secilc/docs/cil_container_statements.md
index 41a4612c..f70160cb 100644
--- a/secilc/docs/cil_container_statements.md
+++ b/secilc/docs/cil_container_statements.md
@@ -10,6 +10,8 @@ Not allowed in [`macro`](cil_call_macro_statements.md#macro) and [`optional`](ci
 
 [`sensitivity`](cil_mls_labeling_statements.md#sensitivity) and [`category`](cil_mls_labeling_statements.md#category) statements are not allowed in [`block`](cil_container_statements.md#block) blocks.
 
+Duplicate declarations of a [`block`](cil_container_statements.md#block) in the same namespace will normally cause an error, but inheriting a block into a namespace (with [`blockinherit`](cil_container_statements.md#blockinherit)) that already has a block with the same name will only result in a warning message and not cause an error. The policy from both blocks will end up in the binary policy. This behavior was used in the past to allow a block to be declared so that an [`in-statement`](cil_container_statements.md#in) could be used on it, but now an [`in-statement`](cil_container_statements.md#in) can be specified to occur after inheritance, so this behavior is not necessary (but is still allowed).
+
 **Statement definition:**
 
 ```secil
@@ -278,7 +280,7 @@ This example will instantiate the optional block `ext_gateway.move_file` into po
 in
 --
 
-Allows the insertion of CIL statements into a named container ([`block`](cil_container_statements.md#block), [`optional`](cil_container_statements.md#optional) or [`macro`](cil_call_macro_statements.md#macro)).
+Allows the insertion of CIL statements into a named container ([`block`](cil_container_statements.md#block), [`optional`](cil_container_statements.md#optional) or [`macro`](cil_call_macro_statements.md#macro)). This insertion can be specified to occur either before or after block inheritance has been resolved.
 
 Not allowed in [`macro`](cil_call_macro_statements.md#macro), [`booleanif`](cil_conditional_statements.md#booleanif), and other [`in`](cil_container_statements.md#in) blocks.
 
@@ -287,7 +289,7 @@ Not allowed in [`macro`](cil_call_macro_statements.md#macro), [`booleanif`](cil_
 **Statement definition:**
 
 ```secil
-    (in container_id
+    (in [before|after] container_id
         cil_statement
         ...
     )
@@ -306,10 +308,14 @@ Not allowed in [`macro`](cil_call_macro_statements.md#macro), [`booleanif`](cil_
 <td align="left"><p>The <code>in</code> keyword.</p></td>
 </tr>
 <tr class="even">
+<td align="left"><p><code>before|after</code></p></td>
+<td align="left"><p>An optional value that specifies whether to process the [`in`](cil_container_statements.md#in) <code>before</code> or <code>after</code> block inheritance. If no value is specified, then the [`in`](cil_container_statements.md#in) will be processed before block inheritance.</p></td>
+</tr>
+<tr class="odd">
 <td align="left"><p><code>container_id</code></p></td>
 <td align="left"><p>A valid <code>block</code>, <code>optional</code> or <code>macro</code> namespace identifier.</p></td>
 </tr>
-<tr class="odd">
+<tr class="even">
 <td align="left"><p><code>cil_statement</code></p></td>
 <td align="left"><p>Zero or more valid CIL statements.</p></td>
 </tr>
-- 
2.31.1

Re: [PATCH] libsepol/cil: Allow some duplicate macro and block declarations

[James Carter]

Sorry, didn't mean to send this patch again.
Jim

On Fri, Aug 13, 2021 at 7:52 AM James Carter <jwcart2@gmail.com> wrote:
>
> The commit d155b410d4bbc90d28f361b966f0429598da8188 (libsepol/cil:
> Check for duplicate blocks, optionals, and macros) added checks when
> copying blocks, macros, and optionals so that a duplicate would cause
> an exit with an error. Unfortunately, some policies exist that depend
> on this behavior when using inheritance.
>
> The behavior is as follows.
>
> For macros only the first declared macro matters.
> ;
> (macro m ((type ARG1))
>   (allow ARG1 self (CLASS (PERM1)))
> )
> (block b
>   (macro m ((type ARG1))
>     (allow ARG1 self (CLASS (PERM2)))
>   )
> )
> (blockinherit b)
> (type t)
> (call m (t))
> ;
> For this policy segment, the macro m in block b will not be called.
> Only the original macro m will be.
>
> This behavior has been used to override macros that are going to
> be inherited. Only the inherited macros that have not already been
> declared in the destination namespace will be used.
>
> Blocks seem to work fine even though there are two of them
> ;
> (block b1
>   (blockinherit b2)
>   (block b
>     (type t1)
>     (allow t1 self (CLASS (PERM)))
>   )
> )
> (block b2
>   (block b
>     (type t2)
>     (allow t2 self (CLASS (PERM)))
>   )
> )
> (blockinherit b1)
> ;
> In this example, the blockinherit of b2 will cause there to be
> two block b's in block b1. Note that if both block b's tried to
> declare the same type, then that would be an error. The blockinherit
> of b1 will copy both block b's.
>
> This behavior has been used to allow the use of in-statements for
> a block that is being inherited. Since the in-statements are resolved
> before block inheritance, this only works if a block with the same
> name as the block to be inherited is declared in the namespace.
>
> To support the use of these two behaviors, allow duplicate blocks
> and macros when they occur as the result of block inheritance. In
> any other circumstances and error for a redeclaration will be given.
>
> Since the duplicate macro is not going to be used it is just skipped.
> The duplicate block will use the datum of the original block. In both
> cases a warning message will be produced (it will only be seen if
> "-v" is used when compiling the policy).
>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/src/cil_copy_ast.c | 69 ++++++++++++++++++++++++---------
>  1 file changed, 50 insertions(+), 19 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_copy_ast.c b/libsepol/cil/src/cil_copy_ast.c
> index 9c0231f2..67be2ec8 100644
> --- a/libsepol/cil/src/cil_copy_ast.c
> +++ b/libsepol/cil/src/cil_copy_ast.c
> @@ -43,6 +43,7 @@
>  #include "cil_verify.h"
>
>  struct cil_args_copy {
> +       struct cil_tree_node *orig_dest;
>         struct cil_tree_node *dest;
>         struct cil_db *db;
>  };
> @@ -101,17 +102,23 @@ int cil_copy_block(__attribute__((unused)) struct cil_db *db, void *data, void *
>         struct cil_block *orig = data;
>         char *key = orig->datum.name;
>         struct cil_symtab_datum *datum = NULL;
> -       struct cil_block *new;
>
>         cil_symtab_get_datum(symtab, key, &datum);
>         if (datum != NULL) {
> -               cil_tree_log(NODE(datum), CIL_ERR, "Re-declaration of %s %s", cil_node_to_string(NODE(datum)), key);
> -               return SEPOL_ERR;
> +               if (FLAVOR(datum) != CIL_BLOCK) {
> +                       cil_tree_log(NODE(orig), CIL_ERR, "Block %s being copied", key);
> +                       cil_tree_log(NODE(datum), CIL_ERR, "  Conflicts with %s already declared", cil_node_to_string(NODE(datum)));
> +                       return SEPOL_ERR;
> +               }
> +               cil_tree_log(NODE(orig), CIL_WARN, "Block %s being copied", key);
> +               cil_tree_log(NODE(datum), CIL_WARN, "  Previously declared", key);
> +               *copy = datum;
> +       } else {
> +               struct cil_block *new;
> +               cil_block_init(&new);
> +               *copy = new;
>         }
>
> -       cil_block_init(&new);
> -       *copy = new;
> -
>         return SEPOL_OK;
>  }
>
> @@ -1511,21 +1518,26 @@ int cil_copy_macro(__attribute__((unused)) struct cil_db *db, void *data, void *
>         struct cil_macro *orig = data;
>         char *key = orig->datum.name;
>         struct cil_symtab_datum *datum = NULL;
> -       struct cil_macro *new;
>
>         cil_symtab_get_datum(symtab, key, &datum);
>         if (datum != NULL) {
> -               cil_tree_log(NODE(datum), CIL_ERR, "Re-declaration of %s %s", cil_node_to_string(NODE(datum)), key);
> -               return SEPOL_ERR;
> -       }
> -
> -       cil_macro_init(&new);
> -       if (orig->params != NULL) {
> -               cil_copy_list(orig->params, &new->params);
> +               if (FLAVOR(datum) != CIL_MACRO) {
> +                       cil_tree_log(NODE(orig), CIL_ERR, "Macro %s being copied", key);
> +                       cil_tree_log(NODE(datum), CIL_ERR, "  Conflicts with %s already declared", cil_node_to_string(NODE(datum)));
> +                       return SEPOL_ERR;
> +               }
> +               cil_tree_log(NODE(orig), CIL_WARN, "Skipping macro %s", key);
> +               cil_tree_log(NODE(datum), CIL_WARN, "  Previously declared");
> +               *copy = NULL;
> +       } else {
> +               struct cil_macro *new;
> +               cil_macro_init(&new);
> +               if (orig->params != NULL) {
> +                       cil_copy_list(orig->params, &new->params);
> +               }
> +               *copy = new;
>         }
>
> -       *copy = new;
> -
>         return SEPOL_OK;
>  }
>
> @@ -1700,7 +1712,7 @@ int cil_copy_src_info(__attribute__((unused)) struct cil_db *db, void *data, voi
>         return SEPOL_OK;
>  }
>
> -int __cil_copy_node_helper(struct cil_tree_node *orig, __attribute__((unused)) uint32_t *finished, void *extra_args)
> +int __cil_copy_node_helper(struct cil_tree_node *orig, uint32_t *finished, void *extra_args)
>  {
>         int rc = SEPOL_ERR;
>         struct cil_tree_node *parent = NULL;
> @@ -2005,6 +2017,16 @@ int __cil_copy_node_helper(struct cil_tree_node *orig, __attribute__((unused)) u
>
>         rc = (*copy_func)(db, orig->data, &data, symtab);
>         if (rc == SEPOL_OK) {
> +               if (orig->flavor == CIL_MACRO && data == NULL) {
> +                       /* Skipping macro re-declaration */
> +                       if (args->orig_dest->flavor != CIL_BLOCKINHERIT) {
> +                               cil_log(CIL_ERR, "  Re-declaration of macro is only allowed when inheriting a block\n");
> +                               return SEPOL_ERR;
> +                       }
> +                       *finished = CIL_TREE_SKIP_HEAD;
> +                       return SEPOL_OK;
> +               }
> +
>                 cil_tree_node_init(&new);
>
>                 new->parent = parent;
> @@ -2013,7 +2035,15 @@ int __cil_copy_node_helper(struct cil_tree_node *orig, __attribute__((unused)) u
>                 new->flavor = orig->flavor;
>                 new->data = data;
>
> -               if (orig->flavor >= CIL_MIN_DECLARATIVE) {
> +               if (orig->flavor == CIL_BLOCK && DATUM(data)->nodes->head != NULL) {
> +                       /* Duplicate block */
> +                       if (args->orig_dest->flavor != CIL_BLOCKINHERIT) {
> +                               cil_log(CIL_ERR, "  Re-declaration of block is only allowed when inheriting a block\n");
> +                               rc = SEPOL_ERR;
> +                               goto exit;
> +                       }
> +                       cil_list_append(DATUM(new->data)->nodes, CIL_NODE, new);
> +               } else if (orig->flavor >= CIL_MIN_DECLARATIVE) {
>                         /* Check the flavor of data if was found in the destination symtab */
>                         if (DATUM(data)->nodes->head && FLAVOR(data) != orig->flavor) {
>                                 cil_tree_log(orig, CIL_ERR, "Incompatible flavor when trying to copy %s", DATUM(data)->name);
> @@ -2098,12 +2128,13 @@ int cil_copy_ast(struct cil_db *db, struct cil_tree_node *orig, struct cil_tree_
>         int rc = SEPOL_ERR;
>         struct cil_args_copy extra_args;
>
> +       extra_args.orig_dest = dest;
>         extra_args.dest = dest;
>         extra_args.db = db;
>
>         rc = cil_tree_walk(orig, __cil_copy_node_helper, NULL,  __cil_copy_last_child_helper, &extra_args);
>         if (rc != SEPOL_OK) {
> -               cil_log(CIL_INFO, "cil_tree_walk failed, rc: %d\n", rc);
> +               cil_tree_log(dest, CIL_ERR, "Failed to copy %s to %s", cil_node_to_string(orig), cil_node_to_string(dest));
>                 goto exit;
>         }
>
> --
> 2.31.1
>