2017-10-21 13:43:02

by Nicolas Belouin

[permalink] [raw]
Subject: [RFC PATCH 1/2] security, capabilities: Add CAP_SYS_MOUNT

With CAP_SYS_ADMIN being bloated and inapropriate for actions such
as mounting/unmounting filesystems, the creation of a new capability
is needed.
CAP_SYS_MOUNT is meant to give a process the ability to call for mount,
umount and umount2 syscalls.

Signed-off-by: Nicolas Belouin <[email protected]>
---
include/uapi/linux/capability.h | 5 ++++-
security/selinux/include/classmap.h | 4 ++--
2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 230e05d35191..ce230aa6d928 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -365,8 +365,11 @@ struct vfs_ns_cap_data {

#define CAP_AUDIT_READ 37

+/* Allow mounting, unmounting filesystems */

-#define CAP_LAST_CAP CAP_AUDIT_READ
+#define CAP_SYS_MOUNT 38
+
+#define CAP_LAST_CAP CAP_SYS_MOUNT

#define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)

diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 35ffb29a69cb..a873dce97fd5 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -24,9 +24,9 @@
"audit_control", "setfcap"

#define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
- "wake_alarm", "block_suspend", "audit_read"
+ "wake_alarm", "block_suspend", "audit_read", "sys_mount"

-#if CAP_LAST_CAP > CAP_AUDIT_READ
+#if CAP_LAST_CAP > CAP_SYS_MOUNT
#error New capability defined, please update COMMON_CAP2_PERMS.
#endif

--
2.14.2


2017-10-21 13:43:03

by Nicolas Belouin

[permalink] [raw]
Subject: [RFC PATCH 2/2] fs: add the possibility to use CAP_SYS_MOUNT to (u)mount a fs

Fulfill the purpose of CAP_SYS_MOUNT by adding it as a sufficient
capability to mount and unmount filesystems.

Signed-off-by: Nicolas Belouin <[email protected]>
---
fs/cachefiles/daemon.c | 2 +-
fs/ext4/ioctl.c | 2 +-
fs/namespace.c | 3 ++-
fs/super.c | 14 +++++++++-----
4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c
index 1ee54ffd3a24..fc53bdeacc8a 100644
--- a/fs/cachefiles/daemon.c
+++ b/fs/cachefiles/daemon.c
@@ -91,7 +91,7 @@ static int cachefiles_daemon_open(struct inode *inode, struct file *file)
_enter("");

/* only the superuser may do this */
- if (!capable(CAP_SYS_ADMIN))
+ if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_MOUNT))
return -EPERM;

/* the cachefiles device may only be open once at a time */
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index afb66d4ab5cf..19d838e558e2 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -458,7 +458,7 @@ static int ext4_shutdown(struct super_block *sb, unsigned long arg)
struct ext4_sb_info *sbi = EXT4_SB(sb);
__u32 flags;

- if (!capable(CAP_SYS_ADMIN))
+ if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_MOUNT))
return -EPERM;

if (get_user(flags, (__u32 __user *)arg))
diff --git a/fs/namespace.c b/fs/namespace.c
index 3b601f115b6c..1eaa6a9f1631 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1661,7 +1661,8 @@ void __detach_mounts(struct dentry *dentry)
*/
static inline bool may_mount(void)
{
- return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
+ return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN) ||
+ ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_MOUNT);
}

static inline bool may_mandlock(void)
diff --git a/fs/super.c b/fs/super.c
index 166c4ee0d0ed..1d84d8b87216 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -474,7 +474,7 @@ struct super_block *sget_userns(struct file_system_type *type,

if (!(flags & (SB_KERNMOUNT|SB_SUBMOUNT)) &&
!(type->fs_flags & FS_USERNS_MOUNT) &&
- !capable(CAP_SYS_ADMIN))
+ !capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_MOUNT))
return ERR_PTR(-EPERM);
retry:
spin_lock(&sb_lock);
@@ -551,7 +551,9 @@ struct super_block *sget(struct file_system_type *type,
user_ns = &init_user_ns;

/* Ensure the requestor has permissions over the target filesystem */
- if (!(flags & (SB_KERNMOUNT|SB_SUBMOUNT)) && !ns_capable(user_ns, CAP_SYS_ADMIN))
+ if (!(flags & (SB_KERNMOUNT | SB_SUBMOUNT)) &&
+ !ns_capable(user_ns, CAP_SYS_ADMIN) &&
+ !ns_capable(user_ns, CAP_SYS_MOUNT))
return ERR_PTR(-EPERM);

return sget_userns(type, test, set, flags, user_ns, data);
@@ -1020,10 +1022,12 @@ struct dentry *mount_ns(struct file_system_type *fs_type,
{
struct super_block *sb;

- /* Don't allow mounting unless the caller has CAP_SYS_ADMIN
- * over the namespace.
+ /* Don't allow mounting unless the caller has CAP_SYS_ADMIN (deprecated)
+ * or CAP_SYS_MOUNT over the namespace.
*/
- if (!(flags & SB_KERNMOUNT) && !ns_capable(user_ns, CAP_SYS_ADMIN))
+ if (!(flags & SB_KERNMOUNT) &&
+ !ns_capable(user_ns, CAP_SYS_ADMIN) &&
+ !ns_capable(user_ns, CAP_SYS_MOUNT))
return ERR_PTR(-EPERM);

sb = sget_userns(fs_type, ns_test_super, ns_set_super, flags,
--
2.14.2

2017-10-21 17:31:24

by Casey Schaufler

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] security, capabilities: Add CAP_SYS_MOUNT

On 10/21/2017 6:43 AM, Nicolas Belouin wrote:
> With CAP_SYS_ADMIN being bloated and inapropriate for actions such
> as mounting/unmounting filesystems, the creation of a new capability
> is needed.
> CAP_SYS_MOUNT is meant to give a process the ability to call for mount,
> umount and umount2 syscalls.

This is increased granularity for it's own sake. There is no
compelling reason to break out this capability in particular.
Can you identify existing use cases where you would have
CAP_SYS_MOUNT without also having CAP_SYS_ADMIN? I should think
that all the work that's gone into unprivileged mounts over
the past couple years would make this unnecessary.

> Signed-off-by: Nicolas Belouin <[email protected]>
> ---
> include/uapi/linux/capability.h | 5 ++++-
> security/selinux/include/classmap.h | 4 ++--
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 230e05d35191..ce230aa6d928 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -365,8 +365,11 @@ struct vfs_ns_cap_data {
>
> #define CAP_AUDIT_READ 37
>
> +/* Allow mounting, unmounting filesystems */
>
> -#define CAP_LAST_CAP CAP_AUDIT_READ
> +#define CAP_SYS_MOUNT 38
> +
> +#define CAP_LAST_CAP CAP_SYS_MOUNT
>
> #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 35ffb29a69cb..a873dce97fd5 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -24,9 +24,9 @@
> "audit_control", "setfcap"
>
> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
> - "wake_alarm", "block_suspend", "audit_read"
> + "wake_alarm", "block_suspend", "audit_read", "sys_mount"
>
> -#if CAP_LAST_CAP > CAP_AUDIT_READ
> +#if CAP_LAST_CAP > CAP_SYS_MOUNT
> #error New capability defined, please update COMMON_CAP2_PERMS.
> #endif
>

2017-10-21 18:41:58

by Nicolas Belouin

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] security, capabilities: Add CAP_SYS_MOUNT



On October 21, 2017 7:31:24 PM GMT+02:00, Casey Schaufler <[email protected]> wrote:
>On 10/21/2017 6:43 AM, Nicolas Belouin wrote:
>> With CAP_SYS_ADMIN being bloated and inapropriate for actions such
>> as mounting/unmounting filesystems, the creation of a new capability
>> is needed.
>> CAP_SYS_MOUNT is meant to give a process the ability to call for
>mount,
>> umount and umount2 syscalls.
>
>This is increased granularity for it's own sake. There is no
>compelling reason to break out this capability in particular.

Obviously there is a need to break CAP_SYS_ADMIN in pieces, to do so, you have to start somewhere, so I chose to begin with this.

>Can you identify existing use cases where you would have
>CAP_SYS_MOUNT without also having CAP_SYS_ADMIN? I should think
>that all the work that's gone into unprivileged mounts over
>the past couple years would make this unnecessary.

If you look at the udiskd deamon used by most desktop environments, it is launched as root or at least with CAP_SYS_ADMIN. Here, you could use CAP_SYS_MOUNT. There might also be a use within containers as you don't want to give CAP_SYS_ADMIN to a container if it just need to mount/unmount filesystems. If you go even further, it could be used to allow swapon/swapoff (maybe in future patch set).

>
>> Signed-off-by: Nicolas Belouin <[email protected]>
>> ---
>> include/uapi/linux/capability.h | 5 ++++-
>> security/selinux/include/classmap.h | 4 ++--
>> 2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/uapi/linux/capability.h
>b/include/uapi/linux/capability.h
>> index 230e05d35191..ce230aa6d928 100644
>> --- a/include/uapi/linux/capability.h
>> +++ b/include/uapi/linux/capability.h
>> @@ -365,8 +365,11 @@ struct vfs_ns_cap_data {
>>
>> #define CAP_AUDIT_READ 37
>>
>> +/* Allow mounting, unmounting filesystems */
>>
>> -#define CAP_LAST_CAP CAP_AUDIT_READ
>> +#define CAP_SYS_MOUNT 38
>> +
>> +#define CAP_LAST_CAP CAP_SYS_MOUNT
>>
>> #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>>
>> diff --git a/security/selinux/include/classmap.h
>b/security/selinux/include/classmap.h
>> index 35ffb29a69cb..a873dce97fd5 100644
>> --- a/security/selinux/include/classmap.h
>> +++ b/security/selinux/include/classmap.h
>> @@ -24,9 +24,9 @@
>> "audit_control", "setfcap"
>>
>> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
>> - "wake_alarm", "block_suspend", "audit_read"
>> + "wake_alarm", "block_suspend", "audit_read", "sys_mount"
>>
>> -#if CAP_LAST_CAP > CAP_AUDIT_READ
>> +#if CAP_LAST_CAP > CAP_SYS_MOUNT
>> #error New capability defined, please update COMMON_CAP2_PERMS.
>> #endif
>>

Nicolas

2017-10-22 00:54:11

by Casey Schaufler

[permalink] [raw]
Subject: Re: [kernel-hardening] [RFC PATCH 1/2] security, capabilities: Add CAP_SYS_MOUNT

On 10/21/2017 11:41 AM, Nicolas Belouin wrote:
>
> On October 21, 2017 7:31:24 PM GMT+02:00, Casey Schaufler <[email protected]> wrote:
>> On 10/21/2017 6:43 AM, Nicolas Belouin wrote:
>>> With CAP_SYS_ADMIN being bloated and inapropriate for actions such
>>> as mounting/unmounting filesystems, the creation of a new capability
>>> is needed.
>>> CAP_SYS_MOUNT is meant to give a process the ability to call for
>> mount,
>>> umount and umount2 syscalls.
>> This is increased granularity for it's own sake. There is no
>> compelling reason to break out this capability in particular.
> Obviously there is a need to break CAP_SYS_ADMIN in pieces,

No. This is a baseless assumption. Granularity for the sake of
granularity is bad. Data General (a dead company) followed the
fine grained capability path and ended up with 330 capabilities.
Developers can't handle the granularity we already have (hell,
half of them don't know what mode bits are for) and making it
finer will only make it harder for them to make use of the ones
we have.

> to do so, you have to start somewhere, so I chose to begin with this.
>
>> Can you identify existing use cases where you would have
>> CAP_SYS_MOUNT without also having CAP_SYS_ADMIN? I should think
>> that all the work that's gone into unprivileged mounts over
>> the past couple years would make this unnecessary.
> If you look at the udiskd deamon used by most desktop environments, it is launched as root or at least with CAP_SYS_ADMIN. Here, you could use CAP_SYS_MOUNT.

Does this demon do anything else that uses CAP_SYS_ADMIN?
If it has to have that anyway, it's not an argument for breaking
out the mount capability.
 

> There might also be a use within containers as you don't want to give CAP_SYS_ADMIN to a container if it just need to mount/unmount filesystems.

There is massive work going on elsewhere to allow containers to
mount without privilege. And I'm not at all interested in "might".

> If you go even further, it could be used to allow swapon/swapoff (maybe in future patch set).

No, you'd be asking for CAP_SWAP for that. Swap control has nothing to do
with mounting filesystems.

>
>>> Signed-off-by: Nicolas Belouin <[email protected]>
>>> ---
>>> include/uapi/linux/capability.h | 5 ++++-
>>> security/selinux/include/classmap.h | 4 ++--
>>> 2 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/capability.h
>> b/include/uapi/linux/capability.h
>>> index 230e05d35191..ce230aa6d928 100644
>>> --- a/include/uapi/linux/capability.h
>>> +++ b/include/uapi/linux/capability.h
>>> @@ -365,8 +365,11 @@ struct vfs_ns_cap_data {
>>>
>>> #define CAP_AUDIT_READ 37
>>>
>>> +/* Allow mounting, unmounting filesystems */
>>>
>>> -#define CAP_LAST_CAP CAP_AUDIT_READ
>>> +#define CAP_SYS_MOUNT 38
>>> +
>>> +#define CAP_LAST_CAP CAP_SYS_MOUNT
>>>
>>> #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>>>
>>> diff --git a/security/selinux/include/classmap.h
>> b/security/selinux/include/classmap.h
>>> index 35ffb29a69cb..a873dce97fd5 100644
>>> --- a/security/selinux/include/classmap.h
>>> +++ b/security/selinux/include/classmap.h
>>> @@ -24,9 +24,9 @@
>>> "audit_control", "setfcap"
>>>
>>> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
>>> - "wake_alarm", "block_suspend", "audit_read"
>>> + "wake_alarm", "block_suspend", "audit_read", "sys_mount"
>>>
>>> -#if CAP_LAST_CAP > CAP_AUDIT_READ
>>> +#if CAP_LAST_CAP > CAP_SYS_MOUNT
>>> #error New capability defined, please update COMMON_CAP2_PERMS.
>>> #endif
>>>
> Nicolas
>

2017-10-23 12:57:39

by Stephen Smalley

[permalink] [raw]
Subject: Re: [RFC PATCH 1/2] security, capabilities: Add CAP_SYS_MOUNT

On Sat, 2017-10-21 at 15:43 +0200, Nicolas Belouin wrote:
> With CAP_SYS_ADMIN being bloated and inapropriate for actions such
> as mounting/unmounting filesystems, the creation of a new capability
> is needed.
> CAP_SYS_MOUNT is meant to give a process the ability to call for
> mount,
> umount and umount2 syscalls.

If adding a new capability isn't deemed acceptable, then another option
would be to introduce LSM hooks where there isn't already coverage and
implement finer-grained permission checks there. In some cases, that
already occurs for mount and umount*. That also offers the possibility
of taking the object of the operation into account, unlike capabilities
which are only subject/process-based.


>
> Signed-off-by: Nicolas Belouin <[email protected]>
> ---
>  include/uapi/linux/capability.h     | 5 ++++-
>  security/selinux/include/classmap.h | 4 ++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/include/uapi/linux/capability.h
> b/include/uapi/linux/capability.h
> index 230e05d35191..ce230aa6d928 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -365,8 +365,11 @@ struct vfs_ns_cap_data {
>  
>  #define CAP_AUDIT_READ 37
>  
> +/* Allow mounting, unmounting filesystems */
>  
> -#define CAP_LAST_CAP         CAP_AUDIT_READ
> +#define CAP_SYS_MOUNT 38
> +
> +#define CAP_LAST_CAP         CAP_SYS_MOUNT
>  
>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>  
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index 35ffb29a69cb..a873dce97fd5 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -24,9 +24,9 @@
>       "audit_control", "setfcap"
>  
>  #define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog", \
> - "wake_alarm", "block_suspend", "audit_read"
> + "wake_alarm", "block_suspend", "audit_read",
> "sys_mount"
>  
> -#if CAP_LAST_CAP > CAP_AUDIT_READ
> +#if CAP_LAST_CAP > CAP_SYS_MOUNT
>  #error New capability defined, please update COMMON_CAP2_PERMS.
>  #endif
>