| Message ID | 20190410165605.211749-1-mortonm@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] LSM: SafeSetID: fix pr_warn() to include newline | expand |
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 >
On Wed, Apr 10, 2019 at 7:24 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> > > > > 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? No; if you do that, the first write() sets policy1, and the second write() fails with -EINVAL because of the "if (*ppos != 0) return -EINVAL;" in safesetid_file_write() (which already exists in the current version of the LSM). > I thought policy was meant to be built > over time? i.e. new policy could get appended to existing? That's what the current API does; as I've explained in the commit message, I think that that's a bad idea. Are you asking because you have a usecase where you actually want to "append" rules after loading an initial policy? If so, I think that the simplest way to do that would be to have userspace concatenate the policies and then shove the result of that into the kernel. Otherwise: - you'd need a way to distinguish between policy replacement and appending to policy - to securely replace an existing policy, userspace would always have to concatenate all the new policy fragments anyway
On Wed, Apr 10, 2019 at 10:47 AM Jann Horn <jannh@google.com> wrote: > > On Wed, Apr 10, 2019 at 7:24 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> > > > > > > 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? > > No; if you do that, the first write() sets policy1, and the second > write() fails with -EINVAL because of the "if (*ppos != 0) return > -EINVAL;" in safesetid_file_write() (which already exists in the > current version of the LSM). Ah yes, thanks! I missed that check. Good! > > > I thought policy was meant to be built > > over time? i.e. new policy could get appended to existing? > > That's what the current API does; as I've explained in the commit > message, I think that that's a bad idea. Okay, sounds fine. It wasn't clear to me from the commit message if you meant "write the whole policy during a single open/close" or "write whole policy with a single initial write".
Ready for merge. On Wed, Apr 10, 2019 at 11:20 AM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Apr 10, 2019 at 10:47 AM Jann Horn <jannh@google.com> wrote: > > > > On Wed, Apr 10, 2019 at 7:24 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> > > > > > > > > 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? > > > > No; if you do that, the first write() sets policy1, and the second > > write() fails with -EINVAL because of the "if (*ppos != 0) return > > -EINVAL;" in safesetid_file_write() (which already exists in the > > current version of the LSM). > > Ah yes, thanks! I missed that check. Good! > > > > > > I thought policy was meant to be built > > > over time? i.e. new policy could get appended to existing? > > > > That's what the current API does; as I've explained in the commit > > message, I think that that's a bad idea. > > Okay, sounds fine. It wasn't clear to me from the commit message if > you meant "write the whole policy during a single open/close" or > "write whole policy with a single initial write". > > -- > Kees Cook
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 {