[PATCH v2] selinux: fix regression introduced by move_mount(2) syscall

[PATCH v2] selinux: fix regression introduced by move_mount(2) syscall

[Stephen Smalley]

commit 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
introduced a new move_mount(2) system call and a corresponding new LSM
security_move_mount hook but did not implement this hook for any existing
LSM.  This creates a regression for SELinux with respect to consistent
checking of mounts; the existing selinux_mount hook checks mounton
permission to the mount point path.  Provide a SELinux hook
implementation for move_mount that applies this same check for
consistency.  In the future we may wish to add a new move_mount
filesystem permission and check as well, but this addresses
the immediate regression.

Fixes: 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
v2 drops the RFC prefix, changes the subject to make it more evident that
this is a regression fix, and drops the TBD comment from the hook.

 security/selinux/hooks.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f9224866d60a..b35b5c6ad8be 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2724,6 +2724,14 @@ static int selinux_mount(const char *dev_name,
 		return path_has_perm(cred, path, FILE__MOUNTON);
 }
 
+static int selinux_move_mount(const struct path *from_path,
+			      const struct path *to_path)
+{
+	const struct cred *cred = current_cred();
+
+	return path_has_perm(cred, to_path, FILE__MOUNTON);
+}
+
 static int selinux_umount(struct vfsmount *mnt, int flags)
 {
 	const struct cred *cred = current_cred();
@@ -6913,6 +6921,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(sb_set_mnt_opts, selinux_set_mnt_opts),
 	LSM_HOOK_INIT(sb_clone_mnt_opts, selinux_sb_clone_mnt_opts),
 
+	LSM_HOOK_INIT(move_mount, selinux_move_mount),
+
 	LSM_HOOK_INIT(dentry_init_security, selinux_dentry_init_security),
 	LSM_HOOK_INIT(dentry_create_files_as, selinux_dentry_create_files_as),
 
-- 
2.24.1

Re: [PATCH v2] selinux: fix regression introduced by move_mount(2) syscall

[Ondrej Mosnacek]

On Fri, Jan 17, 2020 at 9:23 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> commit 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
> introduced a new move_mount(2) system call and a corresponding new LSM
> security_move_mount hook but did not implement this hook for any existing
> LSM.  This creates a regression for SELinux with respect to consistent
> checking of mounts; the existing selinux_mount hook checks mounton
> permission to the mount point path.  Provide a SELinux hook
> implementation for move_mount that applies this same check for
> consistency.  In the future we may wish to add a new move_mount
> filesystem permission and check as well, but this addresses
> the immediate regression.
>
> Fixes: 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>

Looks reasonable.

Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>

> ---
> v2 drops the RFC prefix, changes the subject to make it more evident that
> this is a regression fix, and drops the TBD comment from the hook.
>
>  security/selinux/hooks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f9224866d60a..b35b5c6ad8be 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2724,6 +2724,14 @@ static int selinux_mount(const char *dev_name,
>                 return path_has_perm(cred, path, FILE__MOUNTON);
>  }
>
> +static int selinux_move_mount(const struct path *from_path,
> +                             const struct path *to_path)
> +{
> +       const struct cred *cred = current_cred();
> +
> +       return path_has_perm(cred, to_path, FILE__MOUNTON);
> +}
> +
>  static int selinux_umount(struct vfsmount *mnt, int flags)
>  {
>         const struct cred *cred = current_cred();
> @@ -6913,6 +6921,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(sb_set_mnt_opts, selinux_set_mnt_opts),
>         LSM_HOOK_INIT(sb_clone_mnt_opts, selinux_sb_clone_mnt_opts),
>
> +       LSM_HOOK_INIT(move_mount, selinux_move_mount),
> +
>         LSM_HOOK_INIT(dentry_init_security, selinux_dentry_init_security),
>         LSM_HOOK_INIT(dentry_create_files_as, selinux_dentry_create_files_as),
>
> --
> 2.24.1
>

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

Re: [PATCH v2] selinux: fix regression introduced by move_mount(2) syscall

[Paul Moore]

On Fri, Jan 17, 2020 at 3:23 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> commit 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
> introduced a new move_mount(2) system call and a corresponding new LSM
> security_move_mount hook but did not implement this hook for any existing
> LSM.  This creates a regression for SELinux with respect to consistent
> checking of mounts; the existing selinux_mount hook checks mounton
> permission to the mount point path.  Provide a SELinux hook
> implementation for move_mount that applies this same check for
> consistency.  In the future we may wish to add a new move_mount
> filesystem permission and check as well, but this addresses
> the immediate regression.
>
> Fixes: 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> v2 drops the RFC prefix, changes the subject to make it more evident that
> this is a regression fix, and drops the TBD comment from the hook.
>
>  security/selinux/hooks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

This looks good to me too, thanks Stephen.  Because of the nature of
this fix, I'm going to merge this into next now, even though we are at
-rc7.  Since we are effectively treating this as another mount
operation, and reusing the file:mounton permission, I don't believe
there should be any widespread access denials on existing distros ...
I assume you've at least tested this on Fedora and everything looked
okay?

It also looks like the fs tests Richard is working on includes tests
for the move_mount() so I think we are covered as far as the
selinux-testsuite is concerned.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2] selinux: fix regression introduced by move_mount(2) syscall

[Stephen Smalley]

On Mon, Jan 20, 2020 at 7:52 AM Paul Moore <paul@paul-moore.com> wrote:
> This looks good to me too, thanks Stephen.  Because of the nature of
> this fix, I'm going to merge this into next now, even though we are at
> -rc7.  Since we are effectively treating this as another mount
> operation, and reusing the file:mounton permission, I don't believe
> there should be any widespread access denials on existing distros ...
> I assume you've at least tested this on Fedora and everything looked
> okay?

I did basic boot testing plus selinux-testsuite on Fedora without any issues.
I'm not sure that Linux userspace (at least shipped in distros)
besides test/sample programs is using the new system calls yet.
And since anything that performed mounts previously using mount(2)
would have required mounton permission,
I would expect anything converted to use the new system calls would
likewise have that permission already.

> It also looks like the fs tests Richard is working on includes tests
> for the move_mount() so I think we are covered as far as the
> selinux-testsuite is concerned.

Not sure since those tests were just added in the latest version of
his patches and at this point he would
be running on kernels that lack this permission check.

Re: [PATCH v2] selinux: fix regression introduced by move_mount(2) syscall

[Stephen Smalley]

On Mon, Jan 20, 2020 at 10:40 AM Stephen Smalley
<stephen.smalley@gmail.com> wrote:
>
> On Mon, Jan 20, 2020 at 7:52 AM Paul Moore <paul@paul-moore.com> wrote:
> > This looks good to me too, thanks Stephen.  Because of the nature of
> > this fix, I'm going to merge this into next now, even though we are at
> > -rc7.  Since we are effectively treating this as another mount
> > operation, and reusing the file:mounton permission, I don't believe
> > there should be any widespread access denials on existing distros ...
> > I assume you've at least tested this on Fedora and everything looked
> > okay?
>
> I did basic boot testing plus selinux-testsuite on Fedora without any issues.
> I'm not sure that Linux userspace (at least shipped in distros)
> besides test/sample programs is using the new system calls yet.
> And since anything that performed mounts previously using mount(2)
> would have required mounton permission,
> I would expect anything converted to use the new system calls would
> likewise have that permission already.
>
> > It also looks like the fs tests Richard is working on includes tests
> > for the move_mount() so I think we are covered as far as the
> > selinux-testsuite is concerned.
>
> Not sure since those tests were just added in the latest version of
> his patches and at this point he would
> be running on kernels that lack this permission check.

Ah, never mind - I see that he mentioned that he applied my move_mount
patch before performing those tests.
That means that there should be a test failure on kernels >= 5.2 (i.e.
that have move_mount(2)) that lack this
patch IIUC.

Re: [PATCH v2] selinux: fix regression introduced by move_mount(2) syscall

[Stephen Smalley]

On Mon, Jan 20, 2020 at 10:43 AM Stephen Smalley
<stephen.smalley@gmail.com> wrote:
>
> On Mon, Jan 20, 2020 at 10:40 AM Stephen Smalley
> <stephen.smalley@gmail.com> wrote:
> >
> > On Mon, Jan 20, 2020 at 7:52 AM Paul Moore <paul@paul-moore.com> wrote:
> > > It also looks like the fs tests Richard is working on includes tests
> > > for the move_mount() so I think we are covered as far as the
> > > selinux-testsuite is concerned.
> >
> > Not sure since those tests were just added in the latest version of
> > his patches and at this point he would
> > be running on kernels that lack this permission check.
>
> Ah, never mind - I see that he mentioned that he applied my move_mount
> patch before performing those tests.
> That means that there should be a test failure on kernels >= 5.2 (i.e.
> that have move_mount(2)) that lack this
> patch IIUC.

Never mind again; he has the move_mount test under a conditional that
will get updated to be specific to a kernel version if/when my patch
lands.

Re: [PATCH v2] selinux: fix regression introduced by move_mount(2) syscall

[Paul Moore]

On Mon, Jan 20, 2020 at 10:40 AM Stephen Smalley
<stephen.smalley@gmail.com> wrote:
> On Mon, Jan 20, 2020 at 7:52 AM Paul Moore <paul@paul-moore.com> wrote:
> > This looks good to me too, thanks Stephen.  Because of the nature of
> > this fix, I'm going to merge this into next now, even though we are at
> > -rc7.  Since we are effectively treating this as another mount
> > operation, and reusing the file:mounton permission, I don't believe
> > there should be any widespread access denials on existing distros ...
> > I assume you've at least tested this on Fedora and everything looked
> > okay?
>
> I did basic boot testing plus selinux-testsuite on Fedora without any issues.
> I'm not sure that Linux userspace (at least shipped in distros)
> besides test/sample programs is using the new system calls yet.
> And since anything that performed mounts previously using mount(2)
> would have required mounton permission,
> I would expect anything converted to use the new system calls would
> likewise have that permission already.

It is the last sentence that I was getting at in my reply.  It seems
reasonable to equate moving a mount to mounting a filesystem (which
this patch does), and thus any domain which wants, and should have the
permission, to move a mounted filesystem is likely to already have the
file:mounton permission.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2] selinux: fix regression introduced by move_mount(2) syscall

[David Howells]

Stephen Smalley <sds@tycho.nsa.gov> wrote:

> commit 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
> introduced a new move_mount(2) system call and a corresponding new LSM
> security_move_mount hook but did not implement this hook for any existing
> LSM.  This creates a regression for SELinux with respect to consistent
> checking of mounts; the existing selinux_mount hook checks mounton
> permission to the mount point path.  Provide a SELinux hook
> implementation for move_mount that applies this same check for
> consistency.  In the future we may wish to add a new move_mount
> filesystem permission and check as well, but this addresses
> the immediate regression.
> 
> Fixes: 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>

Reviewed-by: David Howells <dhowells@redhat.com>