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;
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.
Thanks,
Jim
> --
> Ondrej Mosnacek
> Software Engineer, Linux Security - SELinux kernel
> Red Hat, Inc.
>
