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

Christian Brauner
2018-05-24T15:57:44Z

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
>