Re: [PATCH 3/3] kernfs: Initialize security of newly created nodes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 9, 2019 at 10:42 AM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On 1/9/19 4:10 AM, Ondrej Mosnacek wrote:
> > Use the new security_object_init_security() hook to allow LSMs to
> > possibly assign a non-default security context to newly created nodes
> > based on the context of their parent node.
> >
> > This fixes an issue with cgroupfs under SELinux, where newly created
> > cgroup subdirectories would not inherit its parent's context if it had
> > been set explicitly to a non-default value (other than the genfs context
> > specified by the policy). This can be reproduced as follows:
> >
> >      # mkdir /sys/fs/cgroup/unified/test
> >      # chcon -R system_u:object_r:cgroup_t:s0:c123 /sys/fs/cgroup/unified/test
> >      # ls -lZ /sys/fs/cgroup/unified
> >      total 0
> >      -r--r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.controllers
> >      -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.max.depth
> >      -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.max.descendants
> >      -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.procs
> >      -r--r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.stat
> >      -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.subtree_control
> >      -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.threads
> >      drwxr-xr-x.  2 root root system_u:object_r:cgroup_t:s0      0 Jan  8 04:54 init.scope
> >      drwxr-xr-x. 25 root root system_u:object_r:cgroup_t:s0      0 Jan  8 04:54 system.slice
> >      drwxr-xr-x.  2 root root system_u:object_r:cgroup_t:s0:c123 0 Jan  8 04:59 test
> >      drwxr-xr-x.  3 root root system_u:object_r:cgroup_t:s0      0 Jan  8 04:55 user.slice
> >      # mkdir /sys/fs/cgroup/unified/test/subdir
> >
> > Actual result:
> >
> >      # ls -ldZ /sys/fs/cgroup/unified/test/subdir
> >      drwxr-xr-x. 2 root root system_u:object_r:cgroup_t:s0 0 Jan  8 05:10 /sys/fs/cgroup/unified/test/subdir
> >
> > Expected result:
> >
> >      # ls -ldZ /sys/fs/cgroup/unified/test/subdir
> >      drwxr-xr-x. 2 root root unconfined_u:object_r:cgroup_t:s0:c123 0 Jan  8 05:10 /sys/fs/cgroup/unified/test/subdir
> >
> > Link: https://github.com/SELinuxProject/selinux-kernel/issues/39
> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
>
> Reviewed-by: Stephen Smalley <sds@xxxxxxxxxxxxx>

Looks okay to me as well.

Some part of me wonders if it might be smart to check for non-NULL
secdata being returned from kernfs_node_setsecdata and doing a WARN.
Maybe I'm overly pessimistic, but I'm pretty sure
kernfs_node_init_security() will get improperly used at some point.
At least we have a comment to *maybe* warn future abusers.

> > ---
> >   fs/kernfs/dir.c             | 49 ++++++++++++++++++++++++++++++++++---
> >   fs/kernfs/inode.c           |  9 +++----
> >   fs/kernfs/kernfs-internal.h |  4 +++
> >   3 files changed, 54 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> > index 4ca0b5c18192..8a678a934f65 100644
> > --- a/fs/kernfs/dir.c
> > +++ b/fs/kernfs/dir.c
> > @@ -15,6 +15,7 @@
> >   #include <linux/slab.h>
> >   #include <linux/security.h>
> >   #include <linux/hash.h>
> > +#include <linux/stringhash.h>
> >
> >   #include "kernfs-internal.h"
> >
> > @@ -617,7 +618,43 @@ struct kernfs_node *kernfs_node_from_dentry(struct dentry *dentry)
> >       return NULL;
> >   }
> >
> > -static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
> > +static int kernfs_node_init_security(struct kernfs_node *parent,
> > +                                  struct kernfs_node *kn, umode_t mode)
> > +{
> > +     struct kernfs_iattrs *attrs;
> > +     struct qstr q;
> > +     void *ctx;
> > +     u32 ctxlen;
> > +     int ret;
> > +
> > +     /* If parent has no explicit context set, leave child unset as well */
> > +     if (!parent->iattr)
> > +             return 0;
> > +     if (!parent->iattr->ia_secdata || !parent->iattr->ia_secdata_len)
> > +             return 0;
> > +
> > +     q.name = kn->name;
> > +     q.hash_len = hashlen_string(parent, kn->name);
> > +
> > +     ret = security_object_init_security(parent->iattr->ia_secdata,
> > +                                         parent->iattr->ia_secdata_len,
> > +                                         &q, (u16)mode, &ctx, &ctxlen);
> > +     if (ret)
> > +             return ret;
> > +
> > +     attrs = kernfs_iattrs(kn);
> > +     if (!attrs) {
> > +             security_release_secctx(ctx, ctxlen);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     kernfs_node_setsecdata(attrs, &ctx, &ctxlen);
> > +     /* The inode is fresh, so the returned ctx is always NULL. */
> > +     return 0;
> > +}
> > +
> > +static struct kernfs_node *__kernfs_new_node(struct kernfs_node *parent,
> > +                                          struct kernfs_root *root,
> >                                            const char *name, umode_t mode,
> >                                            kuid_t uid, kgid_t gid,
> >                                            unsigned flags)
> > @@ -674,6 +711,12 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
> >                       goto err_out3;
> >       }
> >
> > +     if (parent) {
> > +             ret = kernfs_node_init_security(parent, kn, mode);
> > +             if (ret)
> > +                     goto err_out3;
> > +     }
> > +
> >       return kn;
> >
> >    err_out3:
> > @@ -692,7 +735,7 @@ struct kernfs_node *kernfs_new_node(struct kernfs_node *parent,
> >   {
> >       struct kernfs_node *kn;
> >
> > -     kn = __kernfs_new_node(kernfs_root(parent),
> > +     kn = __kernfs_new_node(parent, kernfs_root(parent),
> >                              name, mode, uid, gid, flags);
> >       if (kn) {
> >               kernfs_get(parent);
> > @@ -962,7 +1005,7 @@ struct kernfs_root *kernfs_create_root(struct kernfs_syscall_ops *scops,
> >       INIT_LIST_HEAD(&root->supers);
> >       root->next_generation = 1;
> >
> > -     kn = __kernfs_new_node(root, "", S_IFDIR | S_IRUGO | S_IXUGO,
> > +     kn = __kernfs_new_node(NULL, root, "", S_IFDIR | S_IRUGO | S_IXUGO,
> >                              GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
> >                              KERNFS_DIR);
> >       if (!kn) {
> > diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> > index 80cebcd94c90..e6db8d23437b 100644
> > --- a/fs/kernfs/inode.c
> > +++ b/fs/kernfs/inode.c
> > @@ -31,7 +31,7 @@ static const struct inode_operations kernfs_iops = {
> >       .listxattr      = kernfs_iop_listxattr,
> >   };
> >
> > -static struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
> > +struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
> >   {
> >       static DEFINE_MUTEX(iattr_mutex);
> >       struct kernfs_iattrs *ret;
> > @@ -135,8 +135,8 @@ out:
> >       return error;
> >   }
> >
> > -static int kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
> > -                               u32 *secdata_len)
> > +void kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
> > +                         u32 *secdata_len)
> >   {
> >       void *old_secdata;
> >       size_t old_secdata_len;
> > @@ -149,7 +149,6 @@ static int kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
> >
> >       *secdata = old_secdata;
> >       *secdata_len = old_secdata_len;
> > -     return 0;
> >   }
> >
> >   ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size)
> > @@ -365,7 +364,7 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
> >               return error;
> >
> >       mutex_lock(&kernfs_mutex);
> > -     error = kernfs_node_setsecdata(attrs, &secdata, &secdata_len);
> > +     kernfs_node_setsecdata(attrs, &secdata, &secdata_len);
> >       mutex_unlock(&kernfs_mutex);
> >
> >       if (secdata)
> > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
> > index 3d83b114bb08..f6fb2df24c30 100644
> > --- a/fs/kernfs/kernfs-internal.h
> > +++ b/fs/kernfs/kernfs-internal.h
> > @@ -92,6 +92,10 @@ int kernfs_iop_getattr(const struct path *path, struct kstat *stat,
> >   ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size);
> >   int __kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr);
> >
> > +struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn);
> > +void kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
> > +                         u32 *secdata_len);
> > +
> >   /*
> >    * dir.c
> >    */
> >
>


-- 
paul moore
www.paul-moore.com



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [Monitors]

  Powered by Linux