* [PATCH 03/10] LSM: SafeSetID: refactor policy hash table
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
* [PATCH 04/10] LSM: SafeSetID: refactor safesetid_security_capable()
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>
At the moment, safesetid_security_capable() has two nested conditional
blocks, and one big comment for all the logic. Chop it up and reduce the
amount of indentation.
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
security/safesetid/lsm.c | 41 +++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index 15cd13b5a211..ab429e1816c5 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -55,21 +55,32 @@ static int safesetid_security_capable(const struct cred *cred,
int cap,
unsigned int opts)
{
- if (cap == CAP_SETUID &&
- 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
- * giving powers gated by CAP_SETUID that are related
- * to functionality other than calling set*uid() (e.g.
- * allowing user to set up userns uid mappings).
- */
- pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
- __kuid_val(cred->uid));
- return -1;
- }
- }
- return 0;
+ /* We're only interested in CAP_SETUID. */
+ if (cap != CAP_SETUID)
+ return 0;
+
+ /*
+ * If CAP_SETUID is currently used for a set*uid() syscall, we want to
+ * let it go through here; the real security check happens later, in the
+ * task_fix_setuid hook.
+ */
+ if ((opts & CAP_OPT_INSETID) != 0)
+ return 0;
+
+ /*
+ * If no policy applies to this task, allow the use of CAP_SETUID for
+ * other purposes.
+ */
+ if (setuid_policy_lookup(cred->uid, INVALID_UID) == SIDPOL_DEFAULT)
+ return 0;
+
+ /*
+ * Reject use of CAP_SETUID for functionality other than calling
+ * set*uid() (e.g. setting up userns uid mappings).
+ */
+ pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
+ __kuid_val(cred->uid));
+ return -1;
}
/*
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH 05/10] LSM: SafeSetID: refactor policy parsing
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>
In preparation for changing the policy parsing logic, refactor the line
parsing logic to be less verbose and move it into a separate function.
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
I made a minor change to Jann's original patch to use u32 instead of
s32 for the 'parsed_parent' and 'parsed_child' variables.
security/safesetid/securityfs.c | 84 +++++++++++++--------------------
1 file changed, 33 insertions(+), 51 deletions(-)
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 2c6c829be044..90784a8d950a 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -33,68 +33,50 @@ static struct safesetid_file_entry safesetid_files[] = {
/*
* In the case the input buffer contains one or more invalid UIDs, the kuid_t
- * variables pointed to by 'parent' and 'child' will get updated but this
+ * variables pointed to by @parent and @child will get updated but this
* function will return an error.
+ * Contents of @buf may be modified.
*/
-static int parse_safesetid_whitelist_policy(const char __user *buf,
- size_t len,
- kuid_t *parent,
- kuid_t *child)
+static int parse_policy_line(
+ struct file *file, char *buf, kuid_t *parent, kuid_t *child)
{
- char *kern_buf;
- char *parent_buf;
- char *child_buf;
- const char separator[] = ":";
+ char *child_str;
int ret;
- size_t first_substring_length;
- long parsed_parent;
- long parsed_child;
+ u32 parsed_parent, parsed_child;
- /* Duplicate string from user memory and NULL-terminate */
- kern_buf = memdup_user_nul(buf, len);
- if (IS_ERR(kern_buf))
- return PTR_ERR(kern_buf);
-
- /*
- * Format of |buf| string should be <UID>:<UID>.
- * Find location of ":" in kern_buf (copied from |buf|).
- */
- first_substring_length = strcspn(kern_buf, separator);
- if (first_substring_length == 0 || first_substring_length == len) {
- ret = -EINVAL;
- goto free_kern;
- }
-
- parent_buf = kmemdup_nul(kern_buf, first_substring_length, GFP_KERNEL);
- if (!parent_buf) {
- ret = -ENOMEM;
- goto free_kern;
- }
+ /* Format of |buf| string should be <UID>:<UID>. */
+ child_str = strchr(buf, ':');
+ if (child_str == NULL)
+ return -EINVAL;
+ *child_str = '\0';
+ child_str++;
- ret = kstrtol(parent_buf, 0, &parsed_parent);
+ ret = kstrtou32(buf, 0, &parsed_parent);
if (ret)
- goto free_both;
+ return ret;
- child_buf = kern_buf + first_substring_length + 1;
- ret = kstrtol(child_buf, 0, &parsed_child);
+ ret = kstrtou32(child_str, 0, &parsed_child);
if (ret)
- goto free_both;
+ return ret;
*parent = make_kuid(current_user_ns(), parsed_parent);
- if (!uid_valid(*parent)) {
- ret = -EINVAL;
- goto free_both;
- }
-
*child = make_kuid(current_user_ns(), parsed_child);
- if (!uid_valid(*child)) {
- ret = -EINVAL;
- goto free_both;
- }
+ if (!uid_valid(*parent) || !uid_valid(*child))
+ return -EINVAL;
-free_both:
- kfree(parent_buf);
-free_kern:
+ return 0;
+}
+
+static int parse_safesetid_whitelist_policy(
+ struct file *file, const char __user *buf, size_t len,
+ kuid_t *parent, kuid_t *child)
+{
+ char *kern_buf = memdup_user_nul(buf, len);
+ int ret;
+
+ if (IS_ERR(kern_buf))
+ return PTR_ERR(kern_buf);
+ ret = parse_policy_line(file, kern_buf, parent, child);
kfree(kern_buf);
return ret;
}
@@ -121,8 +103,8 @@ static ssize_t safesetid_file_write(struct file *file,
flush_safesetid_whitelist_entries();
break;
case SAFESETID_WHITELIST_ADD:
- ret = parse_safesetid_whitelist_policy(buf, len, &parent,
- &child);
+ ret = parse_safesetid_whitelist_policy(file, buf, len,
+ &parent, &child);
if (ret)
return ret;
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH 06/10] LSM: SafeSetID: fix userns handling in securityfs
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>
Looking at current_cred() in write handlers is bad form, stop doing that.
Also, let's just require that the write is coming from the initial user
namespace. Especially SAFESETID_WHITELIST_FLUSH requires privilege over all
namespaces, and SAFESETID_WHITELIST_ADD should probably require it as well.
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
security/safesetid/securityfs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 87e42b7f3e33..76c1e8a6ab93 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -59,8 +59,8 @@ static int parse_policy_line(
if (ret)
return ret;
- *parent = make_kuid(current_user_ns(), parsed_parent);
- *child = make_kuid(current_user_ns(), parsed_child);
+ *parent = make_kuid(file->f_cred->user_ns, parsed_parent);
+ *child = make_kuid(file->f_cred->user_ns, parsed_child);
if (!uid_valid(*parent) || !uid_valid(*child))
return -EINVAL;
@@ -92,7 +92,7 @@ static ssize_t safesetid_file_write(struct file *file,
kuid_t child;
int ret;
- if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
+ if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
return -EPERM;
if (*ppos != 0)
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH 07/10] LSM: SafeSetID: rewrite userspace API to atomic updates
From: Micah Morton @ 2019-04-10 16:56 UTC (permalink / raw)
To: jmorris, keescook, casey, linux-security-module; +Cc: Jann Horn, Micah Morton
From: Jann Horn <jannh@google.com>
The current API of the SafeSetID LSM uses one write() per rule, and applies
each written rule instantly. This has several downsides:
- While a policy is being loaded, once a single parent-child pair has been
loaded, the parent is restricted to that specific child, even if
subsequent rules would allow transitions to other child UIDs. This means
that during policy loading, set*uid() can randomly fail.
- To replace the policy without rebooting, it is necessary to first flush
all old rules. This creates a time window in which no constraints are
placed on the use of CAP_SETUID.
- If we want to perform sanity checks on the final policy, this requires
that the policy isn't constructed in a piecemeal fashion without telling
the kernel when it's done.
Other kernel APIs - including things like the userns code and netfilter -
avoid this problem by performing updates atomically. Luckily, SafeSetID
hasn't landed in a stable (upstream) release yet, so maybe it's not too
late to completely change the API.
The new API for SafeSetID is: If you want to change the policy, open
"safesetid/whitelist_policy" and write the entire policy,
newline-delimited, in there.
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
security/safesetid/lsm.c | 84 +++-----
security/safesetid/lsm.h | 24 +--
security/safesetid/securityfs.c | 194 ++++++++++--------
.../selftests/safesetid/safesetid-test.c | 16 +-
4 files changed, 149 insertions(+), 169 deletions(-)
diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index ab429e1816c5..4ab4d7cdba31 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -24,28 +24,38 @@
/* Flag indicating whether initialization completed */
int safesetid_initialized;
-#define NUM_BITS 8 /* 256 buckets in hash table */
+struct setuid_ruleset __rcu *safesetid_setuid_rules;
-static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
-
-static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
-
-static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
+/* Compute a decision for a transition from @src to @dst under @policy. */
+enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
+ kuid_t src, kuid_t dst)
{
- struct entry *entry;
+ struct setuid_rule *rule;
enum sid_policy_type result = SIDPOL_DEFAULT;
- rcu_read_lock();
- hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
- entry, next, __kuid_val(src)) {
- if (!uid_eq(entry->src_uid, src))
+ hash_for_each_possible(policy->rules, rule, next, __kuid_val(src)) {
+ if (!uid_eq(rule->src_uid, src))
continue;
- if (uid_eq(entry->dst_uid, dst)) {
- rcu_read_unlock();
+ if (uid_eq(rule->dst_uid, dst))
return SIDPOL_ALLOWED;
- }
result = SIDPOL_CONSTRAINED;
}
+ return result;
+}
+
+/*
+ * Compute a decision for a transition from @src to @dst under the active
+ * policy.
+ */
+static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
+{
+ enum sid_policy_type result = SIDPOL_DEFAULT;
+ struct setuid_ruleset *pol;
+
+ rcu_read_lock();
+ pol = rcu_dereference(safesetid_setuid_rules);
+ if (pol)
+ result = _setuid_policy_lookup(pol, src, dst);
rcu_read_unlock();
return result;
}
@@ -139,52 +149,6 @@ static int safesetid_task_fix_setuid(struct cred *new,
return -EACCES;
}
-int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
-{
- struct entry *new;
-
- /* Return if entry already exists */
- if (setuid_policy_lookup(parent, child) == SIDPOL_ALLOWED)
- return 0;
-
- new = kzalloc(sizeof(struct entry), GFP_KERNEL);
- if (!new)
- return -ENOMEM;
- new->src_uid = parent;
- new->dst_uid = child;
- spin_lock(&safesetid_whitelist_hashtable_spinlock);
- hash_add_rcu(safesetid_whitelist_hashtable,
- &new->next,
- __kuid_val(parent));
- spin_unlock(&safesetid_whitelist_hashtable_spinlock);
- return 0;
-}
-
-void flush_safesetid_whitelist_entries(void)
-{
- struct entry *entry;
- struct hlist_node *hlist_node;
- unsigned int bkt_loop_cursor;
- HLIST_HEAD(free_list);
-
- /*
- * Could probably use hash_for_each_rcu here instead, but this should
- * be fine as well.
- */
- spin_lock(&safesetid_whitelist_hashtable_spinlock);
- hash_for_each_safe(safesetid_whitelist_hashtable, bkt_loop_cursor,
- hlist_node, entry, next) {
- hash_del_rcu(&entry->next);
- hlist_add_head(&entry->dlist, &free_list);
- }
- spin_unlock(&safesetid_whitelist_hashtable_spinlock);
- synchronize_rcu();
- hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) {
- hlist_del(&entry->dlist);
- kfree(entry);
- }
-}
-
static struct security_hook_list safesetid_security_hooks[] = {
LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
LSM_HOOK_INIT(capable, safesetid_security_capable)
diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
index 6806f902794c..4a34f558d964 100644
--- a/security/safesetid/lsm.h
+++ b/security/safesetid/lsm.h
@@ -21,12 +21,6 @@
/* Flag indicating whether initialization completed */
extern int safesetid_initialized;
-/* Function type. */
-enum safesetid_whitelist_file_write_type {
- SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */
- 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 */
@@ -35,18 +29,24 @@ enum sid_policy_type {
/*
* Hash table entry to store safesetid policy signifying that 'src_uid'
- * can setid to 'dst_uid'.
+ * can setuid to 'dst_uid'.
*/
-struct entry {
+struct setuid_rule {
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);
+#define SETID_HASH_BITS 8 /* 256 buckets in hash table */
+
+struct setuid_ruleset {
+ DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
+ struct rcu_head rcu;
+};
+
+enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
+ kuid_t src, kuid_t dst);
-void flush_safesetid_whitelist_entries(void);
+extern struct setuid_ruleset __rcu *safesetid_setuid_rules;
#endif /* _SAFESETID_H */
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 76c1e8a6ab93..13fce4c10930 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -11,25 +11,15 @@
* published by the Free Software Foundation.
*
*/
+
+#define pr_fmt(fmt) "SafeSetID: " fmt
+
#include <linux/security.h>
#include <linux/cred.h>
#include "lsm.h"
-static struct dentry *safesetid_policy_dir;
-
-struct safesetid_file_entry {
- const char *name;
- enum safesetid_whitelist_file_write_type type;
- struct dentry *dentry;
-};
-
-static struct safesetid_file_entry safesetid_files[] = {
- {.name = "add_whitelist_policy",
- .type = SAFESETID_WHITELIST_ADD},
- {.name = "flush_whitelist_policies",
- .type = SAFESETID_WHITELIST_FLUSH},
-};
+static DEFINE_SPINLOCK(policy_update_lock);
/*
* In the case the input buffer contains one or more invalid UIDs, the kuid_t
@@ -37,8 +27,8 @@ static struct safesetid_file_entry safesetid_files[] = {
* function will return an error.
* Contents of @buf may be modified.
*/
-static int parse_policy_line(
- struct file *file, char *buf, kuid_t *parent, kuid_t *child)
+static int parse_policy_line(struct file *file, char *buf,
+ struct setuid_rule *rule)
{
char *child_str;
int ret;
@@ -59,26 +49,103 @@ static int parse_policy_line(
if (ret)
return ret;
- *parent = make_kuid(file->f_cred->user_ns, parsed_parent);
- *child = make_kuid(file->f_cred->user_ns, parsed_child);
- if (!uid_valid(*parent) || !uid_valid(*child))
+ rule->src_uid = make_kuid(file->f_cred->user_ns, parsed_parent);
+ rule->dst_uid = make_kuid(file->f_cred->user_ns, parsed_child);
+ if (!uid_valid(rule->src_uid) || !uid_valid(rule->dst_uid))
return -EINVAL;
return 0;
}
-static int parse_safesetid_whitelist_policy(
- struct file *file, const char __user *buf, size_t len,
- kuid_t *parent, kuid_t *child)
+static void __release_ruleset(struct rcu_head *rcu)
{
- char *kern_buf = memdup_user_nul(buf, len);
- int ret;
+ struct setuid_ruleset *pol =
+ container_of(rcu, struct setuid_ruleset, rcu);
+ int bucket;
+ struct setuid_rule *rule;
+ struct hlist_node *tmp;
+
+ hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
+ kfree(rule);
+ kfree(pol);
+}
- if (IS_ERR(kern_buf))
- return PTR_ERR(kern_buf);
- ret = parse_policy_line(file, kern_buf, parent, child);
- kfree(kern_buf);
- return ret;
+static void release_ruleset(struct setuid_ruleset *pol)
+{
+ call_rcu(&pol->rcu, __release_ruleset);
+}
+
+static ssize_t handle_policy_update(struct file *file,
+ const char __user *ubuf, size_t len)
+{
+ struct setuid_ruleset *pol;
+ char *buf, *p, *end;
+ int err;
+
+ pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
+ if (!pol)
+ return -ENOMEM;
+ hash_init(pol->rules);
+
+ p = buf = memdup_user_nul(ubuf, len);
+ if (IS_ERR(buf)) {
+ err = PTR_ERR(buf);
+ goto out_free_pol;
+ }
+
+ /* policy lines, including the last one, end with \n */
+ while (*p != '\0') {
+ struct setuid_rule *rule;
+
+ end = strchr(p, '\n');
+ if (end == NULL) {
+ err = -EINVAL;
+ goto out_free_buf;
+ }
+ *end = '\0';
+
+ rule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
+ if (!rule) {
+ err = -ENOMEM;
+ goto out_free_buf;
+ }
+
+ err = parse_policy_line(file, p, rule);
+ if (err)
+ goto out_free_rule;
+
+ if (_setuid_policy_lookup(pol, rule->src_uid, rule->dst_uid) ==
+ SIDPOL_ALLOWED) {
+ pr_warn("bad policy: duplicate entry\n");
+ err = -EEXIST;
+ goto out_free_rule;
+ }
+
+ hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
+ p = end + 1;
+ continue;
+
+out_free_rule:
+ kfree(rule);
+ goto out_free_buf;
+ }
+
+ /*
+ * Everything looks good, apply the policy and release the old one.
+ * What we really want here is an xchg() wrapper for RCU, but since that
+ * doesn't currently exist, just use a spinlock for now.
+ */
+ spin_lock(&policy_update_lock);
+ rcu_swap_protected(safesetid_setuid_rules, pol,
+ lockdep_is_held(&policy_update_lock));
+ spin_unlock(&policy_update_lock);
+ err = len;
+
+out_free_buf:
+ kfree(buf);
+out_free_pol:
+ release_ruleset(pol);
+ return err;
}
static ssize_t safesetid_file_write(struct file *file,
@@ -86,90 +153,45 @@ static ssize_t safesetid_file_write(struct file *file,
size_t len,
loff_t *ppos)
{
- struct safesetid_file_entry *file_entry =
- file->f_inode->i_private;
- kuid_t parent;
- kuid_t child;
- int ret;
-
if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
return -EPERM;
if (*ppos != 0)
return -EINVAL;
- switch (file_entry->type) {
- case SAFESETID_WHITELIST_FLUSH:
- flush_safesetid_whitelist_entries();
- break;
- case SAFESETID_WHITELIST_ADD:
- ret = parse_safesetid_whitelist_policy(file, buf, len,
- &parent, &child);
- if (ret)
- return ret;
-
- ret = add_safesetid_whitelist_entry(parent, child);
- if (ret)
- return ret;
- break;
- default:
- pr_warn("Unknown securityfs file %d\n", file_entry->type);
- break;
- }
-
- /* Return len on success so caller won't keep trying to write */
- return len;
+ return handle_policy_update(file, buf, len);
}
static const struct file_operations safesetid_file_fops = {
.write = safesetid_file_write,
};
-static void safesetid_shutdown_securityfs(void)
-{
- int i;
-
- for (i = 0; i < ARRAY_SIZE(safesetid_files); ++i) {
- struct safesetid_file_entry *entry =
- &safesetid_files[i];
- securityfs_remove(entry->dentry);
- entry->dentry = NULL;
- }
-
- securityfs_remove(safesetid_policy_dir);
- safesetid_policy_dir = NULL;
-}
-
static int __init safesetid_init_securityfs(void)
{
- int i;
int ret;
+ struct dentry *policy_dir;
+ struct dentry *policy_file;
if (!safesetid_initialized)
return 0;
- safesetid_policy_dir = securityfs_create_dir("safesetid", NULL);
- if (IS_ERR(safesetid_policy_dir)) {
- ret = PTR_ERR(safesetid_policy_dir);
+ policy_dir = securityfs_create_dir("safesetid", NULL);
+ if (IS_ERR(policy_dir)) {
+ ret = PTR_ERR(policy_dir);
goto error;
}
- for (i = 0; i < ARRAY_SIZE(safesetid_files); ++i) {
- struct safesetid_file_entry *entry =
- &safesetid_files[i];
- entry->dentry = securityfs_create_file(
- entry->name, 0200, safesetid_policy_dir,
- entry, &safesetid_file_fops);
- if (IS_ERR(entry->dentry)) {
- ret = PTR_ERR(entry->dentry);
- goto error;
- }
+ policy_file = securityfs_create_file("whitelist_policy", 0200,
+ policy_dir, NULL, &safesetid_file_fops);
+ if (IS_ERR(policy_file)) {
+ ret = PTR_ERR(policy_file);
+ goto error;
}
return 0;
error:
- safesetid_shutdown_securityfs();
+ securityfs_remove(policy_dir);
return ret;
}
fs_initcall(safesetid_init_securityfs);
diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c
index 892c8e8b1b8b..4f03813d1911 100644
--- a/tools/testing/selftests/safesetid/safesetid-test.c
+++ b/tools/testing/selftests/safesetid/safesetid-test.c
@@ -142,23 +142,17 @@ static void ensure_securityfs_mounted(void)
static void write_policies(void)
{
+ static char *policy_str =
+ "1:2\n"
+ "1:3\n";
ssize_t written;
int fd;
fd = open(add_whitelist_policy_file, O_WRONLY);
if (fd < 0)
die("cant open add_whitelist_policy file\n");
- written = write(fd, "1:2", strlen("1:2"));
- if (written != strlen("1:2")) {
- if (written >= 0) {
- die("short write to %s\n", add_whitelist_policy_file);
- } else {
- die("write to %s failed: %s\n",
- add_whitelist_policy_file, strerror(errno));
- }
- }
- written = write(fd, "1:3", strlen("1:3"));
- if (written != strlen("1:3")) {
+ written = write(fd, policy_str, strlen(policy_str));
+ if (written != strlen(policy_str)) {
if (written >= 0) {
die("short write to %s\n", add_whitelist_policy_file);
} else {
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH 08/10] LSM: SafeSetID: add read handler
From: Micah Morton @ 2019-04-10 16:56 UTC (permalink / raw)
To: jmorris, keescook, casey, linux-security-module; +Cc: Jann Horn, Micah Morton
From: Jann Horn <jannh@google.com>
For debugging a running system, it is very helpful to be able to see what
policy the system is using. Add a read handler that can dump out a copy of
the loaded policy.
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
security/safesetid/lsm.h | 3 +++
security/safesetid/securityfs.c | 38 +++++++++++++++++++++++++++++++--
2 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
index 4a34f558d964..9380329fe30a 100644
--- a/security/safesetid/lsm.h
+++ b/security/safesetid/lsm.h
@@ -17,6 +17,7 @@
#include <linux/types.h>
#include <linux/uidgid.h>
#include <linux/hashtable.h>
+#include <linux/refcount.h>
/* Flag indicating whether initialization completed */
extern int safesetid_initialized;
@@ -41,7 +42,9 @@ struct setuid_rule {
struct setuid_ruleset {
DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
+ char *policy_str;
struct rcu_head rcu;
+ refcount_t refcount;
};
enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 13fce4c10930..7a08fff2bc14 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -67,12 +67,14 @@ static void __release_ruleset(struct rcu_head *rcu)
hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
kfree(rule);
+ kfree(pol->policy_str);
kfree(pol);
}
static void release_ruleset(struct setuid_ruleset *pol)
{
- call_rcu(&pol->rcu, __release_ruleset);
+ if (pol != NULL && refcount_dec_and_test(&pol->refcount))
+ call_rcu(&pol->rcu, __release_ruleset);
}
static ssize_t handle_policy_update(struct file *file,
@@ -85,6 +87,8 @@ static ssize_t handle_policy_update(struct file *file,
pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
if (!pol)
return -ENOMEM;
+ refcount_set(&pol->refcount, 1);
+ pol->policy_str = NULL;
hash_init(pol->rules);
p = buf = memdup_user_nul(ubuf, len);
@@ -92,6 +96,11 @@ static ssize_t handle_policy_update(struct file *file,
err = PTR_ERR(buf);
goto out_free_pol;
}
+ pol->policy_str = kstrdup(buf, GFP_KERNEL);
+ if (pol->policy_str == NULL) {
+ err = -ENOMEM;
+ goto out_free_buf;
+ }
/* policy lines, including the last one, end with \n */
while (*p != '\0') {
@@ -162,7 +171,32 @@ static ssize_t safesetid_file_write(struct file *file,
return handle_policy_update(file, buf, len);
}
+static ssize_t safesetid_file_read(struct file *file, char __user *buf,
+ size_t len, loff_t *ppos)
+{
+ ssize_t res;
+ struct setuid_ruleset *pol;
+ const char *kbuf;
+
+ rcu_read_lock();
+ pol = rcu_dereference(safesetid_setuid_rules);
+ if (!pol) {
+ rcu_read_unlock();
+ return 0;
+ }
+ if (!refcount_inc_not_zero(&pol->refcount)) {
+ rcu_read_unlock();
+ return -EBUSY;
+ }
+ rcu_read_unlock();
+ kbuf = pol->policy_str;
+ res = simple_read_from_buffer(buf, len, ppos, kbuf, strlen(kbuf));
+ release_ruleset(pol);
+ return res;
+}
+
static const struct file_operations safesetid_file_fops = {
+ .read = safesetid_file_read,
.write = safesetid_file_write,
};
@@ -181,7 +215,7 @@ static int __init safesetid_init_securityfs(void)
goto error;
}
- policy_file = securityfs_create_file("whitelist_policy", 0200,
+ policy_file = securityfs_create_file("whitelist_policy", 0600,
policy_dir, NULL, &safesetid_file_fops);
if (IS_ERR(policy_file)) {
ret = PTR_ERR(policy_file);
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH 09/10] LSM: SafeSetID: verify transitive constrainedness
From: Micah Morton @ 2019-04-10 16:56 UTC (permalink / raw)
To: jmorris, keescook, casey, linux-security-module; +Cc: Jann Horn, Micah Morton
From: Jann Horn <jannh@google.com>
Someone might write a ruleset like the following, expecting that it
securely constrains UID 1 to UIDs 1, 2 and 3:
1:2
1:3
However, because no constraints are applied to UIDs 2 and 3, an attacker
with UID 1 can simply first switch to UID 2, then switch to any UID from
there. The secure way to write this ruleset would be:
1:2
1:3
2:2
3:3
, which uses "transition to self" as a way to inhibit the default-allow
policy without allowing anything specific.
This is somewhat unintuitive. To make sure that policy authors don't
accidentally write insecure policies because of this, let the kernel verify
that a new ruleset does not contain any entries that are constrained, but
transitively unconstrained.
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
security/safesetid/securityfs.c | 21 +++++++++++++++++++
.../selftests/safesetid/safesetid-test.c | 4 +++-
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 7a08fff2bc14..3ec64487f0e9 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -77,6 +77,23 @@ static void release_ruleset(struct setuid_ruleset *pol)
call_rcu(&pol->rcu, __release_ruleset);
}
+static int verify_ruleset(struct setuid_ruleset *pol)
+{
+ int bucket;
+ struct setuid_rule *rule;
+
+ hash_for_each(pol->rules, bucket, rule, next) {
+ if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
+ SIDPOL_DEFAULT) {
+ pr_warn("insecure policy rejected: uid %d is constrained but transitively unconstrained through uid %d\n",
+ __kuid_val(rule->src_uid),
+ __kuid_val(rule->dst_uid));
+ return -EINVAL;
+ }
+ }
+ return 0;
+}
+
static ssize_t handle_policy_update(struct file *file,
const char __user *ubuf, size_t len)
{
@@ -139,6 +156,10 @@ static ssize_t handle_policy_update(struct file *file,
goto out_free_buf;
}
+ err = verify_ruleset(pol);
+ if (err)
+ goto out_free_buf;
+
/*
* Everything looks good, apply the policy and release the old one.
* What we really want here is an xchg() wrapper for RCU, but since that
diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c
index 4f03813d1911..8f40c6ecdad1 100644
--- a/tools/testing/selftests/safesetid/safesetid-test.c
+++ b/tools/testing/selftests/safesetid/safesetid-test.c
@@ -144,7 +144,9 @@ static void write_policies(void)
{
static char *policy_str =
"1:2\n"
- "1:3\n";
+ "1:3\n"
+ "2:2\n"
+ "3:3\n";
ssize_t written;
int fd;
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH 10/10] LSM: SafeSetID: fix use of literal -1 in capable hook
From: Micah Morton @ 2019-04-10 16:56 UTC (permalink / raw)
To: jmorris, keescook, casey, linux-security-module; +Cc: Jann Horn, Micah Morton
From: Jann Horn <jannh@google.com>
The capable() hook returns an error number. -EPERM is actually the same as
-1, so this doesn't make a difference in behavior.
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
security/safesetid/lsm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index 4ab4d7cdba31..61b84e20f2dd 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -90,7 +90,7 @@ static int safesetid_security_capable(const struct cred *cred,
*/
pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
__kuid_val(cred->uid));
- return -1;
+ return -EPERM;
}
/*
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* Re: [PATCH 01/10] LSM: SafeSetID: fix pr_warn() to include newline
From: Kees Cook @ 2019-04-10 17:09 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190410165434.206579-1-mortonm@chromium.org>
On Wed, Apr 10, 2019 at 9:54 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> Fix the pr_warn() calls in the SafeSetID LSM to have newlines at the end.
> Without this, denial messages will be buffered as incomplete lines in
> log_output(), and will then only show up once something else prints into
> dmesg.
>
> 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 | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index cecd38e2ac80..2daecab3a4c0 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -91,7 +91,7 @@ static int safesetid_security_capable(const struct cred *cred,
> * to functionality other than calling set*uid() (e.g.
> * allowing user to set up userns uid mappings).
> */
> - pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions",
> + pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> __kuid_val(cred->uid));
> return -1;
> }
> @@ -103,7 +103,7 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
> {
> if (check_setuid_policy_hashtable_key_value(parent, child))
> return 0;
> - pr_warn("UID transition (%d -> %d) blocked",
> + pr_warn("UID transition (%d -> %d) blocked\n",
> __kuid_val(parent),
> __kuid_val(child));
> /*
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 02/10] LSM: SafeSetID: fix check for setresuid(new1, new2, new3)
From: Kees Cook @ 2019-04-10 17:11 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190410165519.209565-1-mortonm@chromium.org>
On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> With the old code, when a process with the (real,effective,saved) UID set
> (1,1,1) calls setresuid(2,3,4), safesetid_task_fix_setuid() only checks
> whether the transition 1->2 is permitted; the transitions 1->3 and 1->4 are
> not checked. Fix this.
>
> This is also a good opportunity to refactor safesetid_task_fix_setuid() to
> be less verbose - having one branch per set*uid() syscall is unnecessary.
>
> Note that this slightly changes semantics: The UID transition check for
> UIDs that were not in the old cred struct is now always performed against
> the policy of the RUID. I think that's more consistent anyway, since the
> RUID is also the one that decides whether any policy is enforced at all.
>
> 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 | 125 +++++++++++----------------------------
> 1 file changed, 35 insertions(+), 90 deletions(-)
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index 2daecab3a4c0..5310fcf3052a 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -99,20 +99,30 @@ static int safesetid_security_capable(const struct cred *cred,
> return 0;
> }
>
> -static int check_uid_transition(kuid_t parent, kuid_t child)
> +/*
> + * Check whether a caller with old credentials @old is allowed to switch to
> + * credentials that contain @new_uid.
> + */
> +static bool uid_permitted_for_cred(const struct cred *old, kuid_t new_uid)
> {
> - if (check_setuid_policy_hashtable_key_value(parent, child))
> - return 0;
> - pr_warn("UID transition (%d -> %d) blocked\n",
> - __kuid_val(parent),
> - __kuid_val(child));
> + bool permitted;
> +
> + /* If our old creds already had this UID in it, it's fine. */
> + if (uid_eq(new_uid, old->uid) || uid_eq(new_uid, old->euid) ||
> + uid_eq(new_uid, old->suid))
> + return true;
> +
> /*
> - * Kill this process to avoid potential security vulnerabilities
> - * that could arise from a missing whitelist entry preventing a
> - * privileged process from dropping to a lesser-privileged one.
> + * Transitions to new UIDs require a check against the policy of the old
> + * RUID.
> */
> - force_sig(SIGKILL, current);
> - return -EACCES;
> + permitted = check_setuid_policy_hashtable_key_value(old->uid, new_uid);
> + if (!permitted) {
> + pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n",
> + __kuid_val(old->uid), __kuid_val(old->euid),
> + __kuid_val(old->suid), __kuid_val(new_uid));
> + }
> + return permitted;
> }
>
> /*
> @@ -125,88 +135,23 @@ static int safesetid_task_fix_setuid(struct cred *new,
> int flags)
> {
>
> - /* Do nothing if there are no setuid restrictions for this UID. */
> + /* Do nothing if there are no setuid restrictions for our old RUID. */
> if (!check_setuid_policy_hashtable_key(old->uid))
> return 0;
>
> - switch (flags) {
> - case LSM_SETID_RE:
> - /*
> - * Users for which setuid restrictions exist can only set the
> - * real UID to the real UID or the effective UID, unless an
> - * explicit whitelist policy allows the transition.
> - */
> - if (!uid_eq(old->uid, new->uid) &&
> - !uid_eq(old->euid, new->uid)) {
> - return check_uid_transition(old->uid, new->uid);
> - }
> - /*
> - * Users for which setuid restrictions exist can only set the
> - * effective UID to the real UID, the effective UID, or the
> - * saved set-UID, unless an explicit whitelist policy allows
> - * the transition.
> - */
> - if (!uid_eq(old->uid, new->euid) &&
> - !uid_eq(old->euid, new->euid) &&
> - !uid_eq(old->suid, new->euid)) {
> - return check_uid_transition(old->euid, new->euid);
> - }
> - break;
> - case LSM_SETID_ID:
> - /*
> - * Users for which setuid restrictions exist cannot change the
> - * real UID or saved set-UID unless an explicit whitelist
> - * policy allows the transition.
> - */
> - if (!uid_eq(old->uid, new->uid))
> - return check_uid_transition(old->uid, new->uid);
> - if (!uid_eq(old->suid, new->suid))
> - return check_uid_transition(old->suid, new->suid);
> - break;
> - case LSM_SETID_RES:
> - /*
> - * Users for which setuid restrictions exist cannot change the
> - * real UID, effective UID, or saved set-UID to anything but
> - * one of: the current real UID, the current effective UID or
> - * the current saved set-user-ID unless an explicit whitelist
> - * policy allows the transition.
> - */
> - if (!uid_eq(new->uid, old->uid) &&
> - !uid_eq(new->uid, old->euid) &&
> - !uid_eq(new->uid, old->suid)) {
> - return check_uid_transition(old->uid, new->uid);
> - }
> - if (!uid_eq(new->euid, old->uid) &&
> - !uid_eq(new->euid, old->euid) &&
> - !uid_eq(new->euid, old->suid)) {
> - return check_uid_transition(old->euid, new->euid);
> - }
> - if (!uid_eq(new->suid, old->uid) &&
> - !uid_eq(new->suid, old->euid) &&
> - !uid_eq(new->suid, old->suid)) {
> - return check_uid_transition(old->suid, new->suid);
> - }
> - break;
> - case LSM_SETID_FS:
> - /*
> - * Users for which setuid restrictions exist cannot change the
> - * filesystem UID to anything but one of: the current real UID,
> - * the current effective UID or the current saved set-UID
> - * unless an explicit whitelist policy allows the transition.
> - */
> - if (!uid_eq(new->fsuid, old->uid) &&
> - !uid_eq(new->fsuid, old->euid) &&
> - !uid_eq(new->fsuid, old->suid) &&
> - !uid_eq(new->fsuid, old->fsuid)) {
> - return check_uid_transition(old->fsuid, new->fsuid);
> - }
> - break;
> - default:
> - pr_warn("Unknown setid state %d\n", flags);
> - force_sig(SIGKILL, current);
> - return -EINVAL;
> - }
> - return 0;
> + if (uid_permitted_for_cred(old, new->uid) &&
> + uid_permitted_for_cred(old, new->euid) &&
> + uid_permitted_for_cred(old, new->suid) &&
> + uid_permitted_for_cred(old, new->fsuid))
> + return 0;
> +
> + /*
> + * Kill this process to avoid potential security vulnerabilities
> + * that could arise from a missing whitelist entry preventing a
> + * privileged process from dropping to a lesser-privileged one.
> + */
> + force_sig(SIGKILL, current);
> + return -EACCES;
> }
>
> int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 03/10] LSM: SafeSetID: refactor policy hash table
From: Kees Cook @ 2019-04-10 17:12 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190410165534.210333-1-mortonm@chromium.org>
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
* Re: [PATCH 04/10] LSM: SafeSetID: refactor safesetid_security_capable()
From: Kees Cook @ 2019-04-10 17:13 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190410165541.210809-1-mortonm@chromium.org>
On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> At the moment, safesetid_security_capable() has two nested conditional
> blocks, and one big comment for all the logic. Chop it up and reduce the
> amount of indentation.
>
> 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 | 41 +++++++++++++++++++++++++---------------
> 1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index 15cd13b5a211..ab429e1816c5 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -55,21 +55,32 @@ static int safesetid_security_capable(const struct cred *cred,
> int cap,
> unsigned int opts)
> {
> - if (cap == CAP_SETUID &&
> - 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
> - * giving powers gated by CAP_SETUID that are related
> - * to functionality other than calling set*uid() (e.g.
> - * allowing user to set up userns uid mappings).
> - */
> - pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> - __kuid_val(cred->uid));
> - return -1;
> - }
> - }
> - return 0;
> + /* We're only interested in CAP_SETUID. */
> + if (cap != CAP_SETUID)
> + return 0;
> +
> + /*
> + * If CAP_SETUID is currently used for a set*uid() syscall, we want to
> + * let it go through here; the real security check happens later, in the
> + * task_fix_setuid hook.
> + */
> + if ((opts & CAP_OPT_INSETID) != 0)
> + return 0;
> +
> + /*
> + * If no policy applies to this task, allow the use of CAP_SETUID for
> + * other purposes.
> + */
> + if (setuid_policy_lookup(cred->uid, INVALID_UID) == SIDPOL_DEFAULT)
> + return 0;
> +
> + /*
> + * Reject use of CAP_SETUID for functionality other than calling
> + * set*uid() (e.g. setting up userns uid mappings).
> + */
> + pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> + __kuid_val(cred->uid));
> + return -1;
> }
>
> /*
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 05/10] LSM: SafeSetID: refactor policy parsing
From: Kees Cook @ 2019-04-10 17:15 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190410165548.211254-1-mortonm@chromium.org>
On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> In preparation for changing the policy parsing logic, refactor the line
> parsing logic to be less verbose and move it into a separate function.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> I made a minor change to Jann's original patch to use u32 instead of
> s32 for the 'parsed_parent' and 'parsed_child' variables.
>
> security/safesetid/securityfs.c | 84 +++++++++++++--------------------
> 1 file changed, 33 insertions(+), 51 deletions(-)
>
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 2c6c829be044..90784a8d950a 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -33,68 +33,50 @@ static struct safesetid_file_entry safesetid_files[] = {
>
> /*
> * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> - * variables pointed to by 'parent' and 'child' will get updated but this
> + * variables pointed to by @parent and @child will get updated but this
> * function will return an error.
> + * Contents of @buf may be modified.
> */
> -static int parse_safesetid_whitelist_policy(const char __user *buf,
> - size_t len,
> - kuid_t *parent,
> - kuid_t *child)
> +static int parse_policy_line(
> + struct file *file, char *buf, kuid_t *parent, kuid_t *child)
> {
> - char *kern_buf;
> - char *parent_buf;
> - char *child_buf;
> - const char separator[] = ":";
> + char *child_str;
> int ret;
> - size_t first_substring_length;
> - long parsed_parent;
> - long parsed_child;
> + u32 parsed_parent, parsed_child;
>
> - /* Duplicate string from user memory and NULL-terminate */
> - kern_buf = memdup_user_nul(buf, len);
> - if (IS_ERR(kern_buf))
> - return PTR_ERR(kern_buf);
> -
> - /*
> - * Format of |buf| string should be <UID>:<UID>.
> - * Find location of ":" in kern_buf (copied from |buf|).
> - */
> - first_substring_length = strcspn(kern_buf, separator);
> - if (first_substring_length == 0 || first_substring_length == len) {
> - ret = -EINVAL;
> - goto free_kern;
> - }
> -
> - parent_buf = kmemdup_nul(kern_buf, first_substring_length, GFP_KERNEL);
> - if (!parent_buf) {
> - ret = -ENOMEM;
> - goto free_kern;
> - }
> + /* Format of |buf| string should be <UID>:<UID>. */
> + child_str = strchr(buf, ':');
> + if (child_str == NULL)
> + return -EINVAL;
> + *child_str = '\0';
> + child_str++;
>
> - ret = kstrtol(parent_buf, 0, &parsed_parent);
> + ret = kstrtou32(buf, 0, &parsed_parent);
> if (ret)
> - goto free_both;
> + return ret;
>
> - child_buf = kern_buf + first_substring_length + 1;
> - ret = kstrtol(child_buf, 0, &parsed_child);
> + ret = kstrtou32(child_str, 0, &parsed_child);
> if (ret)
> - goto free_both;
> + return ret;
>
> *parent = make_kuid(current_user_ns(), parsed_parent);
> - if (!uid_valid(*parent)) {
> - ret = -EINVAL;
> - goto free_both;
> - }
> -
> *child = make_kuid(current_user_ns(), parsed_child);
> - if (!uid_valid(*child)) {
> - ret = -EINVAL;
> - goto free_both;
> - }
> + if (!uid_valid(*parent) || !uid_valid(*child))
> + return -EINVAL;
>
> -free_both:
> - kfree(parent_buf);
> -free_kern:
> + return 0;
> +}
> +
> +static int parse_safesetid_whitelist_policy(
> + struct file *file, const char __user *buf, size_t len,
> + kuid_t *parent, kuid_t *child)
> +{
> + char *kern_buf = memdup_user_nul(buf, len);
> + int ret;
> +
> + if (IS_ERR(kern_buf))
> + return PTR_ERR(kern_buf);
> + ret = parse_policy_line(file, kern_buf, parent, child);
> kfree(kern_buf);
> return ret;
> }
> @@ -121,8 +103,8 @@ static ssize_t safesetid_file_write(struct file *file,
> flush_safesetid_whitelist_entries();
> break;
> case SAFESETID_WHITELIST_ADD:
> - ret = parse_safesetid_whitelist_policy(buf, len, &parent,
> - &child);
> + ret = parse_safesetid_whitelist_policy(file, buf, len,
> + &parent, &child);
> if (ret)
> return ret;
>
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 06/10] LSM: SafeSetID: fix userns handling in securityfs
From: Kees Cook @ 2019-04-10 17:16 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190410165558.211483-1-mortonm@chromium.org>
On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> Looking at current_cred() in write handlers is bad form, stop doing that.
>
> Also, let's just require that the write is coming from the initial user
> namespace. Especially SAFESETID_WHITELIST_FLUSH requires privilege over all
> namespaces, and SAFESETID_WHITELIST_ADD should probably require it as well.
>
> 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/securityfs.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 87e42b7f3e33..76c1e8a6ab93 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -59,8 +59,8 @@ static int parse_policy_line(
> if (ret)
> return ret;
>
> - *parent = make_kuid(current_user_ns(), parsed_parent);
> - *child = make_kuid(current_user_ns(), parsed_child);
> + *parent = make_kuid(file->f_cred->user_ns, parsed_parent);
> + *child = make_kuid(file->f_cred->user_ns, parsed_child);
> if (!uid_valid(*parent) || !uid_valid(*child))
> return -EINVAL;
>
> @@ -92,7 +92,7 @@ static ssize_t safesetid_file_write(struct file *file,
> kuid_t child;
> int ret;
>
> - if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
> + if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
> return -EPERM;
>
> if (*ppos != 0)
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 58/59] LSM: Specify which LSM to display with /proc/self/attr/display
From: Casey Schaufler @ 2019-04-10 17:18 UTC (permalink / raw)
To: Stephen Smalley
Cc: Schaufler, Casey, James Morris, linux-security-module, selinux,
casey
In-Reply-To: <CAB9W1A2Bgr+gVGNxyVOwd7A52Gp8Kx0dPyd91pSjtXKp4UHoxw@mail.gmail.com>
On 4/10/2019 7:09 AM, Stephen Smalley wrote:
> On Wed, Apr 10, 2019 at 8:43 AM Stephen Smalley
> <stephen.smalley@gmail.com> wrote:
>> On Tue, Apr 9, 2019 at 5:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> Create a new entry "display" in /proc/.../attr for controlling
>>> which LSM security information is displayed for a process.
>>> The name of an active LSM that supplies hooks for human readable
>>> data may be written to "display" to set the value. The name of
>>> the LSM currently in use can be read from "display".
>>>
>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>> ---
>>> fs/proc/base.c | 1 +
>>> security/security.c | 123 ++++++++++++++++++++++++++++++++++++++++++--
>>> 2 files changed, 121 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>> index ddef482f1334..7bf70e041315 100644
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -2618,6 +2618,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>>> ATTR(NULL, "fscreate", 0666),
>>> ATTR(NULL, "keycreate", 0666),
>>> ATTR(NULL, "sockcreate", 0666),
>>> + ATTR(NULL, "display", 0666),
>>> #ifdef CONFIG_SECURITY_SMACK
>>> DIR("smack", 0555,
>>> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
>>> diff --git a/security/security.c b/security/security.c
>>> index 29149db3f78a..6e304aa796f9 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -47,9 +47,13 @@ static struct kmem_cache *lsm_inode_cache;
>>>
>>> char *lsm_names;
>>>
>>> -/* Socket blobs include infrastructure managed data */
>>> +/*
>>> + * Socket blobs include infrastructure managed data
>>> + * Cred blobs include context display instructions
>>> + */
>>> static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
>>> .lbs_sock = sizeof(struct lsm_export),
>>> + .lbs_cred = sizeof(struct lsm_one_hooks),
>>> };
>>>
>>> /**
>>> @@ -751,7 +755,10 @@ int lsm_superblock_alloc(struct super_block *sb)
>>>
>>> #define call_one_int_hook(FUNC, IRC, ...) ({ \
>>> int RC = IRC; \
>>> - if (lsm_base_one.FUNC.FUNC) \
>>> + struct lsm_one_hooks *LOH = current_cred()->security; \
>>> + if (LOH->FUNC.FUNC) \
>>> + RC = LOH->FUNC.FUNC(__VA_ARGS__); \
>>> + else if (LOH->lsm == NULL && lsm_base_one.FUNC.FUNC) \
>>> RC = lsm_base_one.FUNC.FUNC(__VA_ARGS__); \
>>> RC; \
>>> })
>>> @@ -1617,6 +1624,7 @@ int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
>>>
>>> void security_cred_free(struct cred *cred)
>>> {
>>> + struct lsm_one_hooks *loh;
>>> /*
>>> * There is a failure case in prepare_creds() that
>>> * may result in a call here with ->security being NULL.
>>> @@ -1626,26 +1634,44 @@ void security_cred_free(struct cred *cred)
>>>
>>> call_void_hook(cred_free, cred);
>>>
>>> + loh = cred->security;
>>> + kfree(loh->lsm);
>>> kfree(cred->security);
>>> cred->security = NULL;
>>> }
>>>
>>> +static int copy_loh(struct lsm_one_hooks *new, struct lsm_one_hooks *old,
>>> + gfp_t gfp)
>>> +{
>>> + *new = *old;
>>> + if (old->lsm) {
>>> + new->lsm = kstrdup(old->lsm, gfp);
>>> + if (unlikely(new->lsm == NULL))
>>> + return -ENOMEM;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp)
>>> {
>>> int rc = lsm_cred_alloc(new, gfp);
>>>
>>> - if (rc)
>>> + if (unlikely(rc))
>>> return rc;
>>>
>>> rc = call_int_hook(cred_prepare, 0, new, old, gfp);
>>> if (unlikely(rc))
>>> security_cred_free(new);
>>> + else
>>> + rc = copy_loh(new->security, old->security, gfp);
>>> +
>>> return rc;
>>> }
>>>
>>> void security_transfer_creds(struct cred *new, const struct cred *old)
>>> {
>>> call_void_hook(cred_transfer, new, old);
>>> + WARN_ON(copy_loh(new->security, old->security, GFP_KERNEL));
>>> }
>>>
>>> void security_cred_getsecid(const struct cred *c, struct lsm_export *l)
>>> @@ -1960,10 +1986,28 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>> char **value)
>>> {
>>> struct security_hook_list *hp;
>>> + struct lsm_one_hooks *loh = current_cred()->security;
>>> + char *s;
>>> +
>>> + if (!strcmp(name, "display")) {
>>> + if (loh->lsm)
>>> + s = loh->lsm;
>>> + else if (lsm_base_one.lsm)
>>> + s = lsm_base_one.lsm;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + *value = kstrdup(s, GFP_KERNEL);
>>> + if (*value)
>>> + return strlen(s);
>>> + return -ENOMEM;
>>> + }
>>>
>>> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>>> if (lsm != NULL && strcmp(lsm, hp->lsm))
>>> continue;
>>> + if (lsm == NULL && loh->lsm && strcmp(loh->lsm, hp->lsm))
>>> + continue;
>>> return hp->hook.getprocattr(p, name, value);
>>> }
>>> return -EINVAL;
>>> @@ -1973,10 +2017,83 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>>> size_t size)
>>> {
>>> struct security_hook_list *hp;
>>> + struct lsm_one_hooks *loh = current_cred()->security;
>>> + bool found = false;
>>> + char *s;
>>> +
>>> + /*
>>> + * End the passed name at a newline.
>>> + */
>>> + s = strnchr(value, size, '\n');
>>> + if (s)
>>> + *s = '\0';
>>> +
>>> + if (!strcmp(name, "display")) {
>>> + union security_list_options secid_to_secctx;
>>> + union security_list_options secctx_to_secid;
>>> + union security_list_options socket_getpeersec_stream;
>>> +
>>> + if (size == 0 || size >= 100)
>>> + return -EINVAL;
>>> +
>>> + secid_to_secctx.secid_to_secctx = NULL;
>>> + hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx,
>>> + list) {
>>> + if (size >= strlen(hp->lsm) &&
>>> + !strncmp(value, hp->lsm, size)) {
>>> + secid_to_secctx = hp->hook;
>>> + found = true;
>>> + break;
>>> + }
>>> + }
>>> + secctx_to_secid.secctx_to_secid = NULL;
>>> + hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid,
>>> + list) {
>>> + if (size >= strlen(hp->lsm) &&
>>> + !strncmp(value, hp->lsm, size)) {
>>> + secctx_to_secid = hp->hook;
>>> + found = true;
>>> + break;
>>> + }
>>> + }
>>> + socket_getpeersec_stream.socket_getpeersec_stream = NULL;
>>> + hlist_for_each_entry(hp,
>>> + &security_hook_heads.socket_getpeersec_stream,
>>> + list) {
>>> + if (size >= strlen(hp->lsm) &&
>>> + !strncmp(value, hp->lsm, size)) {
>>> + socket_getpeersec_stream = hp->hook;
>>> + found = true;
>>> + break;
>>> + }
>>> + }
>>> + if (!found)
>>> + return -EINVAL;
>>> +
>>> + /*
>>> + * The named lsm is active and supplies one or more
>>> + * of the relevant hooks. Switch to it.
>>> + */
>>> + s = kmemdup(value, size + 1, GFP_KERNEL);
>>> + if (s == NULL)
>>> + return -ENOMEM;
>>> + s[size] = '\0';
>>> +
>>> + if (loh->lsm)
>>> + kfree(loh->lsm);
>>> + loh->lsm = s;
>>> + loh->secid_to_secctx = secid_to_secctx;
>>> + loh->secctx_to_secid = secctx_to_secid;
>>> + loh->socket_getpeersec_stream = socket_getpeersec_stream;
>> You can't just write to the cred security blob like this; it is a
>> shared data structure, not per-task.
> To be clear, you either need to perform a new = prepare_creds(); /*
> modify new->security as desired */; commit_creds(new); sequence here,
> or use the task security blob instead of the cred security blob. The
> latter seems more amenable to your needs.
You're right. The task blob makes more sense.
^ permalink raw reply
* Re: [PATCH v5 1/2] LSM: SafeSetID: gate setgid transitions
From: James Morris @ 2019-04-10 17:21 UTC (permalink / raw)
To: Micah Morton
Cc: Casey Schaufler, Serge E. Hallyn, Kees Cook, Stephen Smalley,
linux-security-module
In-Reply-To: <CAJ-EccPG2RD75CgiiXzCtLuvay=5VAsdGwqbK1_8opaUtYd2oA@mail.gmail.com>
On Wed, 10 Apr 2019, Micah Morton wrote:
> Lets hold off on merging this for now. We have some fixes that will be
> going in for the existing LSM code and we can circle back to this once
> those have been merged.
Ok.
>
> On Fri, Mar 29, 2019 at 12:44 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> > On 3/29/2019 11:06 AM, James Morris wrote:
> > > On Tue, 5 Mar 2019, mortonm@chromium.org wrote:
> > >
> > >> From: Micah Morton <mortonm@chromium.org>
> > >>
> > >> This patch generalizes the 'task_fix_setuid' LSM hook to enable hooking
> > >> setgid transitions as well as setuid transitions. The hook is renamed to
> > >> 'task_fix_setid'. The patch introduces calls to this hook from the
> > >> setgid functions in kernel/sys.c. This will allow the SafeSetID LSM to
> > >> govern setgid transitions in addition to setuid transitions. This patch
> > >> also makes sure the setgid functions in kernel/sys.c call
> > >> security_capable_setid rather than the ordinary security_capable
> > >> function, so that the security_capable hook in the SafeSetID LSM knows
> > >> it is being invoked from a setid function.
> > >>
> > >> Signed-off-by: Micah Morton <mortonm@chromium.org>
> > > Wondering if there are any further comments or reviews for this before it
> > > is merged?
> >
> > My comments have been addressed.
> >
> >
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 07/10] LSM: SafeSetID: rewrite userspace API to atomic updates
From: Kees Cook @ 2019-04-10 17:24 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190410165605.211749-1-mortonm@chromium.org>
On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> The current API of the SafeSetID LSM uses one write() per rule, and applies
> each written rule instantly. This has several downsides:
>
> - While a policy is being loaded, once a single parent-child pair has been
> loaded, the parent is restricted to that specific child, even if
> subsequent rules would allow transitions to other child UIDs. This means
> that during policy loading, set*uid() can randomly fail.
> - To replace the policy without rebooting, it is necessary to first flush
> all old rules. This creates a time window in which no constraints are
> placed on the use of CAP_SETUID.
> - If we want to perform sanity checks on the final policy, this requires
> that the policy isn't constructed in a piecemeal fashion without telling
> the kernel when it's done.
>
> Other kernel APIs - including things like the userns code and netfilter -
> avoid this problem by performing updates atomically. Luckily, SafeSetID
> hasn't landed in a stable (upstream) release yet, so maybe it's not too
> late to completely change the API.
>
> The new API for SafeSetID is: If you want to change the policy, open
> "safesetid/whitelist_policy" and write the entire policy,
> newline-delimited, in there.
So the entire policy is expected to be sent in a single write() call?
open()
write(policy1)
write(policy2)
close()
means only policy2 is active? I thought policy was meant to be built
over time? i.e. new policy could get appended to existing?
-Kees
>
> Signed-off-by: Jann Horn <jannh@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
> ---
> security/safesetid/lsm.c | 84 +++-----
> security/safesetid/lsm.h | 24 +--
> security/safesetid/securityfs.c | 194 ++++++++++--------
> .../selftests/safesetid/safesetid-test.c | 16 +-
> 4 files changed, 149 insertions(+), 169 deletions(-)
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index ab429e1816c5..4ab4d7cdba31 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -24,28 +24,38 @@
> /* Flag indicating whether initialization completed */
> int safesetid_initialized;
>
> -#define NUM_BITS 8 /* 256 buckets in hash table */
> +struct setuid_ruleset __rcu *safesetid_setuid_rules;
>
> -static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
> -
> -static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
> -
> -static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
> +/* Compute a decision for a transition from @src to @dst under @policy. */
> +enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
> + kuid_t src, kuid_t dst)
> {
> - struct entry *entry;
> + struct setuid_rule *rule;
> enum sid_policy_type result = SIDPOL_DEFAULT;
>
> - rcu_read_lock();
> - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> - entry, next, __kuid_val(src)) {
> - if (!uid_eq(entry->src_uid, src))
> + hash_for_each_possible(policy->rules, rule, next, __kuid_val(src)) {
> + if (!uid_eq(rule->src_uid, src))
> continue;
> - if (uid_eq(entry->dst_uid, dst)) {
> - rcu_read_unlock();
> + if (uid_eq(rule->dst_uid, dst))
> return SIDPOL_ALLOWED;
> - }
> result = SIDPOL_CONSTRAINED;
> }
> + return result;
> +}
> +
> +/*
> + * Compute a decision for a transition from @src to @dst under the active
> + * policy.
> + */
> +static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
> +{
> + enum sid_policy_type result = SIDPOL_DEFAULT;
> + struct setuid_ruleset *pol;
> +
> + rcu_read_lock();
> + pol = rcu_dereference(safesetid_setuid_rules);
> + if (pol)
> + result = _setuid_policy_lookup(pol, src, dst);
> rcu_read_unlock();
> return result;
> }
> @@ -139,52 +149,6 @@ static int safesetid_task_fix_setuid(struct cred *new,
> return -EACCES;
> }
>
> -int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
> -{
> - struct entry *new;
> -
> - /* Return if entry already exists */
> - if (setuid_policy_lookup(parent, child) == SIDPOL_ALLOWED)
> - return 0;
> -
> - new = kzalloc(sizeof(struct entry), GFP_KERNEL);
> - if (!new)
> - return -ENOMEM;
> - new->src_uid = parent;
> - new->dst_uid = child;
> - spin_lock(&safesetid_whitelist_hashtable_spinlock);
> - hash_add_rcu(safesetid_whitelist_hashtable,
> - &new->next,
> - __kuid_val(parent));
> - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> - return 0;
> -}
> -
> -void flush_safesetid_whitelist_entries(void)
> -{
> - struct entry *entry;
> - struct hlist_node *hlist_node;
> - unsigned int bkt_loop_cursor;
> - HLIST_HEAD(free_list);
> -
> - /*
> - * Could probably use hash_for_each_rcu here instead, but this should
> - * be fine as well.
> - */
> - spin_lock(&safesetid_whitelist_hashtable_spinlock);
> - hash_for_each_safe(safesetid_whitelist_hashtable, bkt_loop_cursor,
> - hlist_node, entry, next) {
> - hash_del_rcu(&entry->next);
> - hlist_add_head(&entry->dlist, &free_list);
> - }
> - spin_unlock(&safesetid_whitelist_hashtable_spinlock);
> - synchronize_rcu();
> - hlist_for_each_entry_safe(entry, hlist_node, &free_list, dlist) {
> - hlist_del(&entry->dlist);
> - kfree(entry);
> - }
> -}
> -
> static struct security_hook_list safesetid_security_hooks[] = {
> LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
> LSM_HOOK_INIT(capable, safesetid_security_capable)
> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> index 6806f902794c..4a34f558d964 100644
> --- a/security/safesetid/lsm.h
> +++ b/security/safesetid/lsm.h
> @@ -21,12 +21,6 @@
> /* Flag indicating whether initialization completed */
> extern int safesetid_initialized;
>
> -/* Function type. */
> -enum safesetid_whitelist_file_write_type {
> - SAFESETID_WHITELIST_ADD, /* Add whitelist policy. */
> - 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 */
> @@ -35,18 +29,24 @@ enum sid_policy_type {
>
> /*
> * Hash table entry to store safesetid policy signifying that 'src_uid'
> - * can setid to 'dst_uid'.
> + * can setuid to 'dst_uid'.
> */
> -struct entry {
> +struct setuid_rule {
> 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);
> +#define SETID_HASH_BITS 8 /* 256 buckets in hash table */
> +
> +struct setuid_ruleset {
> + DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
> + struct rcu_head rcu;
> +};
> +
> +enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
> + kuid_t src, kuid_t dst);
>
> -void flush_safesetid_whitelist_entries(void);
> +extern struct setuid_ruleset __rcu *safesetid_setuid_rules;
>
> #endif /* _SAFESETID_H */
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 76c1e8a6ab93..13fce4c10930 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -11,25 +11,15 @@
> * published by the Free Software Foundation.
> *
> */
> +
> +#define pr_fmt(fmt) "SafeSetID: " fmt
> +
> #include <linux/security.h>
> #include <linux/cred.h>
>
> #include "lsm.h"
>
> -static struct dentry *safesetid_policy_dir;
> -
> -struct safesetid_file_entry {
> - const char *name;
> - enum safesetid_whitelist_file_write_type type;
> - struct dentry *dentry;
> -};
> -
> -static struct safesetid_file_entry safesetid_files[] = {
> - {.name = "add_whitelist_policy",
> - .type = SAFESETID_WHITELIST_ADD},
> - {.name = "flush_whitelist_policies",
> - .type = SAFESETID_WHITELIST_FLUSH},
> -};
> +static DEFINE_SPINLOCK(policy_update_lock);
>
> /*
> * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> @@ -37,8 +27,8 @@ static struct safesetid_file_entry safesetid_files[] = {
> * function will return an error.
> * Contents of @buf may be modified.
> */
> -static int parse_policy_line(
> - struct file *file, char *buf, kuid_t *parent, kuid_t *child)
> +static int parse_policy_line(struct file *file, char *buf,
> + struct setuid_rule *rule)
> {
> char *child_str;
> int ret;
> @@ -59,26 +49,103 @@ static int parse_policy_line(
> if (ret)
> return ret;
>
> - *parent = make_kuid(file->f_cred->user_ns, parsed_parent);
> - *child = make_kuid(file->f_cred->user_ns, parsed_child);
> - if (!uid_valid(*parent) || !uid_valid(*child))
> + rule->src_uid = make_kuid(file->f_cred->user_ns, parsed_parent);
> + rule->dst_uid = make_kuid(file->f_cred->user_ns, parsed_child);
> + if (!uid_valid(rule->src_uid) || !uid_valid(rule->dst_uid))
> return -EINVAL;
>
> return 0;
> }
>
> -static int parse_safesetid_whitelist_policy(
> - struct file *file, const char __user *buf, size_t len,
> - kuid_t *parent, kuid_t *child)
> +static void __release_ruleset(struct rcu_head *rcu)
> {
> - char *kern_buf = memdup_user_nul(buf, len);
> - int ret;
> + struct setuid_ruleset *pol =
> + container_of(rcu, struct setuid_ruleset, rcu);
> + int bucket;
> + struct setuid_rule *rule;
> + struct hlist_node *tmp;
> +
> + hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
> + kfree(rule);
> + kfree(pol);
> +}
>
> - if (IS_ERR(kern_buf))
> - return PTR_ERR(kern_buf);
> - ret = parse_policy_line(file, kern_buf, parent, child);
> - kfree(kern_buf);
> - return ret;
> +static void release_ruleset(struct setuid_ruleset *pol)
> +{
> + call_rcu(&pol->rcu, __release_ruleset);
> +}
> +
> +static ssize_t handle_policy_update(struct file *file,
> + const char __user *ubuf, size_t len)
> +{
> + struct setuid_ruleset *pol;
> + char *buf, *p, *end;
> + int err;
> +
> + pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
> + if (!pol)
> + return -ENOMEM;
> + hash_init(pol->rules);
> +
> + p = buf = memdup_user_nul(ubuf, len);
> + if (IS_ERR(buf)) {
> + err = PTR_ERR(buf);
> + goto out_free_pol;
> + }
> +
> + /* policy lines, including the last one, end with \n */
> + while (*p != '\0') {
> + struct setuid_rule *rule;
> +
> + end = strchr(p, '\n');
> + if (end == NULL) {
> + err = -EINVAL;
> + goto out_free_buf;
> + }
> + *end = '\0';
> +
> + rule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
> + if (!rule) {
> + err = -ENOMEM;
> + goto out_free_buf;
> + }
> +
> + err = parse_policy_line(file, p, rule);
> + if (err)
> + goto out_free_rule;
> +
> + if (_setuid_policy_lookup(pol, rule->src_uid, rule->dst_uid) ==
> + SIDPOL_ALLOWED) {
> + pr_warn("bad policy: duplicate entry\n");
> + err = -EEXIST;
> + goto out_free_rule;
> + }
> +
> + hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
> + p = end + 1;
> + continue;
> +
> +out_free_rule:
> + kfree(rule);
> + goto out_free_buf;
> + }
> +
> + /*
> + * Everything looks good, apply the policy and release the old one.
> + * What we really want here is an xchg() wrapper for RCU, but since that
> + * doesn't currently exist, just use a spinlock for now.
> + */
> + spin_lock(&policy_update_lock);
> + rcu_swap_protected(safesetid_setuid_rules, pol,
> + lockdep_is_held(&policy_update_lock));
> + spin_unlock(&policy_update_lock);
> + err = len;
> +
> +out_free_buf:
> + kfree(buf);
> +out_free_pol:
> + release_ruleset(pol);
> + return err;
> }
>
> static ssize_t safesetid_file_write(struct file *file,
> @@ -86,90 +153,45 @@ static ssize_t safesetid_file_write(struct file *file,
> size_t len,
> loff_t *ppos)
> {
> - struct safesetid_file_entry *file_entry =
> - file->f_inode->i_private;
> - kuid_t parent;
> - kuid_t child;
> - int ret;
> -
> if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
> return -EPERM;
>
> if (*ppos != 0)
> return -EINVAL;
>
> - switch (file_entry->type) {
> - case SAFESETID_WHITELIST_FLUSH:
> - flush_safesetid_whitelist_entries();
> - break;
> - case SAFESETID_WHITELIST_ADD:
> - ret = parse_safesetid_whitelist_policy(file, buf, len,
> - &parent, &child);
> - if (ret)
> - return ret;
> -
> - ret = add_safesetid_whitelist_entry(parent, child);
> - if (ret)
> - return ret;
> - break;
> - default:
> - pr_warn("Unknown securityfs file %d\n", file_entry->type);
> - break;
> - }
> -
> - /* Return len on success so caller won't keep trying to write */
> - return len;
> + return handle_policy_update(file, buf, len);
> }
>
> static const struct file_operations safesetid_file_fops = {
> .write = safesetid_file_write,
> };
>
> -static void safesetid_shutdown_securityfs(void)
> -{
> - int i;
> -
> - for (i = 0; i < ARRAY_SIZE(safesetid_files); ++i) {
> - struct safesetid_file_entry *entry =
> - &safesetid_files[i];
> - securityfs_remove(entry->dentry);
> - entry->dentry = NULL;
> - }
> -
> - securityfs_remove(safesetid_policy_dir);
> - safesetid_policy_dir = NULL;
> -}
> -
> static int __init safesetid_init_securityfs(void)
> {
> - int i;
> int ret;
> + struct dentry *policy_dir;
> + struct dentry *policy_file;
>
> if (!safesetid_initialized)
> return 0;
>
> - safesetid_policy_dir = securityfs_create_dir("safesetid", NULL);
> - if (IS_ERR(safesetid_policy_dir)) {
> - ret = PTR_ERR(safesetid_policy_dir);
> + policy_dir = securityfs_create_dir("safesetid", NULL);
> + if (IS_ERR(policy_dir)) {
> + ret = PTR_ERR(policy_dir);
> goto error;
> }
>
> - for (i = 0; i < ARRAY_SIZE(safesetid_files); ++i) {
> - struct safesetid_file_entry *entry =
> - &safesetid_files[i];
> - entry->dentry = securityfs_create_file(
> - entry->name, 0200, safesetid_policy_dir,
> - entry, &safesetid_file_fops);
> - if (IS_ERR(entry->dentry)) {
> - ret = PTR_ERR(entry->dentry);
> - goto error;
> - }
> + policy_file = securityfs_create_file("whitelist_policy", 0200,
> + policy_dir, NULL, &safesetid_file_fops);
> + if (IS_ERR(policy_file)) {
> + ret = PTR_ERR(policy_file);
> + goto error;
> }
>
> return 0;
>
> error:
> - safesetid_shutdown_securityfs();
> + securityfs_remove(policy_dir);
> return ret;
> }
> fs_initcall(safesetid_init_securityfs);
> diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c
> index 892c8e8b1b8b..4f03813d1911 100644
> --- a/tools/testing/selftests/safesetid/safesetid-test.c
> +++ b/tools/testing/selftests/safesetid/safesetid-test.c
> @@ -142,23 +142,17 @@ static void ensure_securityfs_mounted(void)
>
> static void write_policies(void)
> {
> + static char *policy_str =
> + "1:2\n"
> + "1:3\n";
> ssize_t written;
> int fd;
>
> fd = open(add_whitelist_policy_file, O_WRONLY);
> if (fd < 0)
> die("cant open add_whitelist_policy file\n");
> - written = write(fd, "1:2", strlen("1:2"));
> - if (written != strlen("1:2")) {
> - if (written >= 0) {
> - die("short write to %s\n", add_whitelist_policy_file);
> - } else {
> - die("write to %s failed: %s\n",
> - add_whitelist_policy_file, strerror(errno));
> - }
> - }
> - written = write(fd, "1:3", strlen("1:3"));
> - if (written != strlen("1:3")) {
> + written = write(fd, policy_str, strlen(policy_str));
> + if (written != strlen(policy_str)) {
> if (written >= 0) {
> die("short write to %s\n", add_whitelist_policy_file);
> } else {
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] Yama: mark local symbols as static
From: James Morris @ 2019-04-10 17:26 UTC (permalink / raw)
To: Jann Horn; +Cc: Kees Cook, linux-kernel, Serge E. Hallyn, linux-security-module
In-Reply-To: <20190326230841.87834-1-jannh@google.com>
On Wed, 27 Mar 2019, Jann Horn wrote:
> sparse complains that Yama defines functions and a variable as non-static
> even though they don't exist in any header. Fix it by making them static.
>
> Signed-off-by: Jann Horn <jannh@google.com>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 08/10] LSM: SafeSetID: add read handler
From: Kees Cook @ 2019-04-10 17:26 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190410165613.212056-1-mortonm@chromium.org>
On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> For debugging a running system, it is very helpful to be able to see what
> policy the system is using. Add a read handler that can dump out a copy of
> the loaded policy.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
> ---
> security/safesetid/lsm.h | 3 +++
> security/safesetid/securityfs.c | 38 +++++++++++++++++++++++++++++++--
> 2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> index 4a34f558d964..9380329fe30a 100644
> --- a/security/safesetid/lsm.h
> +++ b/security/safesetid/lsm.h
> @@ -17,6 +17,7 @@
> #include <linux/types.h>
> #include <linux/uidgid.h>
> #include <linux/hashtable.h>
> +#include <linux/refcount.h>
>
> /* Flag indicating whether initialization completed */
> extern int safesetid_initialized;
> @@ -41,7 +42,9 @@ struct setuid_rule {
>
> struct setuid_ruleset {
> DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
> + char *policy_str;
> struct rcu_head rcu;
> + refcount_t refcount;
> };
refcount seems like overkill? Why not just use the spinlock? Neither
read nor write are "fast path".
-Kees
>
> enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 13fce4c10930..7a08fff2bc14 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -67,12 +67,14 @@ static void __release_ruleset(struct rcu_head *rcu)
>
> hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
> kfree(rule);
> + kfree(pol->policy_str);
> kfree(pol);
> }
>
> static void release_ruleset(struct setuid_ruleset *pol)
> {
> - call_rcu(&pol->rcu, __release_ruleset);
> + if (pol != NULL && refcount_dec_and_test(&pol->refcount))
> + call_rcu(&pol->rcu, __release_ruleset);
> }
>
> static ssize_t handle_policy_update(struct file *file,
> @@ -85,6 +87,8 @@ static ssize_t handle_policy_update(struct file *file,
> pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
> if (!pol)
> return -ENOMEM;
> + refcount_set(&pol->refcount, 1);
> + pol->policy_str = NULL;
> hash_init(pol->rules);
>
> p = buf = memdup_user_nul(ubuf, len);
> @@ -92,6 +96,11 @@ static ssize_t handle_policy_update(struct file *file,
> err = PTR_ERR(buf);
> goto out_free_pol;
> }
> + pol->policy_str = kstrdup(buf, GFP_KERNEL);
> + if (pol->policy_str == NULL) {
> + err = -ENOMEM;
> + goto out_free_buf;
> + }
>
> /* policy lines, including the last one, end with \n */
> while (*p != '\0') {
> @@ -162,7 +171,32 @@ static ssize_t safesetid_file_write(struct file *file,
> return handle_policy_update(file, buf, len);
> }
>
> +static ssize_t safesetid_file_read(struct file *file, char __user *buf,
> + size_t len, loff_t *ppos)
> +{
> + ssize_t res;
> + struct setuid_ruleset *pol;
> + const char *kbuf;
> +
> + rcu_read_lock();
> + pol = rcu_dereference(safesetid_setuid_rules);
> + if (!pol) {
> + rcu_read_unlock();
> + return 0;
> + }
> + if (!refcount_inc_not_zero(&pol->refcount)) {
> + rcu_read_unlock();
> + return -EBUSY;
> + }
> + rcu_read_unlock();
> + kbuf = pol->policy_str;
> + res = simple_read_from_buffer(buf, len, ppos, kbuf, strlen(kbuf));
> + release_ruleset(pol);
> + return res;
> +}
> +
> static const struct file_operations safesetid_file_fops = {
> + .read = safesetid_file_read,
> .write = safesetid_file_write,
> };
>
> @@ -181,7 +215,7 @@ static int __init safesetid_init_securityfs(void)
> goto error;
> }
>
> - policy_file = securityfs_create_file("whitelist_policy", 0200,
> + policy_file = securityfs_create_file("whitelist_policy", 0600,
> policy_dir, NULL, &safesetid_file_fops);
> if (IS_ERR(policy_file)) {
> ret = PTR_ERR(policy_file);
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 09/10] LSM: SafeSetID: verify transitive constrainedness
From: Kees Cook @ 2019-04-10 17:28 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190410165619.212464-1-mortonm@chromium.org>
On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> Someone might write a ruleset like the following, expecting that it
> securely constrains UID 1 to UIDs 1, 2 and 3:
>
> 1:2
> 1:3
>
> However, because no constraints are applied to UIDs 2 and 3, an attacker
> with UID 1 can simply first switch to UID 2, then switch to any UID from
> there. The secure way to write this ruleset would be:
>
> 1:2
> 1:3
> 2:2
> 3:3
>
> , which uses "transition to self" as a way to inhibit the default-allow
> policy without allowing anything specific.
>
> This is somewhat unintuitive. To make sure that policy authors don't
> accidentally write insecure policies because of this, let the kernel verify
> that a new ruleset does not contain any entries that are constrained, but
> transitively unconstrained.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>
> ---
> security/safesetid/securityfs.c | 21 +++++++++++++++++++
> .../selftests/safesetid/safesetid-test.c | 4 +++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 7a08fff2bc14..3ec64487f0e9 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -77,6 +77,23 @@ static void release_ruleset(struct setuid_ruleset *pol)
> call_rcu(&pol->rcu, __release_ruleset);
> }
>
> +static int verify_ruleset(struct setuid_ruleset *pol)
> +{
> + int bucket;
> + struct setuid_rule *rule;
> +
> + hash_for_each(pol->rules, bucket, rule, next) {
> + if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
> + SIDPOL_DEFAULT) {
> + pr_warn("insecure policy rejected: uid %d is constrained but transitively unconstrained through uid %d\n",
> + __kuid_val(rule->src_uid),
> + __kuid_val(rule->dst_uid));
> + return -EINVAL;
> + }
> + }
> + return 0;
> +}
Why fail open? How about having the policy automatically add the
constraints (since the entire policy is known at verification time)?
-Kees
> +
> static ssize_t handle_policy_update(struct file *file,
> const char __user *ubuf, size_t len)
> {
> @@ -139,6 +156,10 @@ static ssize_t handle_policy_update(struct file *file,
> goto out_free_buf;
> }
>
> + err = verify_ruleset(pol);
> + if (err)
> + goto out_free_buf;
> +
> /*
> * Everything looks good, apply the policy and release the old one.
> * What we really want here is an xchg() wrapper for RCU, but since that
> diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c
> index 4f03813d1911..8f40c6ecdad1 100644
> --- a/tools/testing/selftests/safesetid/safesetid-test.c
> +++ b/tools/testing/selftests/safesetid/safesetid-test.c
> @@ -144,7 +144,9 @@ static void write_policies(void)
> {
> static char *policy_str =
> "1:2\n"
> - "1:3\n";
> + "1:3\n"
> + "2:2\n"
> + "3:3\n";
> ssize_t written;
> int fd;
>
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 10/10] LSM: SafeSetID: fix use of literal -1 in capable hook
From: Kees Cook @ 2019-04-10 17:28 UTC (permalink / raw)
To: Micah Morton
Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <20190410165627.212572-1-mortonm@chromium.org>
On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> The capable() hook returns an error number. -EPERM is actually the same as
> -1, so this doesn't make a difference in behavior.
>
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index 4ab4d7cdba31..61b84e20f2dd 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -90,7 +90,7 @@ static int safesetid_security_capable(const struct cred *cred,
> */
> pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> __kuid_val(cred->uid));
> - return -1;
> + return -EPERM;
> }
>
> /*
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] security: don't use RCU accessors for cred->session_keyring
From: James Morris @ 2019-04-10 17:29 UTC (permalink / raw)
To: Jann Horn
Cc: David Howells, Serge E. Hallyn, linux-kernel, keyrings,
linux-security-module
In-Reply-To: <20190327153938.82007-1-jannh@google.com>
On Wed, 27 Mar 2019, Jann Horn wrote:
> sparse complains that a bunch of places in kernel/cred.c access
> cred->session_keyring without the RCU helpers required by the __rcu
> annotation.
>
> cred->session_keyring is written in the following places:
>
> - prepare_kernel_cred() [in a new cred struct]
> - keyctl_session_to_parent() [in a new cred struct]
> - prepare_creds [in a new cred struct, via memcpy]
> - install_session_keyring_to_cred()
> - from install_session_keyring() on new creds
> - from join_session_keyring() on new creds [twice]
> - from umh_keys_init()
> - from call_usermodehelper_exec_async() on new creds
>
> All of these writes are before the creds are committed; therefore,
> cred->session_keyring doesn't need RCU protection.
>
> Remove the __rcu annotation and fix up all existing users that use __rcu.
>
> Signed-off-by: Jann Horn <jannh@google.com>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH] keys: safe concurrent user->{session,uid}_keyring access
From: James Morris @ 2019-04-10 17:31 UTC (permalink / raw)
To: Jann Horn
Cc: David Howells, Serge E. Hallyn, linux-kernel, keyrings,
linux-security-module
In-Reply-To: <20190327155508.144730-1-jannh@google.com>
On Wed, 27 Mar 2019, Jann Horn wrote:
> The current code can perform concurrent updates and reads on
> user->session_keyring and user->uid_keyring. Add a comment to
> struct user_struct to document the nontrivial locking semantics, and use
> READ_ONCE() for unlocked readers and smp_store_release() for writers to
> prevent memory ordering issues.
>
> Fixes: 69664cf16af4 ("keys: don't generate user and user session keyrings unless they're accessed")
> Signed-off-by: Jann Horn <jannh@google.com>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH] security: inode: fix a missing check for securityfs_create_file
From: James Morris @ 2019-04-10 17:34 UTC (permalink / raw)
To: Kangjie Lu; +Cc: pakki001, Serge E. Hallyn, linux-security-module, linux-kernel
In-Reply-To: <20190315210025.17832-1-kjlu@umn.edu>
On Fri, 15 Mar 2019, Kangjie Lu wrote:
> securityfs_create_file may fail. The fix checks its status and
> returns the error code upstream if it fails.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
> ---
> Return the exact error code upstream.
> ---
> security/inode.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/security/inode.c b/security/inode.c
> index b7772a9b315e..667f8b15027d 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -339,6 +339,11 @@ static int __init securityfs_init(void)
> #ifdef CONFIG_SECURITY
> lsm_dentry = securityfs_create_file("lsm", 0444, NULL, NULL,
> &lsm_ops);
> + if (IS_ERR(lsm_dentry)) {
> + unregister_filesystem(&fs_type);
> + sysfs_remove_mount_point(kernel_kobj, "security");
> + return PTR_ERR(lsm_dentry);
> + }
> #endif
> return 0;
> }
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 09/10] LSM: SafeSetID: verify transitive constrainedness
From: Jann Horn @ 2019-04-10 17:36 UTC (permalink / raw)
To: Kees Cook
Cc: Micah Morton, James Morris, Casey Schaufler,
linux-security-module
In-Reply-To: <CAGXu5j+HTSPDR0Sp17=Jr3gRUSOX39dohy0bgxwyL+Wbh9swkQ@mail.gmail.com>
On Wed, Apr 10, 2019 at 7:28 PM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
> > From: Jann Horn <jannh@google.com>
> >
> > Someone might write a ruleset like the following, expecting that it
> > securely constrains UID 1 to UIDs 1, 2 and 3:
> >
> > 1:2
> > 1:3
> >
> > However, because no constraints are applied to UIDs 2 and 3, an attacker
> > with UID 1 can simply first switch to UID 2, then switch to any UID from
> > there. The secure way to write this ruleset would be:
> >
> > 1:2
> > 1:3
> > 2:2
> > 3:3
> >
> > , which uses "transition to self" as a way to inhibit the default-allow
> > policy without allowing anything specific.
> >
> > This is somewhat unintuitive. To make sure that policy authors don't
> > accidentally write insecure policies because of this, let the kernel verify
> > that a new ruleset does not contain any entries that are constrained, but
> > transitively unconstrained.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
> > ---
> > security/safesetid/securityfs.c | 21 +++++++++++++++++++
> > .../selftests/safesetid/safesetid-test.c | 4 +++-
> > 2 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 7a08fff2bc14..3ec64487f0e9 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -77,6 +77,23 @@ static void release_ruleset(struct setuid_ruleset *pol)
> > call_rcu(&pol->rcu, __release_ruleset);
> > }
> >
> > +static int verify_ruleset(struct setuid_ruleset *pol)
> > +{
> > + int bucket;
> > + struct setuid_rule *rule;
> > +
> > + hash_for_each(pol->rules, bucket, rule, next) {
> > + if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
> > + SIDPOL_DEFAULT) {
> > + pr_warn("insecure policy rejected: uid %d is constrained but transitively unconstrained through uid %d\n",
> > + __kuid_val(rule->src_uid),
> > + __kuid_val(rule->dst_uid));
> > + return -EINVAL;
> > + }
> > + }
> > + return 0;
> > +}
>
> Why fail open? How about having the policy automatically add the
> constraints (since the entire policy is known at verification time)?
Are you suggesting to print a warning, automatically add the
constraint, apply the policy, and still return -EINVAL? I guess that
would work.
I think it is a good idea to require userspace to explicitly write
these rules, since implicitly-added rules might make the rules harder
to understand for someone reading a policy file.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox