[07/10] LSM: SafeSetID: rewrite userspace API to atomic updates
diff mbox series

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

Commit Message

Micah Morton April 10, 2019, 4:56 p.m. UTC
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(-)

Comments

Kees Cook April 10, 2019, 5:24 p.m. UTC | #1
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
>
Jann Horn April 10, 2019, 5:47 p.m. UTC | #2
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
Kees Cook April 10, 2019, 6:20 p.m. UTC | #3
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".
Micah Morton May 7, 2019, 3:02 p.m. UTC | #4
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

Patch
diff mbox series

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 {