[PATCH 3/3] kernfs: Initialize security of newly created nodes
Paul Moore
paul at paul-moore.com
Fri Jan 11 02:08:21 UTC 2019
On Wed, Jan 9, 2019 at 10:42 AM Stephen Smalley <sds at tycho.nsa.gov> 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 at redhat.com>
>
> Reviewed-by: Stephen Smalley <sds at tycho.nsa.gov>
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
More information about the Linux-security-module-archive
mailing list