Re: [PATCH 06/11] capabilities: Allow privileged user in s_user_ns to set security.* xattrs

[Serge E. Hallyn]

On Fri, Dec 22, 2017 at 03:32:30PM +0100, Dongsu Park wrote:
> From: Seth Forshee <seth.forshee@xxxxxxxxxxxxx>
> 
> 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.
> 
> Patch v4 is available: https://patchwork.kernel.org/patch/8944641/
> 
> Cc: linux-security-module@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: James Morris <james.l.morris@xxxxxxxxxx>
> Cc: Serge Hallyn <serge@xxxxxxxxxx>

Reviewed-by: Serge Hallyn <serge@xxxxxxxxxx>

> Signed-off-by: Seth Forshee <seth.forshee@xxxxxxxxxxxxx>
> Signed-off-by: Dongsu Park <dongsu@xxxxxxxxxx>
> ---
>  security/commoncap.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 4f8e0934..dd0afef9 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -920,6 +920,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)
> @@ -932,7 +934,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;
>  }
> @@ -950,6 +952,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)
> @@ -965,7 +969,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.13.6