* [PATCH 03/10] LSM: SafeSetID: refactor policy hash table
@ 2019-04-10 16:55 Micah Morton
2019-04-10 17:12 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Micah Morton @ 2019-04-10 16:55 UTC (permalink / raw)
To: jmorris, keescook, casey, linux-security-module; +Cc: Jann Horn, Micah Morton
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 03/10] LSM: SafeSetID: refactor policy hash table
2019-04-10 16:55 [PATCH 03/10] LSM: SafeSetID: refactor policy hash table Micah Morton
@ 2019-04-10 17:12 ` Kees Cook
2019-05-07 15:01 ` Micah Morton
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2019-04-10 17:12 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 03/10] LSM: SafeSetID: refactor policy hash table
2019-04-10 17:12 ` Kees Cook
@ 2019-05-07 15:01 ` Micah Morton
0 siblings, 0 replies; 3+ messages in thread
From: Micah Morton @ 2019-05-07 15:01 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-05-07 15:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-10 16:55 [PATCH 03/10] LSM: SafeSetID: refactor policy hash table Micah Morton
2019-04-10 17:12 ` Kees Cook
2019-05-07 15:01 ` Micah Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).