[REVIEW,5/6] capabilities: Allow privileged user in s_user_ns to set security.* xattrs

Submitted by Eric W. Biederman on May 23, 2018, 11:25 p.m.

Details

Message ID 20180523232538.4880-5-ebiederm@xmission.com
State New
Series "Wrapping up the vfs support for unprivileged mounts"
Headers show

Commit Message

Eric W. Biederman May 23, 2018, 11:25 p.m.
A privileged user in s_user_ns will generally have the ability to
manipulate the backing store and insert security.* xattrs into
the filesystem directly. Therefore the kernel must be prepared to
handle these xattrs from unprivileged mounts, and it makes little
sense for commoncap to prevent writing these xattrs to the
filesystem. The capability and LSM code have already been updated
to appropriately handle xattrs from unprivileged mounts, so it
is safe to loosen this restriction on setting xattrs.

The exception to this logic is that writing xattrs to a mounted
filesystem may also cause the LSM inode_post_setxattr or
inode_setsecurity callbacks to be invoked. SELinux will deny the
xattr update by virtue of applying mountpoint labeling to
unprivileged userns mounts, and Smack will deny the writes for
any user without global CAP_MAC_ADMIN, so loosening the
capability check in commoncap is safe in this respect as well.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 security/commoncap.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/security/commoncap.c b/security/commoncap.c
index 1ce701fcb3f3..f4c33abd9959 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -919,6 +919,8 @@  int cap_bprm_set_creds(struct linux_binprm *bprm)
 int cap_inode_setxattr(struct dentry *dentry, const char *name,
 		       const void *value, size_t size, int flags)
 {
+	struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
+
 	/* Ignore non-security xattrs */
 	if (strncmp(name, XATTR_SECURITY_PREFIX,
 			sizeof(XATTR_SECURITY_PREFIX) - 1) != 0)
@@ -931,7 +933,7 @@  int cap_inode_setxattr(struct dentry *dentry, const char *name,
 	if (strcmp(name, XATTR_NAME_CAPS) == 0)
 		return 0;
 
-	if (!capable(CAP_SYS_ADMIN))
+	if (!ns_capable(user_ns, CAP_SYS_ADMIN))
 		return -EPERM;
 	return 0;
 }
@@ -949,6 +951,8 @@  int cap_inode_setxattr(struct dentry *dentry, const char *name,
  */
 int cap_inode_removexattr(struct dentry *dentry, const char *name)
 {
+	struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
+
 	/* Ignore non-security xattrs */
 	if (strncmp(name, XATTR_SECURITY_PREFIX,
 			sizeof(XATTR_SECURITY_PREFIX) - 1) != 0)
@@ -964,7 +968,7 @@  int cap_inode_removexattr(struct dentry *dentry, const char *name)
 		return 0;
 	}
 
-	if (!capable(CAP_SYS_ADMIN))
+	if (!ns_capable(user_ns, CAP_SYS_ADMIN))
 		return -EPERM;
 	return 0;
 }

Comments

Christian Brauner May 24, 2018, 3:57 p.m.
On Wed, May 23, 2018 at 06:25:37PM -0500, Eric W. Biederman wrote:
> A privileged user in s_user_ns will generally have the ability to
> manipulate the backing store and insert security.* xattrs into
> the filesystem directly. Therefore the kernel must be prepared to
> handle these xattrs from unprivileged mounts, and it makes little
> sense for commoncap to prevent writing these xattrs to the
> filesystem. The capability and LSM code have already been updated
> to appropriately handle xattrs from unprivileged mounts, so it
> is safe to loosen this restriction on setting xattrs.
> 
> The exception to this logic is that writing xattrs to a mounted
> filesystem may also cause the LSM inode_post_setxattr or
> inode_setsecurity callbacks to be invoked. SELinux will deny the
> xattr update by virtue of applying mountpoint labeling to
> unprivileged userns mounts, and Smack will deny the writes for
> any user without global CAP_MAC_ADMIN, so loosening the
> capability check in commoncap is safe in this respect as well.

Acked-by: Christian Brauner <christian@brauner.io>

> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

Note, I just talked to Serge. This should be Acked-by: Serge Hallyn <serge@hallyn.com>

> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
>  security/commoncap.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 1ce701fcb3f3..f4c33abd9959 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -919,6 +919,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  int cap_inode_setxattr(struct dentry *dentry, const char *name,
>  		       const void *value, size_t size, int flags)
>  {
> +	struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
> +
>  	/* Ignore non-security xattrs */
>  	if (strncmp(name, XATTR_SECURITY_PREFIX,
>  			sizeof(XATTR_SECURITY_PREFIX) - 1) != 0)
> @@ -931,7 +933,7 @@ int cap_inode_setxattr(struct dentry *dentry, const char *name,
>  	if (strcmp(name, XATTR_NAME_CAPS) == 0)
>  		return 0;
>  
> -	if (!capable(CAP_SYS_ADMIN))
> +	if (!ns_capable(user_ns, CAP_SYS_ADMIN))
>  		return -EPERM;
>  	return 0;
>  }
> @@ -949,6 +951,8 @@ int cap_inode_setxattr(struct dentry *dentry, const char *name,
>   */
>  int cap_inode_removexattr(struct dentry *dentry, const char *name)
>  {
> +	struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
> +
>  	/* Ignore non-security xattrs */
>  	if (strncmp(name, XATTR_SECURITY_PREFIX,
>  			sizeof(XATTR_SECURITY_PREFIX) - 1) != 0)
> @@ -964,7 +968,7 @@ int cap_inode_removexattr(struct dentry *dentry, const char *name)
>  		return 0;
>  	}
>  
> -	if (!capable(CAP_SYS_ADMIN))
> +	if (!ns_capable(user_ns, CAP_SYS_ADMIN))
>  		return -EPERM;
>  	return 0;
>  }
> -- 
> 2.14.1
>