* [RFC PATCH] selinux: implement move_mount hook
@ 2020-01-13 16:18 Stephen Smalley
2020-01-14 14:33 ` Stephen Smalley
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Smalley @ 2020-01-13 16:18 UTC (permalink / raw)
To: paul
Cc: selinux, omosnace, dhowells, linux-security-module, jmorris,
richard_c_haines, 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. 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(+)
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),
--
2.24.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] selinux: implement move_mount hook
2020-01-13 16:18 [RFC PATCH] selinux: implement move_mount hook Stephen Smalley
@ 2020-01-14 14:33 ` Stephen Smalley
2020-01-14 23:05 ` Casey Schaufler
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Smalley @ 2020-01-14 14:33 UTC (permalink / raw)
To: paul
Cc: selinux, omosnace, dhowells, linux-security-module, jmorris,
richard_c_haines
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),
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] selinux: implement move_mount hook
2020-01-14 14:33 ` Stephen Smalley
@ 2020-01-14 23:05 ` Casey Schaufler
0 siblings, 0 replies; 3+ messages in thread
From: Casey Schaufler @ 2020-01-14 23:05 UTC (permalink / raw)
To: Stephen Smalley, paul
Cc: selinux, omosnace, dhowells, linux-security-module, jmorris,
richard_c_haines, Casey Schaufler
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),
>>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-14 23:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13 16:18 [RFC PATCH] selinux: implement move_mount hook Stephen Smalley
2020-01-14 14:33 ` Stephen Smalley
2020-01-14 23:05 ` Casey Schaufler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.