[PATCH] selinux: always allow mounting submounts

[PATCH] selinux: always allow mounting submounts

[Ondrej Mosnacek]

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.

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;
-- 
2.17.2

Re: [PATCH] selinux: always allow mounting submounts

[Ondrej Mosnacek]

On Fri, Nov 16, 2018 at 2:12 PM 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.

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@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;
> --
> 2.17.2
>

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH] selinux: always allow mounting submounts

[Paul Moore]

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

Re: [PATCH] selinux: always allow mounting submounts

[Ondrej Mosnacek]

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 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 <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
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH] selinux: always allow mounting submounts

[Ondrej Mosnacek]

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

domain_trans/test ........... ok
entrypoint/test ............. ok
execshare/test .............. ok
exectrace/test .............. ok
execute_no_trans/test ....... ok
fdreceive/test .............. ok
inherit/test ................ ok
link/test ................... ok
mkdir/test .................. ok
msg/test .................... ok
open/test ................... ok
ptrace/test ................. ok
readlink/test ............... ok
relabel/test ................ ok
rename/test ................. ok
rxdir/test .................. ok
sem/test .................... ok
setattr/test ................ ok
setnice/test ................ ok
shm/test .................... ok
sigkill/test ................ ok
stat/test ................... ok
sysctl/test ................. ok
task_create/test ............ ok
task_setnice/test ........... ok
task_setscheduler/test ...... ok
task_getscheduler/test ...... ok
task_getsid/test ............ ok
task_getpgid/test ........... ok
task_setpgid/test ........... ok
file/test ................... ok
ioctl/test .................. ok
capable_file/test ........... ok
capable_net/test ............ ok
capable_sys/test ............ ok
dyntrans/test ............... ok
dyntrace/test ............... ok
bounds/test ................. ok
nnp_nosuid/test ............. ok
mmap/test ................... ok
unix_socket/test ............ ok
inet_socket/test ............ ok
overlay/test ................ 62/119
#   Failed test at overlay/test line 275.

#   Failed test at overlay/test line 293.
overlay/test ................ 97/119
#   Failed test at overlay/test line 547.

#   Failed test at overlay/test line 622.
# Looks like you failed 4 tests of 119.
overlay/test ................ Dubious, test returned 4 (wstat 1024, 0x400)
Failed 4/119 subtests
checkreqprot/test ........... ok
mqueue/test ................. ok
mac_admin/test .............. ok
atsecure/test ............... ok
submount/test ............... ok
cap_userns/test ............. ok
extended_socket_class/test .. ok
sctp/test ................... ok
netlink_socket/test ......... ok
prlimit/test ................ ok
binder/test ................. skipped: Binder not supported by kernel

Test Summary Report
-------------------
overlay/test              (Wstat: 1024 Tests: 119 Failed: 4)
 Failed tests:  81, 83, 107, 112
 Non-zero exit status: 4
Files=54, Tests=615, 133 wallclock secs ( 0.45 usr  0.28 sys +  3.37
cusr 11.14 csys = 15.24 CPU)
Result: FAIL
Failed 1/54 test programs. 4/615 subtests failed.

[1] https://koji.fedoraproject.org/koji/taskinfo?taskID=31037298
[2] https://github.com/SELinuxProject/selinux-testsuite/pull/43

>
> 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 <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
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH] selinux: always allow mounting submounts

[Paul Moore]

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.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: always allow mounting submounts

[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

Re: [PATCH] selinux: always allow mounting submounts

[Ondrej Mosnacek]

Hi Eric,

On Wed, Nov 28, 2018 at 4:42 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> 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?

No, you're right that this patch doesn't "fix" that commit in the
usual sense (the bug has pretty much always been there). However, that
commit is the one that introduces the SB_KERNMOUNT flag and thus this
patch can be only applied on trees that have that commit. That's what
I tried to communicate with the "Fixes:" tag. Maybe I abused it a
little, but it is often used to guide backporting so I figured it
would make sense like this.

>
> 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
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH] selinux: always allow mounting submounts

[Eric W. Biederman]

Ondrej Mosnacek <omosnace@redhat.com> writes:

> On Wed, Nov 28, 2018 at 4:42 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> 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?
>
> No, you're right that this patch doesn't "fix" that commit in the
> usual sense (the bug has pretty much always been there). However, that
> commit is the one that introduces the SB_KERNMOUNT flag and thus this
> patch can be only applied on trees that have that commit. That's what
> I tried to communicate with the "Fixes:" tag. Maybe I abused it a
> little, but it is often used to guide backporting so I figured it
> would make sense like this.

That makes sense.  In cases like that I use Ref: instead of Fixes:
That makes the connection clear, without implying the other patch was
wrong.

That and I would say something like.  It is now possible to fix this
as submounts are not detectable.  Or something like that.

Eric