Message ID | 20190222145718.5740-1-omosnace@redhat.com (mailing list archive) |
---|---|
Headers | show |
Series | Allow initializing the kernfs node's secctx based on its parent | expand |
On Fri, Feb 22, 2019 at 3:57 PM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com/T/ > Changes in v6: > - remove copy-pasted duplicate macro definition > > v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@redhat.com/T/ > Changes in v5: > - fix misplaced semicolon detected by 0day robot > > v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@redhat.com/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@redhat.com/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@redhat.com/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@redhat.com/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(-) > > -- > 2.20.1 Ping about this series... Casey, are you OK with this new version?
On Wed, Mar 6, 2019 at 10:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Fri, Feb 22, 2019 at 3:57 PM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com/T/ > > Changes in v6: > > - remove copy-pasted duplicate macro definition > > > > v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@redhat.com/T/ > > Changes in v5: > > - fix misplaced semicolon detected by 0day robot > > > > v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@redhat.com/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@redhat.com/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@redhat.com/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@redhat.com/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(-) > > > > -- > > 2.20.1 > > Ping about this series... Casey, are you OK with this new version? We've got two weeks, well one and a half weeks, to decide as the earliest I would merge this would be after the merge window closes :)
On 3/6/2019 7:54 AM, Ondrej Mosnacek wrote: > On Fri, Feb 22, 2019 at 3:57 PM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com/T/ >> Changes in v6: >> - remove copy-pasted duplicate macro definition >> >> v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@redhat.com/T/ >> Changes in v5: >> - fix misplaced semicolon detected by 0day robot >> >> v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@redhat.com/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@redhat.com/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@redhat.com/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@redhat.com/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(-) >> >> -- >> 2.20.1 > Ping about this series... Casey, are you OK with this new version? I'm still not wildly enthusiastic about it, but I can't offer a better solution right now. You can add my Acked-by: Casey Schaufler <casey@schaufler-ca.com>
On Wed, Mar 6, 2019 at 5:20 PM Paul Moore <paul@paul-moore.com> wrote: > On Wed, Mar 6, 2019 at 10:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Fri, Feb 22, 2019 at 3:57 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > [...] > > > > Ping about this series... Casey, are you OK with this new version? > > We've got two weeks, well one and a half weeks, to decide as the > earliest I would merge this would be after the merge window closes :) Sure, I just wanted to know early if there would be any more changes needed. -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc.
On Wed, Mar 6, 2019 at 7:04 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 3/6/2019 7:54 AM, Ondrej Mosnacek wrote: > > On Fri, Feb 22, 2019 at 3:57 PM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com/T/ > >> Changes in v6: > >> - remove copy-pasted duplicate macro definition > >> > >> v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@redhat.com/T/ > >> Changes in v5: > >> - fix misplaced semicolon detected by 0day robot > >> > >> v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@redhat.com/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@redhat.com/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@redhat.com/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@redhat.com/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(-) > >> > >> -- > >> 2.20.1 > > Ping about this series... Casey, are you OK with this new version? > > I'm still not wildly enthusiastic about it, but I can't > offer a better solution right now. You can add my > > Acked-by: Casey Schaufler <casey@schaufler-ca.com> Thanks!
On Fri, Feb 22, 2019 at 9:57 AM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com/T/ > Changes in v6: > - remove copy-pasted duplicate macro definition > > v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@redhat.com/T/ > Changes in v5: > - fix misplaced semicolon detected by 0day robot > > v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@redhat.com/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@redhat.com/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@redhat.com/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@redhat.com/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.
On Thu, Mar 21, 2019 at 3:14 AM Paul Moore <paul@paul-moore.com> wrote: > On Fri, Feb 22, 2019 at 9:57 AM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com/T/ > > Changes in v6: > > - remove copy-pasted duplicate macro definition > > > > v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@redhat.com/T/ > > Changes in v5: > > - fix misplaced semicolon detected by 0day robot > > > > v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@redhat.com/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@redhat.com/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@redhat.com/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@redhat.com/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 :)
On Thu, Mar 21, 2019 at 4:56 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Mar 21, 2019 at 3:14 AM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Feb 22, 2019 at 9:57 AM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com/T/ > > > Changes in v6: > > > - remove copy-pasted duplicate macro definition > > > > > > v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@redhat.com/T/ > > > Changes in v5: > > > - fix misplaced semicolon detected by 0day robot > > > > > > v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@redhat.com/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@redhat.com/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@redhat.com/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@redhat.com/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 :) Thanks for double-checking.