On Tue, Aug 17, 2021 at 5:05 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> On Tue, Aug 17, 2021 at 4:31 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
> >
> > 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>
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
> > >
> >
