[RFC PATCH] selinux: implement move_mount hook

[Stephen Smalley]

On 1/13/20 11:18 AM, Stephen Smalley 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.  We may wish to consider defining a new filesystem
> move_mount permission and/or a new dir(ectory) move_mount permission
> and checking it in this hook in the future.
> 
> Fixes: 2db154b3ea8e ("vfs: syscall: Add move_mount(2) to move mounts around")
> Signed-off-by: Stephen Smalley <sds at tycho.nsa.gov>

NB I cc'd lsm list on this patch just as a heads-up/reminder that this 
hook hasn't been implemented for any security modules AFAICT, not just 
SELinux.  I see that there was some discussion of this in the past with 
a trivial patch proposed by Tetsuo to just disable the syscall when 
TOMOYO or AppArmor is enabled, but no action seems to have been taken,
https://lore.kernel.org/linux-security-module/5802b8b1-f734-1670-f83b-465eda133936@i-love.sakura.ne.jp/
https://lore.kernel.org/linux-security-module/1565365478-6550-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp/

The move_mount syscall does check may_mount() and hence requires 
CAP_SYS_ADMIN for the user namespace associated with the mount 
namespace, so both SELinux and AppArmor would at least restrict the use 
of this syscall to processes allowed CAP_SYS_ADMIN by policy, but TOMOYO 
doesn't implement the capable hook either so move_mount is entirely 
unrestricted by it at present.  Looks like Smack doesn't implement any 
mount checking so it doesn't care about move_mount (especially since it 
requires CAP_SYS_ADMIN already).

> ---
>   security/selinux/hooks.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0606e107fca3..244874b103ff 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2766,6 +2766,19 @@ 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();
> +
> +	/*
> +	 *  TBD: Check new FILESYSTEM__MOVE_MOUNT permission to
> +	 *  from_path->dentry->s_sb and/or new DIR__MOVE_MOUNT
> +	 *  permission to from_path?
> +	 */
> +	return path_has_perm(cred, to_path, FILE__MOUNTON);
> +}
> +
>   static int selinux_umount(struct vfsmount *mnt, int flags)
>   {
>   	const struct cred *cred = current_cred();
> @@ -6943,6 +6956,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),
>   
>