Re: [PATCH v7 0/7] Allow initializing the kernfs node's secctx based on its parent

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

 



On Thu, Mar 21, 2019 at 3:14 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Fri, Feb 22, 2019 at 9:57 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > TL;DR:
> > This series adds a new security hook that allows to initialize the security
> > context of kernfs properly, taking into account the parent context (and
> > possibly other attributes). Kernfs nodes require special handling here, since
> > they are not bound to specific inodes/superblocks, but instead represent the
> > backing tree structure that is used to build the VFS tree when the kernfs
> > tree is mounted.
> >
> > Changes in v7:
> > - simplify the new security hook's interface
> >   - rather than trying to extract kernfs data into common structures, just
> >     pass the kernfs nodes themselves and add helper functions to
> >     <linux/kernfs.h> for accessing their security xattrs
> >   - in case other LSMs need more kernfs node attributes than the file mode
> >     (uid/gid/...), they can simply add new helpers to <linux/kernfs.h> as
> >     needed
> > - refactor "kernfs: use simple_xattrs for security attributes" to keep using
> >   a single common simple_xattrs structure
> >   - turns out having two separate simple_xattrs wouldn't work right (see
> >     the definition of simple_xattr_list() in fs/xattr.c)
> > - drop unnecessary initializations from inode_doinit_use_xattr()
> > - move the IOP_XATTR check out of inode_doinit_use_xattr()
> > - add two kernfs cleanup patches
> >   - these could be applied independently, but the rest of the patches depend on
> >     them, so I'd rather they stay bundled with the rest to avoid cross-tree
> >     conflicts
> >
> > v6: https://lore.kernel.org/selinux/20190214095015.16032-1-omosnace@xxxxxxxxxx/T/
> > Changes in v6:
> > - remove copy-pasted duplicate macro definition
> >
> > v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@xxxxxxxxxx/T/
> > Changes in v5:
> > - fix misplaced semicolon detected by 0day robot
> >
> > v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@xxxxxxxxxx/T/
> > Changes in v4:
> > - reorder and rename hook arguments
> > - avoid allocating kernfs_iattrs unless needed
> >
> > v3: https://lore.kernel.org/selinux/20190130114150.27807-1-omosnace@xxxxxxxxxx/T/
> > Changes in v3:
> > - rename the hook to "kernfs_init_security"
> > - change the hook interface to simply pass pointers to struct iattr and
> >   struct simple_xattrs of both the new node and its parent
> > - add full security xattr support to kernfs (and fixup SELinux behavior
> >   to handle it properly)
> >
> > v2: https://lore.kernel.org/selinux/20190109162830.8309-1-omosnace@xxxxxxxxxx/T/
> > Changes in v2:
> > - add docstring for the new hook in union security_list_options
> > - initialize *ctx to NULL and *ctxlen to 0 in case the hook is not
> >   implemented
> >
> > v1: https://lore.kernel.org/selinux/20190109091028.24485-1-omosnace@xxxxxxxxxx/T/
> >
> > The kernfs nodes initially do not store any security context and rely on
> > the LSM to assign some default context to inodes created over them. Kernfs
> > inodes, however, allow setting an explicit context via the *setxattr(2)
> > syscalls, in which case the context is stored inside the kernfs node's
> > internal structure.
> >
> > SELinux (and possibly other LSMs) initialize the context of newly created
> > FS objects based on the parent object's context (usually the child inherits
> > the parent's context, unless the policy dictates otherwise). This is done
> > by hooking the creation of the new inode corresponding to the newly created
> > file/directory via security_inode_init_security() (most filesystems always
> > create a fresh inode when a new FS object is created). However, kernfs nodes
> > can be created "behind the scenes" while the filesystem is not mounted
> > anywhere and thus no inodes can exist for them yet.
> >
> > Therefore, to allow maintaining similar behavior for kernfs nodes, a new
> > LSM hook is needed, which will allow initializing the kernfs node's
> > security context based on its own attributes and those of the parent's
> > node.
> >
> > The main motivation for this change is that the userspace users of cgroupfs
> > (which is built on kernfs) expect the usual security context inheritance
> > to work under SELinux (see [1] and [2]). This functionality is required for
> > better confinement of containers under SELinux.
> >
> > Patch 1/7 simplifies the kernfs_iattrs structure and patch 2/7 optimizes
> > kernfs to not allocate kernfs_iattrs when getting the value of an xattr.
> >
> > Patch 3/7 changes SELinux to fetch security context from extended
> > attributes on kernfs filesystems, falling back to genfs-defined context
> > if that fails. Without this patch the 4/7 would be a regression for
> > SELinux (due to the removal of ...notifysecctx() call.
> >
> > Patch 4/7 implements full security xattr support in kernfs using
> > simple_xattrs; patch 5/7 adds the new LSM hook; patch 6/7 implements the
> > new hook in SELinux; and patch 7/7 modifies kernfs to call the new hook
> > on new node creation.
> >
> > Testing:
> > - passed the reproducer from the commit message of the last patch
> > - passed SELinux testsuite on Fedora Rawhide (x86_64) when applied on top
> >   of current Rawhide kernel (5.0.0-0.rc7.git2.1) [3]
> >   - including the new proposed selinux-testsuite subtest [4] (adapted
> >     from the reproducer)
> >
> > [1] https://github.com/SELinuxProject/selinux-kernel/issues/39
> > [2] https://bugzilla.redhat.com/show_bug.cgi?id=1553803
> > [3] https://koji.fedoraproject.org/koji/taskinfo?taskID=32963825
> > [4] https://github.com/SELinuxProject/selinux-testsuite/pull/48
> >
> > Ondrej Mosnacek (7):
> >   kernfs: clean up struct kernfs_iattrs
> >   kernfs: do not alloc iattrs in kernfs_xattr_get
> >   selinux: try security xattr after genfs for kernfs filesystems
> >   kernfs: use simple_xattrs for security attributes
> >   LSM: add new hook for kernfs node initialization
> >   selinux: implement the kernfs_init_security hook
> >   kernfs: initialize security of newly created nodes
> >
> >  fs/kernfs/dir.c                     |  28 ++--
> >  fs/kernfs/inode.c                   | 166 +++++++++------------
> >  fs/kernfs/kernfs-internal.h         |   8 +-
> >  fs/kernfs/symlink.c                 |   4 +-
> >  include/linux/kernfs.h              |  15 ++
> >  include/linux/lsm_hooks.h           |  13 ++
> >  include/linux/security.h            |   9 ++
> >  security/security.c                 |   6 +
> >  security/selinux/hooks.c            | 223 +++++++++++++++++++---------
> >  security/selinux/include/security.h |   1 +
> >  10 files changed, 290 insertions(+), 183 deletions(-)
>
> I just merged this patchset into selinux/next, thanks everyone.
>
> Ondrej, every patch in the patchset except for one required some
> amount of merge fixups, please take a look and make sure everything in
> the selinux/next branch still looks right to you.

Looks good to me, thanks!  I checked by rebasing the original branch
on v5.1-rc1 myself and then comparing my result with selinux/next.
The only difference was one extra empty line on my side, but that
doesn't bother me :)

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.



[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