On Fri, Nov 16, 2018 at 2:12 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > If a superblock has the MS_SUBMOUNT flag set, we should always allow > mounting it. These mounts are done automatically by the kernel either as > part of mounting some parent mount (e.g. debugfs always mounts tracefs > under "tracing" for compatibility) or they are mounted automatically as > needed on subdirectory accesses (e.g. NFS crossmnt mounts). Since such > automounts are either an implicit consequence of the parent mount (which > is already checked) or they can happen during regular accesses (where it > doesn't make sense to check against the current task's context), the > mount permission check should be skipped for them. > > Without this patch, attempts to access contents of an automounted > directory can cause unexpected SELinux denials. > > In the current kernel tree, the MS_SUBMOUNT flag is set only via > vfs_submount(), which is called only from the following places: > - AFS, when automounting special "symlinks" referencing other cells > - CIFS, when automounting "referrals" > - NFS, when automounting subtrees > - debugfs, when automounting tracefs > > In all cases the submounts are meant to be transparent to the user and > it makes sense that if mounting the master is allowed, then so should be > the automounts. Note that CAP_SYS_ADMIN capability checking is already > skipped for (SB_KERNMOUNT|SB_SUBMOUNT) in: > - sget_userns() in fs/super.c: > if (!(flags & (SB_KERNMOUNT|SB_SUBMOUNT)) && > !(type->fs_flags & FS_USERNS_MOUNT) && > !capable(CAP_SYS_ADMIN)) > return ERR_PTR(-EPERM); > - sget() in fs/super.c: > /* Ensure the requestor has permissions over the target filesystem */ > if (!(flags & (SB_KERNMOUNT|SB_SUBMOUNT)) && !ns_capable(user_ns, CAP_SYS_ADMIN)) > return ERR_PTR(-EPERM); > > Verified internally on patched RHEL 7.6 with a reproducer using > NFS+httpd and selinux-tesuite. FYI, I have now submitted a pull request that adds a test for this bug to the selinux-testsuite: https://github.com/SELinuxProject/selinux-testsuite/pull/43 > > Fixes: 93faccbbfa95 ("fs: Better permission checking for submounts") > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx> > --- > security/selinux/hooks.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 7ce683259357..7ce012d9ec51 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2934,7 +2934,7 @@ static int selinux_sb_kern_mount(struct super_block *sb, int flags, void *data) > return rc; > > /* Allow all mounts performed by the kernel */ > - if (flags & MS_KERNMOUNT) > + if (flags & (MS_KERNMOUNT | MS_SUBMOUNT)) > return 0; > > ad.type = LSM_AUDIT_DATA_DENTRY; > -- > 2.17.2 > -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc.
