Re: [PATCH 1/2] libsepol/cil: Fix name resolution involving inherited blocks

[Dominick Grift <dominick.grift@defensec.nl>]

James Carter <jwcart2@gmail.com> writes:

> When resolving a name in a block that has been inherited. First,
> a search is done in the parent namespaces (if any) of the
> blockinherit rule with the exception of the global namespace. If
> the name is not found, then a search is done in the namespaces of
> the original block (starting with that block's namespace)  with
> the exception of the global namespace. Finally, if it still has
> not been found, the global namespace is searched.
>
> This does not work if a declaration is in the block being
> inherited.
>

This example does not make sense to me

1. you shouldnt be allowed to inherit a block that is not abstracted:
(block b) does not have a (blockabstract b)

2. the a typeattribute is not associated with any types and so the
compiler would filter it out since its useless

I tried it out here and surprisingly the compiler does not fail, but no
policy ends up in the result, so I do not know how you get to those results

> For example:
>   (block b
>     (typeattribute a)
>     (allow a self (CLASS (PERM)))
>   )
>   (blockinherit b)
>
> This will result in a policy with the following identical allow
> rules:
>   (allow b.a self (CLASS (PERM)))
>   (allow b.a self (CLASS (PERM)))
> rather than the expected:
>   (allow b.a self (CLASS (PERM)))
>   (allow a self (CLASS (PERM)))
> This is because when the typeattribute is copied while resolving
> the inheritance, the new datum is added to the global namespace
> and, since that is searched last, the typeattribute in block b is
> found first.
>
> This behavior means that no declaration that is inherited into the
> global namespace will actually be used.
>
> Instead, if the name is not found in the parent namespaces (if any)
> where the blockinherit is located with the exception of the global
> namespace, start the next search in the namespace of the parent of
> the original block (instead of the original block itself). Now if
> a declaration is inherited from the original block, the new
> declaration will be used. This behavior seems to be the originally
> intended behavior because there is a comment in the code that says,
> "Continue search in original block's parent".
>
> This issue was found by secilc-fuzzer. If the original block
> is made to be abstract, then the type attribute declaration
> in the original block is not in the policy and a segfault
> occurs when creating the binary because the copied allow rule
> refers to a non-existent type attribute.
>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/src/cil_resolve_ast.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index f251ed15..55080396 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -4211,7 +4211,7 @@ static int __cil_resolve_name_with_parents(struct cil_tree_node *node, char *nam
>  			rc = __cil_resolve_name_with_parents(node->parent, name, sym_index, datum);
>  			if (rc != SEPOL_OK) {
>  				/* Continue search in original block's parent */
> -				rc = __cil_resolve_name_with_parents(NODE(inherit->block), name, sym_index, datum);
> +				rc = __cil_resolve_name_with_parents(NODE(inherit->block)->parent, name, sym_index, datum);
>  				goto exit;
>  			}
>  		}

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