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

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

[James Carter]

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.

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;
 			}
 		}
-- 
2.26.3

[PATCH 2/2] secilc/docs: Document the order that inherited rules are resolved in

[James Carter]

In the blockinherit section of the CIL documentation clearly state
the order in which inherited rules are resolved.

That order is:

1) The parent namespaces (if any) where the blockinherit rule is
   located with the exception of the global namespace.

2) The parent namespaces of the block being inherited (but not that
   block's namespace) with the exception of the global namespace.

3) The global namespace.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 secilc/docs/cil_container_statements.md | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/secilc/docs/cil_container_statements.md b/secilc/docs/cil_container_statements.md
index 7a7f67cc..41a4612c 100644
--- a/secilc/docs/cil_container_statements.md
+++ b/secilc/docs/cil_container_statements.md
@@ -103,6 +103,14 @@ blockinherit
 
 Used to add common policy rules to the current namespace via a template that has been defined with the [`blockabstract`](cil_container_statements.md#blockabstract) statement. All [`blockinherit`](cil_container_statements.md#blockinherit) statements are resolved first and then the contents of the block are copied. This is so that inherited blocks will not be inherited. For a concrete example, please see the examples section.
 
+Inherited rules are resolved by searching namespaces in the following order:
+
+-  The parent namespaces (if any) where the [`blockinherit`](cil_container_statements.md#blockinherit) rule is located with the exception of the global namespace.
+
+-  The parent namespaces of the block being inherited (but not that block's namespace) with the exception of the global namespace.
+
+-  The global namespace.
+
 Not allowed in [`macro`](cil_call_macro_statements.md#macro) blocks.
 
 **Statement definition:**
-- 
2.26.3

Re: [PATCH 2/2] secilc/docs: Document the order that inherited rules are resolved in

[Petr Lautrbach]

James Carter <jwcart2@gmail.com> writes:

> In the blockinherit section of the CIL documentation clearly state
> the order in which inherited rules are resolved.
>
> That order is:
>
> 1) The parent namespaces (if any) where the blockinherit rule is
>    located with the exception of the global namespace.
>
> 2) The parent namespaces of the block being inherited (but not that
>    block's namespace) with the exception of the global namespace.
>
> 3) The global namespace.
>
> Signed-off-by: James Carter <jwcart2@gmail.com>

Both
Acked-by: Petr Lautrbach <plautrba@redhat.com>

and merged.

Thanks!


> ---
>  secilc/docs/cil_container_statements.md | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/secilc/docs/cil_container_statements.md b/secilc/docs/cil_container_statements.md
> index 7a7f67cc..41a4612c 100644
> --- a/secilc/docs/cil_container_statements.md
> +++ b/secilc/docs/cil_container_statements.md
> @@ -103,6 +103,14 @@ blockinherit
>  
>  Used to add common policy rules to the current namespace via a template that has been defined with the [`blockabstract`](cil_container_statements.md#blockabstract) statement. All [`blockinherit`](cil_container_statements.md#blockinherit) statements are resolved first and then the contents of the block are copied. This is so that inherited blocks will not be inherited. For a concrete example, please see the examples section.
>  
> +Inherited rules are resolved by searching namespaces in the following order:
> +
> +-  The parent namespaces (if any) where the [`blockinherit`](cil_container_statements.md#blockinherit) rule is located with the exception of the global namespace.
> +
> +-  The parent namespaces of the block being inherited (but not that block's namespace) with the exception of the global namespace.
> +
> +-  The global namespace.
> +
>  Not allowed in [`macro`](cil_call_macro_statements.md#macro) blocks.
>  
>  **Statement definition:**
> -- 
> 2.26.3

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

[Dominick Grift]

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

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

[James Carter]

On Thu, May 6, 2021 at 1:37 PM Dominick Grift
<dominick.grift@defensec.nl> wrote:
>
> 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)
>

You have always been allowed to inherit any block. That is by design.
The blockinherit just means that that block won't show up in policy if
nothing inherits it.

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

You are right that the binary policy will not have either of the rules
because no types have been assigned to the attribute. It still
resolves as I stated and that can be seen by using secil2tree. It
would have been better to use a type declaration instead, but I used
typeattribute because that was what the fuzzer had used.

Thanks,
Jim

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