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

[Micah Morton <mortonm@chromium.org>]

Oh I see, good point. Right now there's no harm in having duplicate
entries, but it could be a problem later if certain functionality were
added (e.g. deletion of individual policy entries). Might as well fix
it. Uploading a v2 patch.

On Tue, Feb 19, 2019 at 10:26 AM Serge E. Hallyn <serge@hallyn.com> wrote:
>
> On Tue, Feb 19, 2019 at 09:04:10AM -0800, Micah Morton wrote:
> > On Sun, Feb 17, 2019 at 10:49 AM Serge E. Hallyn <serge@hallyn.com> wrote:
> > >
> > > On Fri, Feb 15, 2019 at 02:22:28PM -0800, mortonm@chromium.org wrote:
> > > > From: Micah Morton <mortonm@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@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?
>
> I don't believe it does, because you do the check before you lock.  So
> two tasks can race.
>
> Obviously you can't do the malloc under the spinlock, but I think you
> will need to check for an existing entry once, do the malloc, lock,
> then check again for an existing entry, then free the alloced
> 'new' if found.
>
> > > > +     spin_unlock(&safesetid_whitelist_uid_hashtable_spinlock);
> > > > +     return 0;
> > > > +}