[PATCH 2/2] LSM: SafeSetID: gate setgid transitions

[Micah Morton]

On Sun, Feb 17, 2019 at 10:49 AM Serge E. Hallyn <serge at hallyn.com> wrote:
>
> On Fri, Feb 15, 2019 at 02:22:28PM -0800, mortonm at chromium.org wrote:
> > From: Micah Morton <mortonm at chromium.org>
> >
> > The SafeSetID LSM already gates setuid transitions for UIDs on the
> > system whose use of CAP_SETUID has been 'restricted'. This patch
> > implements the analogous functionality for setgid transitions, in order
> > to restrict the use of CAP_SETGID for certain UIDs on the system. One
> > notable consequence of this addition is that a process running under a
> > restricted UID (i.e. one that is only allowed to setgid to certain
> > approved GIDs) will not be allowed to call the setgroups() syscall to
> > set its supplementary group IDs. For now, we leave such support for
> > restricted setgroups() to future work, as it would require hooking the
> > logic in setgroups() and verifying that the array of GIDs passed in from
> > userspace only consists of approved GIDs.
> >
> > Signed-off-by: Micah Morton <mortonm at chromium.org>
> > ---
> > Tested with slight mod to test in tools/testing/selftests/safesetid for
> > testing setgid as well as setuid.
> >
> >  security/safesetid/lsm.c        | 263 +++++++++++++++++++++++++++-----
> >  security/safesetid/lsm.h        |  11 +-
> >  security/safesetid/securityfs.c | 105 +++++++++----
> >  3 files changed, 307 insertions(+), 72 deletions(-)
> >
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index cecd38e2ac80..5d9710b7bb04 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -26,27 +26,30 @@ int safesetid_initialized;
> >
> >  #define NUM_BITS 8 /* 128 buckets in hash table */
> ...
> > +int add_safesetid_whitelist_uid_entry(kuid_t parent, kuid_t child)
> >  {
> > -     struct entry *new;
> > +     struct id_entry *new;
> >
> >       /* Return if entry already exists */
> >       if (check_setuid_policy_hashtable_key_value(parent, child))
> >               return 0;
> >
> > -     new = kzalloc(sizeof(struct entry), GFP_KERNEL);
> > +     new = kzalloc(sizeof(struct id_entry), GFP_KERNEL);
> > +     if (!new)
> > +             return -ENOMEM;
> > +     new->parent_kuid = __kuid_val(parent);
> > +     new->child_kid = __kuid_val(child);
> > +     spin_lock(&safesetid_whitelist_uid_hashtable_spinlock);
> > +     hash_add_rcu(safesetid_whitelist_uid_hashtable,
> > +                  &new->next,
> > +                  __kuid_val(parent));
>
> Do you care at all about the possibility of duplicate entries?

Duplicate entries shouldn't be possible due to the invocation of
check_setuid_policy_hashtable_key_value() above where it says "Return
if entry already exists". Does this make sense?

>
> > +     spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> > +     return 0;
> > +}