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 [off-list ref] wrote:

quoted

On Fri, Feb 15, 2019 at 02:22:28PM -0800, mortonm@chromium.org wrote:

quoted

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 */

...

quoted

+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.

quoted

quoted

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