| Message ID | 20200117202407.12344-1-sds@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] selinux: fix regression introduced by move_mount(2) syscall | expand |
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 >
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.
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>
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.
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.
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.
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.
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),
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(+)