Re: [PATCH] selinux: always allow mounting submounts

[ebiederm@xmission.com (Eric W. Biederman)]

Paul Moore <paul@paul-moore.com> writes:

> On Wed, Nov 21, 2018 at 10:38 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>> On Wed, Nov 21, 2018 at 1:41 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>> > On Tue, Nov 20, 2018 at 11:09 PM Paul Moore <paul@paul-moore.com> wrote:
>> > > On Fri, Nov 16, 2018 at 8:12 AM Ondrej Mosnacek <omosnace@redhat.com> 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 have now tested it on Fedora Rawhide with a scratch kernel with this
>> patch applied [1] (x86_64 only). I ran the whole selinux-testsuite
>> with the submount test [2] and everything passed (except for the known
>> overlay failures and skipped binder test) ...
>
> Merged into selinux/next, thanks.


A few late comments on this.

The change mentioned in fixes did not remove a SB_KERNMOUNT so I don't
see how it is a fix for that.  That change just added SB_SUBMOUNT so you
can test for and detect this situation.  Are you seeing something that I
am not in that change?

I expect what we need for the long term is to move sb_kern_mount except
for the security mount option bits into do_new_mount so security modules
don't have to perform funny checks because the security hook is in the
wrong place.

Further as far as I can tell from reading the code every filesystem that
performs submounts except for nfs is broken.  As no one else calls
security_sb_clone_mnt_opts.  Instead the normal mnt_opts hooks are
called with no security mount options.

Which leads me to point that smack doesn't even implement
sb_clone_mnt_opts so I expect smack gets the security mount options
wrong.

Is it common to specify the security mount options on filesystems?
I see the code.  I see what needs to be done to keep them working.
(Commas in options names ick).  I don't understand how they are used and
how common they are.

I care because the vfs is in the middle of some work to clean up this
side of mounting and at the very least I am review changes and spotting
bugs.  Understanding how the security mount options work from the
perspective of someone who actually uses them would be a real help.

Eric