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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 6, 2021 at 1:37 PM Dominick Grift
<dominick.grift@xxxxxxxxxxx> wrote:
>
> James Carter <jwcart2@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxx
> Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
> https://sks-keyservers.net/pks/lookup?op=get&search=0xDA7E521F10F64098
> Dominick Grift



[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux