Re: [PATCH userspace v2 2/2] libsepol,checkpolicy: add support for self keyword in type transitions

[Ondrej Mosnacek <omosnace@redhat.com>]

On Thu, Apr 28, 2022 at 9:23 PM James Carter <jwcart2@gmail.com> wrote:
> On Wed, Apr 27, 2022 at 6:14 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > With the addition of the anon_inode class in the kernel, 'self'
> > transition rules became useful, but haven't been implemented.
> >
> > This patch implements the self keyword in all 'typetransition'
> > statements at the TE language level and adds the support to the module
> > policydb format. Note that changing the kernel policydb format is not
> > necessary at all, as type transitions are always expanded in the kernel
> > policydb.
> >
>
> These patches also cause "self" to be allowed in type change and type
> member rules as well. That is fine, but it should be stated.

Good point, I'll update the commit messages.

[...]
> > diff --git a/libsepol/src/write.c b/libsepol/src/write.c
> > index d7ac2b25..5f593c1d 100644
> > --- a/libsepol/src/write.c
> > +++ b/libsepol/src/write.c
> > @@ -1929,11 +1929,12 @@ static int role_allow_rule_write(role_allow_rule_t * r, struct policy_file *fp)
> >         return POLICYDB_SUCCESS;
> >  }
> >
> > -static int filename_trans_rule_write(filename_trans_rule_t * t, struct policy_file *fp)
> > +static int filename_trans_rule_write(policydb_t *p, filename_trans_rule_t *t,
> > +                                    struct policy_file *fp)
> >  {
> >         int nel = 0;
> > -       size_t items;
> > -       uint32_t buf[2], len;
> > +       size_t items, entries;
> > +       uint32_t buf[3], len;
> >         filename_trans_rule_t *ftr;
> >
> >         for (ftr = t; ftr; ftr = ftr->next)
>
>
> The flags field needs to be cleared if the policy version being
> written is < MOD_POLICYDB_VERSION_SELF_TYPETRANS.
>
> Something like:
> +        if ((p->policyvers < MOD_POLICYDB_VERSION_SELF_TYPETRANS) &&
> +            (ftr->flags & RULE_SELF)) {
> +            WARN(fp->handle, "Module policy version %u does not support using "
> +                 "\"self\" in a filename type transition rule", p->policyvers);
> +            ftr->flags &= ~RULE_SELF;
> +        }
>
> A similar check is needed in avrule_write(), but there needs to be a
> check that the rule is an AVRULE_TYPE as well.

I agree we need to handle this case, but should we really just discard
the flag? It seems that currently avrule->ttypes is empty if
(avrule->flags & RULE_SELF) is set, so we would create an empty (or
maybe even invalid?) rule in the policy module. Setting ttypes =
stypes would not be equivalent and expanding the rule also doesn't
seem like a great idea. IMHO we should just return an error on any
attempt to downgrade a module containing self AVRULE_TYPE rules below
MOD_POLICYDB_VERSION_SELF_TYPETRANS.

What do you think?

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.