[PATCH 09/10] LSM: SafeSetID: verify transitive constrainedness

[Jann Horn]

On Wed, Apr 10, 2019 at 7:28 PM Kees Cook <keescook at chromium.org> wrote:
> On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm at chromium.org> wrote:
> > From: Jann Horn <jannh at google.com>
> >
> > Someone might write a ruleset like the following, expecting that it
> > securely constrains UID 1 to UIDs 1, 2 and 3:
> >
> >     1:2
> >     1:3
> >
> > However, because no constraints are applied to UIDs 2 and 3, an attacker
> > with UID 1 can simply first switch to UID 2, then switch to any UID from
> > there. The secure way to write this ruleset would be:
> >
> >     1:2
> >     1:3
> >     2:2
> >     3:3
> >
> > , which uses "transition to self" as a way to inhibit the default-allow
> > policy without allowing anything specific.
> >
> > This is somewhat unintuitive. To make sure that policy authors don't
> > accidentally write insecure policies because of this, let the kernel verify
> > that a new ruleset does not contain any entries that are constrained, but
> > transitively unconstrained.
> >
> > Signed-off-by: Jann Horn <jannh at google.com>
> > Signed-off-by: Micah Morton <mortonm at chromium.org>
> > ---
> >  security/safesetid/securityfs.c               | 21 +++++++++++++++++++
> >  .../selftests/safesetid/safesetid-test.c      |  4 +++-
> >  2 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 7a08fff2bc14..3ec64487f0e9 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -77,6 +77,23 @@ static void release_ruleset(struct setuid_ruleset *pol)
> >                 call_rcu(&pol->rcu, __release_ruleset);
> >  }
> >
> > +static int verify_ruleset(struct setuid_ruleset *pol)
> > +{
> > +       int bucket;
> > +       struct setuid_rule *rule;
> > +
> > +       hash_for_each(pol->rules, bucket, rule, next) {
> > +               if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
> > +                   SIDPOL_DEFAULT) {
> > +                       pr_warn("insecure policy rejected: uid %d is constrained but transitively unconstrained through uid %d\n",
> > +                               __kuid_val(rule->src_uid),
> > +                               __kuid_val(rule->dst_uid));
> > +                       return -EINVAL;
> > +               }
> > +       }
> > +       return 0;
> > +}
>
> Why fail open? How about having the policy automatically add the
> constraints (since the entire policy is known at verification time)?

Are you suggesting to print a warning, automatically add the
constraint, apply the policy, and still return -EINVAL? I guess that
would work.

I think it is a good idea to require userspace to explicitly write
these rules, since implicitly-added rules might make the rules harder
to understand for someone reading a policy file.