| Message ID | 20200113161800.63865-1-sds@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] selinux: implement move_mount hook | expand |
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@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), > >
On 1/14/2020 6:33 AM, Stephen Smalley wrote: > 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@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). That's correct. Smack provides controls on subjects and objects. It leaves the mundania of privilege to the capabilities mechanism. > >> --- >> 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), >> > >
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),
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@tycho.nsa.gov> --- security/selinux/hooks.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)