Re: [PATCH] selinux: always allow mounting submounts

[Paul Moore <paul@paul-moore.com>]

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 :)

> Fixes: 93faccbbfa95 ("fs: Better permission checking for submounts")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  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