On Wed, Nov 21, 2018 at 10:38 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > On Wed, Nov 21, 2018 at 1:41 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > On Tue, Nov 20, 2018 at 11:09 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > On Fri, Nov 16, 2018 at 8:12 AM 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. > > > > > > 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. -- paul moore www.paul-moore.com
