[03/10] LSM: SafeSetID: refactor policy hash table
diff mbox series

Message ID 20190410165534.210333-1-mortonm@chromium.org
State New
Headers show
Series
  • [01/10] LSM: SafeSetID: fix pr_warn() to include newline
Related show

Commit Message

Micah Morton April 10, 2019, 4:55 p.m. UTC
From: Jann Horn <jannh@google.com>

parent_kuid and child_kuid are kuids, there is no reason to make them
uint64_t. (And anyway, in the kernel, the normal name for that would be
u64, not uint64_t.)

check_setuid_policy_hashtable_key() and
check_setuid_policy_hashtable_key_value() are basically the same thing,
merge them.

Also fix the comment that claimed that (1<<8)==128.

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
 security/safesetid/lsm.c | 62 ++++++++++++----------------------------
 security/safesetid/lsm.h | 19 ++++++++++++
 2 files changed, 37 insertions(+), 44 deletions(-)

Comments

Kees Cook April 10, 2019, 5:12 p.m. UTC | #1
On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> parent_kuid and child_kuid are kuids, there is no reason to make them
> uint64_t. (And anyway, in the kernel, the normal name for that would be
> u64, not uint64_t.)
>
> check_setuid_policy_hashtable_key() and
> check_setuid_policy_hashtable_key_value() are basically the same thing,
> merge them.
>
> Also fix the comment that claimed that (1<<8)==128.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  security/safesetid/lsm.c | 62 ++++++++++++----------------------------
>  security/safesetid/lsm.h | 19 ++++++++++++
>  2 files changed, 37 insertions(+), 44 deletions(-)
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index 5310fcf3052a..15cd13b5a211 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -14,67 +14,40 @@
>
>  #define pr_fmt(fmt) "SafeSetID: " fmt
>
> -#include <linux/hashtable.h>
>  #include <linux/lsm_hooks.h>
>  #include <linux/module.h>
>  #include <linux/ptrace.h>
>  #include <linux/sched/task_stack.h>
>  #include <linux/security.h>
> +#include "lsm.h"
>
>  /* Flag indicating whether initialization completed */
>  int safesetid_initialized;
>
> -#define NUM_BITS 8 /* 128 buckets in hash table */
> +#define NUM_BITS 8 /* 256 buckets in hash table */
>
>  static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
>
> -/*
> - * Hash table entry to store safesetid policy signifying that 'parent' user
> - * can setid to 'child' user.
> - */
> -struct entry {
> -       struct hlist_node next;
> -       struct hlist_node dlist; /* for deletion cleanup */
> -       uint64_t parent_kuid;
> -       uint64_t child_kuid;
> -};
> -
>  static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
>
> -static bool check_setuid_policy_hashtable_key(kuid_t parent)
> +static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
>  {
>         struct entry *entry;
> +       enum sid_policy_type result = SIDPOL_DEFAULT;
>
>         rcu_read_lock();
>         hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> -                                  entry, next, __kuid_val(parent)) {
> -               if (entry->parent_kuid == __kuid_val(parent)) {
> +                                  entry, next, __kuid_val(src)) {
> +               if (!uid_eq(entry->src_uid, src))
> +                       continue;
> +               if (uid_eq(entry->dst_uid, dst)) {
>                         rcu_read_unlock();
> -                       return true;
> +                       return SIDPOL_ALLOWED;
>                 }
> +               result = SIDPOL_CONSTRAINED;
>         }
>         rcu_read_unlock();
> -
> -       return false;
> -}
> -
> -static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> -                                                   kuid_t child)
> -{
> -       struct entry *entry;
> -
> -       rcu_read_lock();
> -       hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> -                                  entry, next, __kuid_val(parent)) {
> -               if (entry->parent_kuid == __kuid_val(parent) &&
> -                   entry->child_kuid == __kuid_val(child)) {
> -                       rcu_read_unlock();
> -                       return true;
> -               }
> -       }
> -       rcu_read_unlock();
> -
> -       return false;
> +       return result;
>  }
>
>  static int safesetid_security_capable(const struct cred *cred,
> @@ -83,7 +56,7 @@ static int safesetid_security_capable(const struct cred *cred,
>                                       unsigned int opts)
>  {
>         if (cap == CAP_SETUID &&
> -           check_setuid_policy_hashtable_key(cred->uid)) {
> +           setuid_policy_lookup(cred->uid, INVALID_UID) != SIDPOL_DEFAULT) {
>                 if (!(opts & CAP_OPT_INSETID)) {
>                         /*
>                          * Deny if we're not in a set*uid() syscall to avoid
> @@ -116,7 +89,8 @@ static bool uid_permitted_for_cred(const struct cred *old, kuid_t new_uid)
>          * Transitions to new UIDs require a check against the policy of the old
>          * RUID.
>          */
> -       permitted = check_setuid_policy_hashtable_key_value(old->uid, new_uid);
> +       permitted =
> +           setuid_policy_lookup(old->uid, new_uid) != SIDPOL_CONSTRAINED;
>         if (!permitted) {
>                 pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n",
>                         __kuid_val(old->uid), __kuid_val(old->euid),
> @@ -136,7 +110,7 @@ static int safesetid_task_fix_setuid(struct cred *new,
>  {
>
>         /* Do nothing if there are no setuid restrictions for our old RUID. */
> -       if (!check_setuid_policy_hashtable_key(old->uid))
> +       if (setuid_policy_lookup(old->uid, INVALID_UID) == SIDPOL_DEFAULT)
>                 return 0;
>
>         if (uid_permitted_for_cred(old, new->uid) &&
> @@ -159,14 +133,14 @@ int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
>         struct entry *new;
>
>         /* Return if entry already exists */
> -       if (check_setuid_policy_hashtable_key_value(parent, child))
> +       if (setuid_policy_lookup(parent, child) == SIDPOL_ALLOWED)
>                 return 0;
>
>         new = kzalloc(sizeof(struct entry), GFP_KERNEL);
>         if (!new)
>                 return -ENOMEM;
> -       new->parent_kuid = __kuid_val(parent);
> -       new->child_kuid = __kuid_val(child);
> +       new->src_uid = parent;
> +       new->dst_uid = child;
>         spin_lock(&safesetid_whitelist_hashtable_spinlock);
>         hash_add_rcu(safesetid_whitelist_hashtable,
>                      &new->next,
> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> index c1ea3c265fcf..6806f902794c 100644
> --- a/security/safesetid/lsm.h
> +++ b/security/safesetid/lsm.h
> @@ -15,6 +15,8 @@
>  #define _SAFESETID_H
>
>  #include <linux/types.h>
> +#include <linux/uidgid.h>
> +#include <linux/hashtable.h>
>
>  /* Flag indicating whether initialization completed */
>  extern int safesetid_initialized;
> @@ -25,6 +27,23 @@ enum safesetid_whitelist_file_write_type {
>         SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
>  };
>
> +enum sid_policy_type {
> +       SIDPOL_DEFAULT, /* source ID is unaffected by policy */
> +       SIDPOL_CONSTRAINED, /* source ID is affected by policy */
> +       SIDPOL_ALLOWED /* target ID explicitly allowed */
> +};
> +
> +/*
> + * Hash table entry to store safesetid policy signifying that 'src_uid'
> + * can setid to 'dst_uid'.
> + */
> +struct entry {
> +       struct hlist_node next;
> +       struct hlist_node dlist; /* for deletion cleanup */
> +       kuid_t src_uid;
> +       kuid_t dst_uid;
> +};
> +
>  /* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
>  int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
>
> --
> 2.21.0.392.gf8f6787159e-goog
>
Micah Morton May 7, 2019, 3:01 p.m. UTC | #2
Ready for merge.

On Wed, Apr 10, 2019 at 10:13 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > parent_kuid and child_kuid are kuids, there is no reason to make them
> > uint64_t. (And anyway, in the kernel, the normal name for that would be
> > u64, not uint64_t.)
> >
> > check_setuid_policy_hashtable_key() and
> > check_setuid_policy_hashtable_key_value() are basically the same thing,
> > merge them.
> >
> > Also fix the comment that claimed that (1<<8)==128.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> >  security/safesetid/lsm.c | 62 ++++++++++++----------------------------
> >  security/safesetid/lsm.h | 19 ++++++++++++
> >  2 files changed, 37 insertions(+), 44 deletions(-)
> >
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index 5310fcf3052a..15cd13b5a211 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -14,67 +14,40 @@
> >
> >  #define pr_fmt(fmt) "SafeSetID: " fmt
> >
> > -#include <linux/hashtable.h>
> >  #include <linux/lsm_hooks.h>
> >  #include <linux/module.h>
> >  #include <linux/ptrace.h>
> >  #include <linux/sched/task_stack.h>
> >  #include <linux/security.h>
> > +#include "lsm.h"
> >
> >  /* Flag indicating whether initialization completed */
> >  int safesetid_initialized;
> >
> > -#define NUM_BITS 8 /* 128 buckets in hash table */
> > +#define NUM_BITS 8 /* 256 buckets in hash table */
> >
> >  static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
> >
> > -/*
> > - * Hash table entry to store safesetid policy signifying that 'parent' user
> > - * can setid to 'child' user.
> > - */
> > -struct entry {
> > -       struct hlist_node next;
> > -       struct hlist_node dlist; /* for deletion cleanup */
> > -       uint64_t parent_kuid;
> > -       uint64_t child_kuid;
> > -};
> > -
> >  static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
> >
> > -static bool check_setuid_policy_hashtable_key(kuid_t parent)
> > +static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
> >  {
> >         struct entry *entry;
> > +       enum sid_policy_type result = SIDPOL_DEFAULT;
> >
> >         rcu_read_lock();
> >         hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> > -                                  entry, next, __kuid_val(parent)) {
> > -               if (entry->parent_kuid == __kuid_val(parent)) {
> > +                                  entry, next, __kuid_val(src)) {
> > +               if (!uid_eq(entry->src_uid, src))
> > +                       continue;
> > +               if (uid_eq(entry->dst_uid, dst)) {
> >                         rcu_read_unlock();
> > -                       return true;
> > +                       return SIDPOL_ALLOWED;
> >                 }
> > +               result = SIDPOL_CONSTRAINED;
> >         }
> >         rcu_read_unlock();
> > -
> > -       return false;
> > -}
> > -
> > -static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> > -                                                   kuid_t child)
> > -{
> > -       struct entry *entry;
> > -
> > -       rcu_read_lock();
> > -       hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> > -                                  entry, next, __kuid_val(parent)) {
> > -               if (entry->parent_kuid == __kuid_val(parent) &&
> > -                   entry->child_kuid == __kuid_val(child)) {
> > -                       rcu_read_unlock();
> > -                       return true;
> > -               }
> > -       }
> > -       rcu_read_unlock();
> > -
> > -       return false;
> > +       return result;
> >  }
> >
> >  static int safesetid_security_capable(const struct cred *cred,
> > @@ -83,7 +56,7 @@ static int safesetid_security_capable(const struct cred *cred,
> >                                       unsigned int opts)
> >  {
> >         if (cap == CAP_SETUID &&
> > -           check_setuid_policy_hashtable_key(cred->uid)) {
> > +           setuid_policy_lookup(cred->uid, INVALID_UID) != SIDPOL_DEFAULT) {
> >                 if (!(opts & CAP_OPT_INSETID)) {
> >                         /*
> >                          * Deny if we're not in a set*uid() syscall to avoid
> > @@ -116,7 +89,8 @@ static bool uid_permitted_for_cred(const struct cred *old, kuid_t new_uid)
> >          * Transitions to new UIDs require a check against the policy of the old
> >          * RUID.
> >          */
> > -       permitted = check_setuid_policy_hashtable_key_value(old->uid, new_uid);
> > +       permitted =
> > +           setuid_policy_lookup(old->uid, new_uid) != SIDPOL_CONSTRAINED;
> >         if (!permitted) {
> >                 pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n",
> >                         __kuid_val(old->uid), __kuid_val(old->euid),
> > @@ -136,7 +110,7 @@ static int safesetid_task_fix_setuid(struct cred *new,
> >  {
> >
> >         /* Do nothing if there are no setuid restrictions for our old RUID. */
> > -       if (!check_setuid_policy_hashtable_key(old->uid))
> > +       if (setuid_policy_lookup(old->uid, INVALID_UID) == SIDPOL_DEFAULT)
> >                 return 0;
> >
> >         if (uid_permitted_for_cred(old, new->uid) &&
> > @@ -159,14 +133,14 @@ int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
> >         struct entry *new;
> >
> >         /* Return if entry already exists */
> > -       if (check_setuid_policy_hashtable_key_value(parent, child))
> > +       if (setuid_policy_lookup(parent, child) == SIDPOL_ALLOWED)
> >                 return 0;
> >
> >         new = kzalloc(sizeof(struct entry), GFP_KERNEL);
> >         if (!new)
> >                 return -ENOMEM;
> > -       new->parent_kuid = __kuid_val(parent);
> > -       new->child_kuid = __kuid_val(child);
> > +       new->src_uid = parent;
> > +       new->dst_uid = child;
> >         spin_lock(&safesetid_whitelist_hashtable_spinlock);
> >         hash_add_rcu(safesetid_whitelist_hashtable,
> >                      &new->next,
> > diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> > index c1ea3c265fcf..6806f902794c 100644
> > --- a/security/safesetid/lsm.h
> > +++ b/security/safesetid/lsm.h
> > @@ -15,6 +15,8 @@
> >  #define _SAFESETID_H
> >
> >  #include <linux/types.h>
> > +#include <linux/uidgid.h>
> > +#include <linux/hashtable.h>
> >
> >  /* Flag indicating whether initialization completed */
> >  extern int safesetid_initialized;
> > @@ -25,6 +27,23 @@ enum safesetid_whitelist_file_write_type {
> >         SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
> >  };
> >
> > +enum sid_policy_type {
> > +       SIDPOL_DEFAULT, /* source ID is unaffected by policy */
> > +       SIDPOL_CONSTRAINED, /* source ID is affected by policy */
> > +       SIDPOL_ALLOWED /* target ID explicitly allowed */
> > +};
> > +
> > +/*
> > + * Hash table entry to store safesetid policy signifying that 'src_uid'
> > + * can setid to 'dst_uid'.
> > + */
> > +struct entry {
> > +       struct hlist_node next;
> > +       struct hlist_node dlist; /* for deletion cleanup */
> > +       kuid_t src_uid;
> > +       kuid_t dst_uid;
> > +};
> > +
> >  /* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
> >  int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
> >
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook

Patch
diff mbox series

diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index 5310fcf3052a..15cd13b5a211 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -14,67 +14,40 @@ 
 
 #define pr_fmt(fmt) "SafeSetID: " fmt
 
-#include <linux/hashtable.h>
 #include <linux/lsm_hooks.h>
 #include <linux/module.h>
 #include <linux/ptrace.h>
 #include <linux/sched/task_stack.h>
 #include <linux/security.h>
+#include "lsm.h"
 
 /* Flag indicating whether initialization completed */
 int safesetid_initialized;
 
-#define NUM_BITS 8 /* 128 buckets in hash table */
+#define NUM_BITS 8 /* 256 buckets in hash table */
 
 static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
 
-/*
- * Hash table entry to store safesetid policy signifying that 'parent' user
- * can setid to 'child' user.
- */
-struct entry {
-	struct hlist_node next;
-	struct hlist_node dlist; /* for deletion cleanup */
-	uint64_t parent_kuid;
-	uint64_t child_kuid;
-};
-
 static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
 
-static bool check_setuid_policy_hashtable_key(kuid_t parent)
+static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
 {
 	struct entry *entry;
+	enum sid_policy_type result = SIDPOL_DEFAULT;
 
 	rcu_read_lock();
 	hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
-				   entry, next, __kuid_val(parent)) {
-		if (entry->parent_kuid == __kuid_val(parent)) {
+				   entry, next, __kuid_val(src)) {
+		if (!uid_eq(entry->src_uid, src))
+			continue;
+		if (uid_eq(entry->dst_uid, dst)) {
 			rcu_read_unlock();
-			return true;
+			return SIDPOL_ALLOWED;
 		}
+		result = SIDPOL_CONSTRAINED;
 	}
 	rcu_read_unlock();
-
-	return false;
-}
-
-static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
-						    kuid_t child)
-{
-	struct entry *entry;
-
-	rcu_read_lock();
-	hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
-				   entry, next, __kuid_val(parent)) {
-		if (entry->parent_kuid == __kuid_val(parent) &&
-		    entry->child_kuid == __kuid_val(child)) {
-			rcu_read_unlock();
-			return true;
-		}
-	}
-	rcu_read_unlock();
-
-	return false;
+	return result;
 }
 
 static int safesetid_security_capable(const struct cred *cred,
@@ -83,7 +56,7 @@  static int safesetid_security_capable(const struct cred *cred,
 				      unsigned int opts)
 {
 	if (cap == CAP_SETUID &&
-	    check_setuid_policy_hashtable_key(cred->uid)) {
+	    setuid_policy_lookup(cred->uid, INVALID_UID) != SIDPOL_DEFAULT) {
 		if (!(opts & CAP_OPT_INSETID)) {
 			/*
 			 * Deny if we're not in a set*uid() syscall to avoid
@@ -116,7 +89,8 @@  static bool uid_permitted_for_cred(const struct cred *old, kuid_t new_uid)
 	 * Transitions to new UIDs require a check against the policy of the old
 	 * RUID.
 	 */
-	permitted = check_setuid_policy_hashtable_key_value(old->uid, new_uid);
+	permitted =
+	    setuid_policy_lookup(old->uid, new_uid) != SIDPOL_CONSTRAINED;
 	if (!permitted) {
 		pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n",
 			__kuid_val(old->uid), __kuid_val(old->euid),
@@ -136,7 +110,7 @@  static int safesetid_task_fix_setuid(struct cred *new,
 {
 
 	/* Do nothing if there are no setuid restrictions for our old RUID. */
-	if (!check_setuid_policy_hashtable_key(old->uid))
+	if (setuid_policy_lookup(old->uid, INVALID_UID) == SIDPOL_DEFAULT)
 		return 0;
 
 	if (uid_permitted_for_cred(old, new->uid) &&
@@ -159,14 +133,14 @@  int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
 	struct entry *new;
 
 	/* Return if entry already exists */
-	if (check_setuid_policy_hashtable_key_value(parent, child))
+	if (setuid_policy_lookup(parent, child) == SIDPOL_ALLOWED)
 		return 0;
 
 	new = kzalloc(sizeof(struct entry), GFP_KERNEL);
 	if (!new)
 		return -ENOMEM;
-	new->parent_kuid = __kuid_val(parent);
-	new->child_kuid = __kuid_val(child);
+	new->src_uid = parent;
+	new->dst_uid = child;
 	spin_lock(&safesetid_whitelist_hashtable_spinlock);
 	hash_add_rcu(safesetid_whitelist_hashtable,
 		     &new->next,
diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
index c1ea3c265fcf..6806f902794c 100644
--- a/security/safesetid/lsm.h
+++ b/security/safesetid/lsm.h
@@ -15,6 +15,8 @@ 
 #define _SAFESETID_H
 
 #include <linux/types.h>
+#include <linux/uidgid.h>
+#include <linux/hashtable.h>
 
 /* Flag indicating whether initialization completed */
 extern int safesetid_initialized;
@@ -25,6 +27,23 @@  enum safesetid_whitelist_file_write_type {
 	SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
 };
 
+enum sid_policy_type {
+	SIDPOL_DEFAULT, /* source ID is unaffected by policy */
+	SIDPOL_CONSTRAINED, /* source ID is affected by policy */
+	SIDPOL_ALLOWED /* target ID explicitly allowed */
+};
+
+/*
+ * Hash table entry to store safesetid policy signifying that 'src_uid'
+ * can setid to 'dst_uid'.
+ */
+struct entry {
+	struct hlist_node next;
+	struct hlist_node dlist; /* for deletion cleanup */
+	kuid_t src_uid;
+	kuid_t dst_uid;
+};
+
 /* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
 int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);