On Fri, May 6, 2022 at 5:42 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> On Fri, May 6, 2022 at 9:49 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > On Thu, Apr 28, 2022 at 9:23 PM James Carter <jwcart2@xxxxxxxxx> wrote:
> > > On Wed, Apr 27, 2022 at 6:14 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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?
> >
>
> avrule->ttypes is not necessarily empty when self is used. I didn't
> realize that myself until recently.
> It is possible to have a (very ugly) rule like:
> type_transition t1 { t2 attr1 self }:file t3;
Ah, so it actually extends the target type list instead of overriding
it... Yes, looking at the code more carefully I can see that it indeed
matches this semantic.
> Ideally, it would be nice to get rid of the empty rules when writing
> out the policy, but the number of rules has already been determined so
> there would be extra work involved. Since it doesn't cause any
> problems (that I have found) and it should be unusual to downgrade the
> policy version anyway, writing the empty rule seems like the best
> choice.
>
> We definitely can't expand the rule because we might not have all of
> the information for an attribute when we are writing a module's
> policy.
>
> Returning an error is fine. We do that when we encounter xperm rules.
> We just discard the rule for default_range rules using GLBLUB,
> permissive types, boolean and conditional rules, and filename
> typetransition rules when the policy version doesn't support them. An
> argument can be made for either behavior.
It would be against my conscience to post a patch that drops rules
from the policy with "only" a warning, so I'll opt for returning an
error :)
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
