| Message ID | 20180523232538.4880-5-ebiederm@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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; }