On 12/18/18 10:50 AM, Ondrej Mosnacek wrote:
On Thu, Dec 13, 2018 at 5:25 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
On 12/13/18 9:17 AM, Ondrej Mosnacek wrote:
Ignore all selinux_inode_notifysecctx() calls on mounts with the
SECURITY_FS_USE_MNTPOINT behavior.
This fixes behavior of kernfs-based filesystems when mounted with the
'context=' option. Before this patch, if a node's context had been
explicitly set to a non-default value and later the filesystem has been
remounted with the 'context=' option, then this node would show up as
having a different context.
Steps to reproduce:
# mount -t cgroup2 cgroup2 /sys/fs/cgroup/unified
# chcon unconfined_u:object_r:user_home_t:s0 /sys/fs/cgroup/unified/cgroup.stat
# ls -lZ /sys/fs/cgroup/unified
total 0
-r--r--r--. 1 root root system_u:object_r:cgroup_t:s0 0 Dec 13 10:41 cgroup.controllers
-rw-r--r--. 1 root root system_u:object_r:cgroup_t:s0 0 Dec 13 10:41 cgroup.max.depth
-rw-r--r--. 1 root root system_u:object_r:cgroup_t:s0 0 Dec 13 10:41 cgroup.max.descendants
-rw-r--r--. 1 root root system_u:object_r:cgroup_t:s0 0 Dec 13 10:41 cgroup.procs
-r--r--r--. 1 root root unconfined_u:object_r:user_home_t:s0 0 Dec 13 10:41 cgroup.stat
-rw-r--r--. 1 root root system_u:object_r:cgroup_t:s0 0 Dec 13 10:41 cgroup.subtree_control
-rw-r--r--. 1 root root system_u:object_r:cgroup_t:s0 0 Dec 13 10:41 cgroup.threads
# umount /sys/fs/cgroup/unified
# mount -o context=system_u:object_r:tmpfs_t:s0 -t cgroup2 cgroup2 /sys/fs/cgroup/unified
Result before:
# ls -lZ /sys/fs/cgroup/unified
total 0
-r--r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.controllers
-rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.max.depth
-rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.max.descendants
-rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.procs
-r--r--r--. 1 root root unconfined_u:object_r:user_home_t:s0 0 Dec 13 10:41 cgroup.stat
-rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.subtree_control
-rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.threads
Result after:
# ls -lZ /sys/fs/cgroup/unified
total 0
-r--r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.controllers
-rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.max.depth
-rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.max.descendants
-rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.procs
-r--r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.stat
-rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.subtree_control
-rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.threads
Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
---
security/selinux/hooks.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d6d29ec54eab..0ca5ed30afe1 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6620,6 +6620,13 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
*/
static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
{
+ struct superblock_security_struct *sbsec = inode->i_sb->s_security;
+
+ /* Do not change context in SECURITY_FS_USE_MNTPOINT case */
+ if ((sbsec->flags & SE_SBINITIALIZED) &&
+ (sbsec->behavior == SECURITY_FS_USE_MNTPOINT))
+ return 0;
+
return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
}
Wondering if we ought to take this into selinux_inode_setsecurity() and
return -EOPNOTSUPP in that case. We already return -EOPNOTSUPP from
selinux_inode_setxattr() if (!(sbsec->flags & SBLABEL_MNT)) and that
should precede other calls to selinux_inode_setsecurity() IIRC.
Maybe, but see below. In selinux_inode_setsecurity() we should indeed
check for SBLABEL_MNT, but only if it is called directly as a hook
(but I'm not sure if it is worth it in this case, since as you say, a
prior selinux_inode_setxattr() failure should always prevent this hook
from being called). selinux_inode_notifysecctx() has a bit different
semantics, IMHO.
Should we just be checking SBLABEL_MNT here instead?
I don't think so. IIUC, the purpose of selinux_inode_notifysecctx() is
to adjust the sid that has been assigned by selinux_d_instantiate() by
the label that is 'stored' for the particular node internally by the
filesystem. I would say the fact whether we want to use the stored
label depends on the sbsec->behavior value (BTW, shouldn't we also
return 0 in case of SECURITY_FS_USE_TASK? or even
SECURITY_FS_USE_TRANS?). I understand the SBLABEL_MNT flag more as an
indication of whether we want the user to allow setting the label
explicitly (and probably also implicitly via tsec->create_sid).
selinux_inode_notifysecctx() provides the filesystem with a way to push
a label for an inode to the security module as opposed to having the
security module pull it via __vfs_getxattr. The latter doesn't work
well for NFSv4.2 security labeling nor for sysfs, albeit for different
reasons.
SBLABEL_MNT is not so much whether we want to allow the user to do it
but rather whether the filesystem and policy make it safe to do so. It
is set mostly based on the ->behavior with exceptions for the
whitelisted filesystem types. The same flag is checked in
selinux_inode_init_security(), where we are returning a label for a
newly created file.
Not sure we want to create two different ways of determining whether the
filesystem supports labeling.
And do we need to separately check SE_SBINITIALIZED?
I'm not sure, but other places in the code check that flag before
checking sbsec->behavior, so it seemed to me as the right thing to do.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
