On Fri, Aug 13, 2021 at 1:52 PM James Carter <jwcart2@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
>
