From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-f66.google.com ([209.85.210.66]:45119 "EHLO mail-ot1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730407AbeKUXPp (ORCPT ); Wed, 21 Nov 2018 18:15:45 -0500 Received: by mail-ot1-f66.google.com with SMTP id 32so4776233ota.12 for ; Wed, 21 Nov 2018 04:41:32 -0800 (PST) MIME-Version: 1.0 References: <20181116131202.26513-1-omosnace@redhat.com> In-Reply-To: From: Ondrej Mosnacek Date: Wed, 21 Nov 2018 13:41:21 +0100 Message-ID: Subject: Re: [PATCH] selinux: always allow mounting submounts To: Paul Moore Cc: selinux@vger.kernel.org, "Eric W . Biederman" , Trond Myklebust , Seth Forshee , linux-fsdevel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Nov 20, 2018 at 11:09 PM Paul Moore wrote: > On Fri, Nov 16, 2018 at 8:12 AM Ondrej Mosnacek 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. > > I think this all sounds reasonable, but please verify this with an > upstream kernel. Upstream our focus is on the upstream kernel > (surprise!), downstream RHEL is your responsibility, not ours :) I tested on RHEL because that's what I can do most conveniently. I don't have a very good workflow/environment for complex testing on upstream right now. I don't expect the results to be any different on the upstream kernel, but I understand your concern. I have been thinking about some patch testing automation using Fedora Rawhide (I hope that's close enough to upstream at least :), so I guess it's time to get scriptin'... I was hoping to get some independent testing after porting the test to the selinux-testsuite... (But I get it, the burden of proof is on my side...) > > > Fixes: 93faccbbfa95 ("fs: Better permission checking for submounts") > > Signed-off-by: Ondrej Mosnacek > > --- > > 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; > > -- > paul moore > www.paul-moore.com -- Ondrej Mosnacek Associate Software Engineer, Security Technologies Red Hat, Inc.