Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1401775 > unrolled thread

Re: [PATCH 1/1] simplified security.nscapability xattr

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/1] simplified security.nscapability xattr "Serge E. Hallyn" <serge@hallyn.com> - 2016-05-16 23:20 +0200
    Re: [PATCH 1/1] simplified security.nscapability xattr "Serge E. Hallyn" <serge@hallyn.com> - 2016-05-16 23:50 +0200
      [PATCH RFC] user-namespaced file capabilities - now with more magic "Serge E. Hallyn" <serge@hallyn.com> - 2016-05-19 00:00 +0200
        Re: [PATCH RFC] user-namespaced file capabilities - now with more  magic Mimi Zohar <zohar@linux.vnet.ibm.com> - 2016-05-19 23:00 +0200
          Re: [PATCH RFC] user-namespaced file capabilities - now with more  magic "Serge E. Hallyn" <serge@hallyn.com> - 2016-05-20 05:50 +0200
            Re: [PATCH RFC] user-namespaced file capabilities - now with more  magic Mimi Zohar <zohar@linux.vnet.ibm.com> - 2016-05-20 13:20 +0200
              Re: [PATCH RFC] user-namespaced file capabilities - now with more magic ebiederm@xmission.com (Eric W. Biederman) - 2016-05-20 20:50 +0200
                Re: [PATCH RFC] user-namespaced file capabilities - now with more  magic Mimi Zohar <zohar@linux.vnet.ibm.com> - 2016-05-20 21:20 +0200
                  Re: [PATCH RFC] user-namespaced file capabilities - now with more magic ebiederm@xmission.com (Eric W. Biederman) - 2016-05-20 21:30 +0200
                Re: [PATCH RFC] user-namespaced file capabilities - now with more  magic "Serge E. Hallyn" <serge@hallyn.com> - 2016-05-20 21:30 +0200
                  Re: [PATCH RFC] user-namespaced file capabilities - now with more  magic "Serge E. Hallyn" <serge@hallyn.com> - 2016-05-20 22:00 +0200
                    Re: [PATCH RFC] user-namespaced file capabilities - now with more  magic Mimi Zohar <zohar@linux.vnet.ibm.com> - 2016-05-21 01:30 +0200
                      Re: [PATCH RFC] user-namespaced file capabilities - now with more  magic "Serge E. Hallyn" <serge@hallyn.com> - 2016-05-21 01:40 +0200
                  Re: [PATCH RFC] user-namespaced file capabilities - now with more magic ebiederm@xmission.com (Eric W. Biederman) - 2016-05-20 22:00 +0200

#1401775 — Re: [PATCH 1/1] simplified security.nscapability xattr

Quoting Serge E. Hallyn (serge@hallyn.com):
...
> There's a problem though.  The above suffices to prevent an unprivileged user
> in a user_ns from unsharing a user_ns to write a file capability and exploit
> that capability in the ns where he is unprivileged.  With one exception, which
> is the case where the unprivileged user is mapped to the same kuid which
> created the namespace.  So if uid 1000 on the host creates a namespace
> where uid 1000 maps to 1000 in the namespace, then 1000 in the namespace
> can create a new user_ns, write the xattr, and exploit it from the
> parent namespace.  This is not an uncommon case.  I'm not sure what to do about
> it.

Ok I think I've convinced myself that requiring a kuid 0 in the container
and storing that in the security.nscapability is best solution.  The DAC
objection is imo not really valid - we don't have to give uid 0 in the
container any special privilege, we just require that the ns have a uid 0
mapping.  I have not been able to think of any other reliable way to verify
that the writer of the capability is authorized to grant privilege to the
file when executed by current.

I'm going to proceed with another POC based on the following design:

1. no new syscalls at the moment.  You can choose to set/query
security.nscapability, but can also just set security.capability from
a user_ns and have the kernel transparently set a security.nscapability
entry for you.

2. For now just a single security.nscapability entry, but in a format
that turning it into an array will be a trivial change

3. When running file foo which has a security.nscapability for kuid 100000,
then any namespace where kuid 100000 is root - or which has an ancestor ns where
that is the case - will run the file with the listed capabilities.

4. When doing getxattr of security.capability from a user_ns, if there is a
security.capability entry, that will be returned;  else if there is a valid
security.nscapability for your ns, that will be returned.

5. when doing a setxattr of security.capability from a user_ns, if there is
a security.nscapability entry, you get EBUSY;  else a security.nscapability 
with your root kuid will be written provided that (a) you are privileged
over your namespace, (b) you are privileged over your root uid, (c) the
file owner maps into your namespace.

6. when doing a getxattr of security.nscapability, the entry will be shown
with kuid mapped into your namespace or -1 if the uid does not map into
your ns.

7. when doing a setxattr of security.nscapability, if an entry exists, you
get -EBUSY;  if you are not privileged over your ns, your root uid, and
the file owner, then you get -EPERM;  the xattr includes a uid field, which
must be either 0 or a value valid in your ns.  The value will be converted
to a kuid and stored on disk.  (Seth, I'm not sure offhand how that should
mesh with your patches, we can talk about it after I send the next patch,
which I'm quite certain will handle it wrongly)

8. If a security.capability exists, it will override any security.nscapability
at execve() (so, inverse of my previous two patches).

-serge

[toc] | [next] | [standalone]

#1401782

On Mon, May 16, 2016 at 04:15:23PM -0500, Serge E. Hallyn wrote:
> Quoting Serge E. Hallyn (serge@hallyn.com):
> ...
> > There's a problem though.  The above suffices to prevent an unprivileged user
> > in a user_ns from unsharing a user_ns to write a file capability and exploit
> > that capability in the ns where he is unprivileged.  With one exception, which
> > is the case where the unprivileged user is mapped to the same kuid which
> > created the namespace.  So if uid 1000 on the host creates a namespace
> > where uid 1000 maps to 1000 in the namespace, then 1000 in the namespace
> > can create a new user_ns, write the xattr, and exploit it from the
> > parent namespace.  This is not an uncommon case.  I'm not sure what to do about
> > it.
> 
> Ok I think I've convinced myself that requiring a kuid 0 in the container
> and storing that in the security.nscapability is best solution.  The DAC
> objection is imo not really valid - we don't have to give uid 0 in the
> container any special privilege, we just require that the ns have a uid 0
> mapping.  I have not been able to think of any other reliable way to verify
> that the writer of the capability is authorized to grant privilege to the
> file when executed by current.
> 
> I'm going to proceed with another POC based on the following design:
> 
> 1. no new syscalls at the moment.  You can choose to set/query
> security.nscapability, but can also just set security.capability from
> a user_ns and have the kernel transparently set a security.nscapability
> entry for you.
> 
> 2. For now just a single security.nscapability entry, but in a format
> that turning it into an array will be a trivial change
> 
> 3. When running file foo which has a security.nscapability for kuid 100000,
> then any namespace where kuid 100000 is root - or which has an ancestor ns where
> that is the case - will run the file with the listed capabilities.
> 
> 4. When doing getxattr of security.capability from a user_ns, if there is a
> security.capability entry, that will be returned;  else if there is a valid
> security.nscapability for your ns, that will be returned.
> 
> 5. when doing a setxattr of security.capability from a user_ns, if there is
> a security.nscapability entry, you get EBUSY;  else a security.nscapability 
> with your root kuid will be written provided that (a) you are privileged
> over your namespace, (b) you are privileged over your root uid, (c) the
> file owner maps into your namespace.

Stéphane pointed out this isn't quite right.  The EBUSY will happen if
a security.nscapability is defined with a kuid over which the writer is
not privileged - else it will overwrite.  It will also happen if
security.capbility is set.

> 6. when doing a getxattr of security.nscapability, the entry will be shown
> with kuid mapped into your namespace or -1 if the uid does not map into
> your ns.
> 
> 7. when doing a setxattr of security.nscapability, if an entry exists, you
> get -EBUSY;  if you are not privileged over your ns, your root uid, and
> the file owner, then you get -EPERM;  the xattr includes a uid field, which
> must be either 0 or a value valid in your ns.  The value will be converted
> to a kuid and stored on disk.  (Seth, I'm not sure offhand how that should
> mesh with your patches, we can talk about it after I send the next patch,
> which I'm quite certain will handle it wrongly)
> 
> 8. If a security.capability exists, it will override any security.nscapability
> at execve() (so, inverse of my previous two patches).
> 
> -serge

[toc] | [prev] | [next] | [standalone]

#1403319 — [PATCH RFC] user-namespaced file capabilities - now with more magic

This patch introduces a new security.nscapability xattr.  It
is mostly like security.capability, but also lists a 'rootid'.
This is the uid_t (in init_user_ns) of the root id (uid 0 in a
namespace) in whose namespaces the file capabilities may take
effect.

A privileged (cap_setfcap) process in the initial user ns may
set and read this xattr directly.  However, its real intent is
to be used as a transparent fallback in user namespaces.

Root in a user ns cannot be trusted to write security.capability
xattrs, because any user on the host could map his own uid to root
in a namespace, write the xattr, and execute the file with privilege
on the host.

With this patch, when root in a user ns asks to write security.capability,
the kernel will transparently write a security.nscapability xattr
instead, filling in the kuid of the calling user's root uid.  Subsequently,
any task executing the file which has the noted k_uid as its root uid,
or which is in a descendent user_ns of such a user_ns, will run the
file with capabilities.

When reading the security.capability xattr from a non-init user_ns, a valid
security.nscapability will be shown if it exists.  Such a task is not
allowed to read security.nscapability.  This could be accomodated, however
it requires the kernel to convert the kuid_t to a valid uid in the reader's
user_ns.  So for now it's simply not supported.

Only a single security.nscapability xattr may be written.  This patch
could be expanded to allow a list of capabilities and rootids, however
I do not believe that to be a worthwhile use case.

This allows a simple setxattr to work, allows tar/untar to
work, and allows us to tar in one namespace and untar in
another while preserving the capability, without risking
leaking privilege into a parent namespace.

Note - listxattr is not being handled here.  So results of that can be
inconsistent with get/setxattr.  Fixing that will require yet more
deceit in fs/xattr.c.

Note2 - it may be less sneaky to hide all the magic behind the
security.nscapability xattr.  So userspace would need to know to
use that xattr name when needed, but with the same format as
security.capability.  The kuid_t rootid would be filled in by the
kernel.  That's a middle ground between my last patch and this one.

Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
 fs/xattr.c                      |  18 ++-
 include/linux/capability.h      |   8 +-
 include/uapi/linux/capability.h |  19 +++
 include/uapi/linux/xattr.h      |   3 +
 security/commoncap.c            | 253 ++++++++++++++++++++++++++++++++++++++--
 5 files changed, 291 insertions(+), 10 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 4861322..5c0e7ae 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
 {
 	struct inode *inode = dentry->d_inode;
 	int error = -EOPNOTSUPP;
+	void *wvalue = NULL;
+	size_t wsize = 0;
 	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
 				   XATTR_SECURITY_PREFIX_LEN);
 
-	if (issec)
+	if (issec) {
 		inode->i_flags &= ~S_NOSEC;
+		/* if root in a non-init user_ns tries to set
+		 * security.capability, write a security.nscapability
+		 * in its place */
+		if (!strcmp(name, "security.capability") &&
+				current_user_ns() != &init_user_ns) {
+			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
+			if (!wvalue)
+				return -EPERM;
+			value = wvalue;
+			size = wsize;
+			name = "security.nscapability";
+		}
+	}
 	if (inode->i_op->setxattr) {
 		error = inode->i_op->setxattr(dentry, name, value, size, flags);
 		if (!error) {
@@ -114,6 +129,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
 			fsnotify_xattr(dentry);
 	}
 
+	kfree(wvalue);
 	return error;
 }
 
diff --git a/include/linux/capability.h b/include/linux/capability.h
index 00690ff..9376146 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -13,7 +13,7 @@
 #define _LINUX_CAPABILITY_H
 
 #include <uapi/linux/capability.h>
-
+#include <linux/uidgid.h>
 
 #define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_3
 #define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_3
@@ -31,6 +31,9 @@ struct cpu_vfs_cap_data {
 	kernel_cap_t inheritable;
 };
 
+#define NS_CAPS_VERSION(x) (x & 0xFF)
+#define NS_CAPS_FLAGS(x) ((x >> 8) & 0xFF)
+
 #define _USER_CAP_HEADER_SIZE  (sizeof(struct __user_cap_header_struct))
 #define _KERNEL_CAP_T_SIZE     (sizeof(kernel_cap_t))
 
@@ -240,4 +243,7 @@ extern bool file_ns_capable(const struct file *file, struct user_namespace *ns,
 /* audit system wants to get cap info from files as well */
 extern int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps);
 
+extern void cap_setxattr_make_nscap(struct dentry *dentry, const void *value,
+		size_t size, void **wvalue, size_t *wsize);
+
 #endif /* !_LINUX_CAPABILITY_H */
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 12c37a1..1f7e4c6 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -62,6 +62,9 @@ typedef struct __user_cap_data_struct {
 #define VFS_CAP_U32_2           2
 #define XATTR_CAPS_SZ_2         (sizeof(__le32)*(1 + 2*VFS_CAP_U32_2))
 
+/* version number for security.nscapability xattrs hdr->hdr_info */
+#define VFS_NS_CAP_REVISION     1
+
 #define XATTR_CAPS_SZ           XATTR_CAPS_SZ_2
 #define VFS_CAP_U32             VFS_CAP_U32_2
 #define VFS_CAP_REVISION	VFS_CAP_REVISION_2
@@ -74,6 +77,22 @@ struct vfs_cap_data {
 	} data[VFS_CAP_U32];
 };
 
+#define VFS_NS_CAP_EFFECTIVE    0x1
+/*
+ * 32-bit hdr_info contains
+ * 16 leftmost: reserved
+ * next 8: flags (only VFS_NS_CAP_EFFECTIVE so far)
+ * last 8: version
+ */
+struct vfs_ns_cap_data {
+       __le32 magic_etc;
+       __le32 rootid;
+       struct {
+               __le32 permitted;    /* Little endian */
+               __le32 inheritable;  /* Little endian */
+       } data[VFS_CAP_U32];
+};
+
 #ifndef __KERNEL__
 
 /*
diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
index 1590c49..67c80ab 100644
--- a/include/uapi/linux/xattr.h
+++ b/include/uapi/linux/xattr.h
@@ -68,6 +68,9 @@
 #define XATTR_CAPS_SUFFIX "capability"
 #define XATTR_NAME_CAPS XATTR_SECURITY_PREFIX XATTR_CAPS_SUFFIX
 
+#define XATTR_NS_CAPS_SUFFIX "nscapability"
+#define XATTR_NAME_NS_CAPS XATTR_SECURITY_PREFIX XATTR_NS_CAPS_SUFFIX
+
 #define XATTR_POSIX_ACL_ACCESS  "posix_acl_access"
 #define XATTR_NAME_POSIX_ACL_ACCESS XATTR_SYSTEM_PREFIX XATTR_POSIX_ACL_ACCESS
 #define XATTR_POSIX_ACL_DEFAULT  "posix_acl_default"
diff --git a/security/commoncap.c b/security/commoncap.c
index 48071ed..53161bc 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -310,13 +310,18 @@ int cap_inode_need_killpriv(struct dentry *dentry)
 	struct inode *inode = d_backing_inode(dentry);
 	int error;
 
-	if (!inode->i_op->getxattr)
+	if (!inode || !inode->i_op->getxattr)
 	       return 0;
 
+	error = inode->i_op->getxattr(dentry, XATTR_NAME_NS_CAPS, NULL, 0);
+	if (error > 0)
+		return 1;
+
 	error = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, NULL, 0);
-	if (error <= 0)
-		return 0;
-	return 1;
+	if (error > 0)
+		return 1;
+
+	return 0;
 }
 
 /**
@@ -330,11 +335,155 @@ int cap_inode_need_killpriv(struct dentry *dentry)
 int cap_inode_killpriv(struct dentry *dentry)
 {
 	struct inode *inode = d_backing_inode(dentry);
+	int ret1, ret2;
 
-	if (!inode->i_op->removexattr)
+	if (!inode || !inode->i_op->removexattr)
 	       return 0;
 
-	return inode->i_op->removexattr(dentry, XATTR_NAME_CAPS);
+	ret1 = inode->i_op->removexattr(dentry, XATTR_NAME_CAPS);
+	ret2 = inode->i_op->removexattr(dentry, XATTR_NAME_NS_CAPS);
+
+	if (ret1 != 0 && ret1 != -ENODATA)
+		return ret1;
+	if (ret2 == -ENODATA)
+		return 0;
+	return ret2;
+}
+
+/*
+ * getsecurity: We are called if reading of security.capable
+ * failed.  Since that does not exist, check whether the
+ * security.nscapability exists.  If it does, convert the kuid
+ * into the caller's context and return it
+ */
+static int cap_inode_getsecurity(struct inode *inode, const char *name, void **buffer, bool alloc)
+{
+	int error, ret;
+	struct user_namespace *ns;
+	kuid_t kroot;
+	uid_t root;
+	char *tmpbuf = NULL;
+	bool foundroot = false;
+	struct vfs_ns_cap_data *nscap;
+	struct dentry *dentry;
+
+	if (!inode->i_op->getxattr)
+		return -EOPNOTSUPP;
+
+	/* TODO - do we want to return the capability with the rootid converted
+	 * if this is security.nscapability?  It's not critical, so just say
+	 * no for now. */
+	if (strcmp(name, "nscapability") == 0 && current_user_ns() != &init_user_ns)
+		return -EPERM;
+
+	if (strcmp(name, "capability") != 0)
+		return -EOPNOTSUPP;
+
+	dentry = d_find_alias(inode);
+	if (!dentry)
+		return -EINVAL;
+
+	ret = vfs_getxattr_alloc(dentry, "security.nscapability",
+			&tmpbuf, 0, GFP_NOFS);
+
+	if (ret != sizeof(struct vfs_ns_cap_data)) {
+		kfree(tmpbuf);
+		return -EOPNOTSUPP;
+	}
+
+	/* verify the uid maps to a ancestor root uid, if so convert
+	   this to a valid security.capability */
+	nscap = (struct vfs_ns_cap_data *) tmpbuf;
+	root = le32_to_cpu(nscap->rootid);
+	kroot = make_kuid(&init_user_ns, root);
+	for (ns = current_user_ns(); ; ns = ns->parent) {
+		if (from_kuid(ns, kroot) == 0) {
+			foundroot = true;
+			break;
+		}
+		if (ns == &init_user_ns)
+			break;
+	}
+	if (!foundroot) {
+		kfree(tmpbuf);
+		return -EOPNOTSUPP;
+	}
+
+	error = sizeof(struct vfs_cap_data);
+	if (alloc) {
+		*buffer = kmalloc(sizeof(struct vfs_cap_data), GFP_ATOMIC);
+		if (*buffer) {
+			struct vfs_ns_cap_data *cap = *buffer;
+			__le32 nsmagic, magic;
+			memcpy(&cap->data, &nscap->data, sizeof(__le32) * 2 * VFS_CAP_U32);
+			nsmagic = le32_to_cpu(nscap->magic_etc);
+			magic = VFS_CAP_REVISION;
+			if (NS_CAPS_FLAGS(nsmagic) & VFS_NS_CAP_EFFECTIVE)
+				magic |= VFS_CAP_FLAGS_EFFECTIVE;
+			cap->magic_etc = cpu_to_le32(magic);
+		}
+	}
+	kfree(tmpbuf);
+	return error;
+}
+
+static bool dentry_has_nonns_capability(struct dentry *dentry)
+{
+	struct inode *inode = d_backing_inode(dentry);
+	int error;
+
+	if (!inode || !inode->i_op->getxattr)
+		return false;
+	error = inode->i_op->getxattr(dentry, "security.capability", NULL, 0);
+	if (error >= 0)
+		return true;
+	return false;
+}
+
+/*
+ * Use requested a write of security.capability but is in a non-init
+ * userns.  So we construct and write a security.nscapability.
+ *
+ * If all is ok, wvalue has an allocated new value.  Otherwise, wvalue
+ * is NULL.
+ */
+void cap_setxattr_make_nscap(struct dentry *dentry, const void *value, size_t size,
+				    void **wvalue, size_t *wsize)
+{
+	struct vfs_ns_cap_data nscap;
+	const struct vfs_cap_data *cap = value;
+	__u32 magic, nsmagic;
+	struct user_namespace *ns = current_user_ns();
+	struct inode *inode = d_backing_inode(dentry);
+	kuid_t rootid;
+
+	if (!value || size != sizeof(struct vfs_cap_data))
+		return;
+	if (!inode || !capable_wrt_inode_uidgid(inode, CAP_SETFCAP))
+		return;
+
+	/* refuse if security.capability exists */
+	if (dentry_has_nonns_capability(dentry))
+		return;
+
+	rootid = make_kuid(ns, 0);
+	if (!uid_valid(rootid))
+		return;
+
+	nscap.rootid = cpu_to_le32(from_kuid(&init_user_ns, rootid));
+	nsmagic = VFS_NS_CAP_REVISION;
+	magic = le32_to_cpu(cap->magic_etc);
+	if (magic & VFS_CAP_FLAGS_EFFECTIVE)
+		nsmagic |= VFS_NS_CAP_REVISION << 8;
+	nscap.magic_etc = cpu_to_le32(nsmagic);
+	memcpy(&nscap.data, &cap->data, sizeof(__le32) * 2 * VFS_CAP_U32);
+
+	*wsize = sizeof(struct vfs_ns_cap_data);
+	*wvalue = kmalloc(*wsize, GFP_ATOMIC);
+	if (!*wvalue)
+		return;
+	memcpy(*wvalue, &nscap, *wsize);
+	return;
 }
 
 /*
@@ -438,6 +587,68 @@ int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data
 	return 0;
 }
 
+int get_vfs_ns_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps)
+{
+	struct inode *inode = d_backing_inode(dentry);
+	unsigned i;
+	u32 magic_etc;
+	ssize_t size;
+	struct vfs_ns_cap_data nscap;
+	bool foundroot = false;
+	kuid_t kroot;
+	uid_t root;
+	struct user_namespace *ns;
+
+	memset(cpu_caps, 0, sizeof(struct cpu_vfs_cap_data));
+
+	if (!inode || !inode->i_op->getxattr)
+		return -ENODATA;
+
+	size = inode->i_op->getxattr((struct dentry *)dentry, XATTR_NAME_NS_CAPS,
+			&nscap, sizeof(nscap));
+	if (size == -ENODATA || size == -EOPNOTSUPP)
+		/* no data, that's ok */
+		return -ENODATA;
+	if (size < 0)
+		return size;
+	if (size != sizeof(nscap))
+		return -EINVAL;
+
+	root = le32_to_cpu(nscap.rootid);
+	kroot = make_kuid(&init_user_ns, root);
+	for (ns = current_user_ns(); ; ns = ns->parent) {
+		if (from_kuid(ns, kroot) == 0) {
+			foundroot = true;
+			break;
+		}
+		if (ns == &init_user_ns)
+			break;
+	}
+	if (!foundroot)
+		return -ENODATA;
+
+	magic_etc = le32_to_cpu(nscap.magic_etc);
+
+	if (NS_CAPS_VERSION(magic_etc) != VFS_NS_CAP_REVISION)
+		return -EINVAL;
+
+	cpu_caps->magic_etc = VFS_CAP_REVISION_2;
+	if (NS_CAPS_FLAGS(magic_etc) & VFS_NS_CAP_EFFECTIVE)
+		cpu_caps->magic_etc |= VFS_CAP_FLAGS_EFFECTIVE;
+	/* copy the entry */
+	CAP_FOR_EACH_U32(i) {
+		if (i >= VFS_CAP_U32_2)
+			break;
+		cpu_caps->permitted.cap[i] = le32_to_cpu(nscap.data[i].permitted);
+		cpu_caps->inheritable.cap[i] = le32_to_cpu(nscap.data[i].inheritable);
+	}
+
+	cpu_caps->permitted.cap[CAP_LAST_U32] &= CAP_LAST_U32_VALID_MASK;
+	cpu_caps->inheritable.cap[CAP_LAST_U32] &= CAP_LAST_U32_VALID_MASK;
+
+	return 0;
+}
+
 /*
  * Attempt to get the on-exec apply capability sets for an executable file from
  * its xattrs and, if present, apply them to the proposed credentials being
@@ -457,10 +668,12 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
 		return 0;
 
 	rc = get_vfs_caps_from_disk(bprm->file->f_path.dentry, &vcaps);
+	if (rc == -ENODATA)
+		rc = get_vfs_ns_caps_from_disk(bprm->file->f_path.dentry, &vcaps);
 	if (rc < 0) {
 		if (rc == -EINVAL)
-			printk(KERN_NOTICE "%s: get_vfs_caps_from_disk returned %d for %s\n",
-				__func__, rc, bprm->filename);
+			printk(KERN_NOTICE "Invalid argument reading file caps for %s\n",
+					bprm->filename);
 		else if (rc == -ENODATA)
 			rc = 0;
 		goto out;
@@ -657,6 +870,18 @@ int cap_inode_setxattr(struct dentry *dentry, const char *name,
 		       const void *value, size_t size, int flags)
 {
 	if (!strcmp(name, XATTR_NAME_CAPS)) {
+		if (current_user_ns() == &init_user_ns && !capable(CAP_SETFCAP))
+			return -EPERM;
+		/* for non-init userns we'll check permission later in
+		 * cap_setxattr_make_nscap() */
+		return 0;
+	}
+
+	if (!strcmp(name, XATTR_NAME_NS_CAPS)) {
+		/* only initial userns is allowed to set security.nscapability
+		 * directly.  We could be more flexible, but would need to
+		 * convert the rootid to target ns.  Defer.
+		 */
 		if (!capable(CAP_SETFCAP))
 			return -EPERM;
 		return 0;
@@ -682,12 +907,23 @@ int cap_inode_setxattr(struct dentry *dentry, const char *name,
  */
 int cap_inode_removexattr(struct dentry *dentry, const char *name)
 {
+	struct inode *inode = d_backing_inode(dentry);
 	if (!strcmp(name, XATTR_NAME_CAPS)) {
 		if (!capable(CAP_SETFCAP))
 			return -EPERM;
 		return 0;
 	}
 
+	if (!strcmp(name, XATTR_NAME_NS_CAPS)) {
+		/* do allow root to clear this capability out if they really
+		 * want to */
+		if (!inode)
+			return -EINVAL;
+		if (!capable_wrt_inode_uidgid(inode, CAP_SETFCAP))
+			return -EPERM;
+		return 0;
+	}
+
 	if (!strncmp(name, XATTR_SECURITY_PREFIX,
 		     sizeof(XATTR_SECURITY_PREFIX) - 1) &&
 	    !capable(CAP_SYS_ADMIN))
@@ -1078,6 +1314,7 @@ struct security_hook_list capability_hooks[] = {
 	LSM_HOOK_INIT(bprm_secureexec, cap_bprm_secureexec),
 	LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv),
 	LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv),
+	LSM_HOOK_INIT(inode_getsecurity, cap_inode_getsecurity),
 	LSM_HOOK_INIT(mmap_addr, cap_mmap_addr),
 	LSM_HOOK_INIT(mmap_file, cap_mmap_file),
 	LSM_HOOK_INIT(task_fix_setuid, cap_task_fix_setuid),
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1403961 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> This patch introduces a new security.nscapability xattr.  It
> is mostly like security.capability, but also lists a 'rootid'.
> This is the uid_t (in init_user_ns) of the root id (uid 0 in a
> namespace) in whose namespaces the file capabilities may take
> effect.
> 
> A privileged (cap_setfcap) process in the initial user ns may
> set and read this xattr directly.  However, its real intent is
> to be used as a transparent fallback in user namespaces.
> 
> Root in a user ns cannot be trusted to write security.capability
> xattrs, because any user on the host could map his own uid to root
> in a namespace, write the xattr, and execute the file with privilege
> on the host.
> 
> With this patch, when root in a user ns asks to write security.capability,
> the kernel will transparently write a security.nscapability xattr
> instead, filling in the kuid of the calling user's root uid.  Subsequently,
> any task executing the file which has the noted k_uid as its root uid,
> or which is in a descendent user_ns of such a user_ns, will run the
> file with capabilities.
> 
> When reading the security.capability xattr from a non-init user_ns, a valid
> security.nscapability will be shown if it exists.  Such a task is not
> allowed to read security.nscapability.  This could be accomodated, however

Add the word "directly" as "to read security.nscapability directly".

> it requires the kernel to convert the kuid_t to a valid uid in the reader's
> user_ns.  So for now it's simply not supported.

I really like the idea that the kernel transparently replaces
nscapability for capability.

> Only a single security.nscapability xattr may be written.  This patch
> could be expanded to allow a list of capabilities and rootids, however
> I do not believe that to be a worthwhile use case.

Ok

> This allows a simple setxattr to work, allows tar/untar to
> work, and allows us to tar in one namespace and untar in
> another while preserving the capability, without risking
> leaking privilege into a parent namespace.
> 
> Note - listxattr is not being handled here.  So results of that can be
> inconsistent with get/setxattr.  Fixing that will require yet more
> deceit in fs/xattr.c.
> 
> Note2 - it may be less sneaky to hide all the magic behind the
> security.nscapability xattr.  So userspace would need to know to
> use that xattr name when needed, but with the same format as
> security.capability.  The kuid_t rootid would be filled in by the
> kernel.  That's a middle ground between my last patch and this one.

The less userspace needs to differentiate between running in a namespace
and not, the better.

Note3 - capability is currently protected by EVM, when enabled.  Should
ns_capability also be a protected xattr?

> Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
> ---
>  fs/xattr.c                      |  18 ++-
>  include/linux/capability.h      |   8 +-
>  include/uapi/linux/capability.h |  19 +++
>  include/uapi/linux/xattr.h      |   3 +
>  security/commoncap.c            | 253 ++++++++++++++++++++++++++++++++++++++--
>  5 files changed, 291 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 4861322..5c0e7ae 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
>  {
>  	struct inode *inode = dentry->d_inode;
>  	int error = -EOPNOTSUPP;
> +	void *wvalue = NULL;
> +	size_t wsize = 0;
>  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
>  				   XATTR_SECURITY_PREFIX_LEN);
> 
> -	if (issec)
> +	if (issec) {
>  		inode->i_flags &= ~S_NOSEC;
> +		/* if root in a non-init user_ns tries to set
> +		 * security.capability, write a security.nscapability
> +		 * in its place */
> +		if (!strcmp(name, "security.capability") &&
> +				current_user_ns() != &init_user_ns) {
> +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
> +			if (!wvalue)
> +				return -EPERM;
> +			value = wvalue;
> +			size = wsize;
> +			name = "security.nscapability";
> +		}

The call to capable_wrt_inode_uidgid() is hidden behind
cap_setxattr_make_nscap().  Does it make sense to call it here instead,
before the security.capability test?  This would lay the foundation for
doing something similar for IMA.

(Will continue reviewing ...)

Mimi

[toc] | [prev] | [next] | [standalone]

#1404114 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> > This patch introduces a new security.nscapability xattr.  It
> > is mostly like security.capability, but also lists a 'rootid'.
> > This is the uid_t (in init_user_ns) of the root id (uid 0 in a
> > namespace) in whose namespaces the file capabilities may take
> > effect.
> > 
> > A privileged (cap_setfcap) process in the initial user ns may
> > set and read this xattr directly.  However, its real intent is
> > to be used as a transparent fallback in user namespaces.
> > 
> > Root in a user ns cannot be trusted to write security.capability
> > xattrs, because any user on the host could map his own uid to root
> > in a namespace, write the xattr, and execute the file with privilege
> > on the host.
> > 
> > With this patch, when root in a user ns asks to write security.capability,
> > the kernel will transparently write a security.nscapability xattr
> > instead, filling in the kuid of the calling user's root uid.  Subsequently,
> > any task executing the file which has the noted k_uid as its root uid,
> > or which is in a descendent user_ns of such a user_ns, will run the
> > file with capabilities.
> > 
> > When reading the security.capability xattr from a non-init user_ns, a valid
> > security.nscapability will be shown if it exists.  Such a task is not
> > allowed to read security.nscapability.  This could be accomodated, however
> 
> Add the word "directly" as "to read security.nscapability directly".

Updated in my git tree.

> > it requires the kernel to convert the kuid_t to a valid uid in the reader's
> > user_ns.  So for now it's simply not supported.
> 
> I really like the idea that the kernel transparently replaces
> nscapability for capability.
> 
> > Only a single security.nscapability xattr may be written.  This patch
> > could be expanded to allow a list of capabilities and rootids, however
> > I do not believe that to be a worthwhile use case.
> 
> Ok
> 
> > This allows a simple setxattr to work, allows tar/untar to
> > work, and allows us to tar in one namespace and untar in
> > another while preserving the capability, without risking
> > leaking privilege into a parent namespace.
> > 
> > Note - listxattr is not being handled here.  So results of that can be
> > inconsistent with get/setxattr.  Fixing that will require yet more
> > deceit in fs/xattr.c.
> > 
> > Note2 - it may be less sneaky to hide all the magic behind the
> > security.nscapability xattr.  So userspace would need to know to
> > use that xattr name when needed, but with the same format as
> > security.capability.  The kuid_t rootid would be filled in by the
> > kernel.  That's a middle ground between my last patch and this one.
> 
> The less userspace needs to differentiate between running in a namespace
> and not, the better.
> 
> Note3 - capability is currently protected by EVM, when enabled.  Should
> ns_capability also be a protected xattr?

Hm - that would protect it from offline attacks, but allow the container
to update it, right?  That sounds good.

> > Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
> > ---
> >  fs/xattr.c                      |  18 ++-
> >  include/linux/capability.h      |   8 +-
> >  include/uapi/linux/capability.h |  19 +++
> >  include/uapi/linux/xattr.h      |   3 +
> >  security/commoncap.c            | 253 ++++++++++++++++++++++++++++++++++++++--
> >  5 files changed, 291 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index 4861322..5c0e7ae 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
> >  {
> >  	struct inode *inode = dentry->d_inode;
> >  	int error = -EOPNOTSUPP;
> > +	void *wvalue = NULL;
> > +	size_t wsize = 0;
> >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
> >  				   XATTR_SECURITY_PREFIX_LEN);
> > 
> > -	if (issec)
> > +	if (issec) {
> >  		inode->i_flags &= ~S_NOSEC;
> > +		/* if root in a non-init user_ns tries to set
> > +		 * security.capability, write a security.nscapability
> > +		 * in its place */
> > +		if (!strcmp(name, "security.capability") &&
> > +				current_user_ns() != &init_user_ns) {
> > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
> > +			if (!wvalue)
> > +				return -EPERM;
> > +			value = wvalue;
> > +			size = wsize;
> > +			name = "security.nscapability";
> > +		}
> 
> The call to capable_wrt_inode_uidgid() is hidden behind
> cap_setxattr_make_nscap().  Does it make sense to call it here instead,
> before the security.capability test?  This would lay the foundation for
> doing something similar for IMA.

Might make sense to move that.  Though looking at it with fresh eyes I wonder
whether adding less code here at __vfs_setxattr_noperm(), i.e.

		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
			return -EPERM;

would be cleaner.

> (Will continue reviewing ...)

Awesome, thanks Mimi.

[toc] | [prev] | [next] | [standalone]

#1404347 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:

> > > diff --git a/fs/xattr.c b/fs/xattr.c
> > > index 4861322..5c0e7ae 100644
> > > --- a/fs/xattr.c
> > > +++ b/fs/xattr.c
> > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
> > >  {
> > >  	struct inode *inode = dentry->d_inode;
> > >  	int error = -EOPNOTSUPP;
> > > +	void *wvalue = NULL;
> > > +	size_t wsize = 0;
> > >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
> > >  				   XATTR_SECURITY_PREFIX_LEN);
> > > 
> > > -	if (issec)
> > > +	if (issec) {
> > >  		inode->i_flags &= ~S_NOSEC;
> > > +		/* if root in a non-init user_ns tries to set
> > > +		 * security.capability, write a security.nscapability
> > > +		 * in its place */
> > > +		if (!strcmp(name, "security.capability") &&
> > > +				current_user_ns() != &init_user_ns) {
> > > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
> > > +			if (!wvalue)
> > > +				return -EPERM;
> > > +			value = wvalue;
> > > +			size = wsize;
> > > +			name = "security.nscapability";
> > > +		}
> > 
> > The call to capable_wrt_inode_uidgid() is hidden behind
> > cap_setxattr_make_nscap().  Does it make sense to call it here instead,
> > before the security.capability test?  This would lay the foundation for
> > doing something similar for IMA.
> 
> Might make sense to move that.  Though looking at it with fresh eyes I wonder
> whether adding less code here at __vfs_setxattr_noperm(), i.e.
> 
> 		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
> 			return -EPERM;
> 
> would be cleaner.

Yes, it would be cleaner,  but I'm suggesting you do all the hard work
making it generic.  Then the rest of us can follow your lead.  Its more
likely that you'll get it right.  At a high level, it might look like:

               /* Permit root in a non-init user_ns to modify the security
                 * namespace xattr equivalents (eg. nscapability, ns_ima, etc). 
                 */
                if ((current_user_ns() != &init_user_ns) &&
                        capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) {

			if  security..capability
				call capability  /* set nscapability? */

			else if security.ima 
				call ima 	/* set ns_ima? */
		}

Mimi

[toc] | [prev] | [next] | [standalone]

#1404684 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
>> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
>> > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
>
>> > > diff --git a/fs/xattr.c b/fs/xattr.c
>> > > index 4861322..5c0e7ae 100644
>> > > --- a/fs/xattr.c
>> > > +++ b/fs/xattr.c
>> > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
>> > >  {
>> > >  	struct inode *inode = dentry->d_inode;
>> > >  	int error = -EOPNOTSUPP;
>> > > +	void *wvalue = NULL;
>> > > +	size_t wsize = 0;
>> > >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
>> > >  				   XATTR_SECURITY_PREFIX_LEN);
>> > > 
>> > > -	if (issec)
>> > > +	if (issec) {
>> > >  		inode->i_flags &= ~S_NOSEC;
>> > > +		/* if root in a non-init user_ns tries to set
>> > > +		 * security.capability, write a security.nscapability
>> > > +		 * in its place */
>> > > +		if (!strcmp(name, "security.capability") &&
>> > > +				current_user_ns() != &init_user_ns) {
>> > > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
>> > > +			if (!wvalue)
>> > > +				return -EPERM;
>> > > +			value = wvalue;
>> > > +			size = wsize;
>> > > +			name = "security.nscapability";
>> > > +		}
>> > 
>> > The call to capable_wrt_inode_uidgid() is hidden behind
>> > cap_setxattr_make_nscap().  Does it make sense to call it here instead,
>> > before the security.capability test?  This would lay the foundation for
>> > doing something similar for IMA.
>> 
>> Might make sense to move that.  Though looking at it with fresh eyes I wonder
>> whether adding less code here at __vfs_setxattr_noperm(), i.e.
>> 
>> 		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
>> 			return -EPERM;
>> 
>> would be cleaner.
>
> Yes, it would be cleaner,  but I'm suggesting you do all the hard work
> making it generic.  Then the rest of us can follow your lead.  Its more
> likely that you'll get it right.  At a high level, it might look like:
>
>                /* Permit root in a non-init user_ns to modify the security
>                  * namespace xattr equivalents (eg. nscapability, ns_ima, etc). 
>                  */
>                 if ((current_user_ns() != &init_user_ns) &&
>                         capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) {
>
> 			if  security..capability
> 				call capability  /* set nscapability? */
>
> 			else if security.ima 
> 				call ima 	/* set ns_ima? */
> 		}

Hmm.  I am confused about this part of the strategy.

I don't understand the capability vs nscapability distinction.  It seems
to add complexity without benefit.

If I am in a nested user namespace and I try to write a capability on a
file and it has nscapability I can't be allowed to (as a more privileged
user namespace already wrote it).

Not rewriting an existing attribute seems to be the only benefit I can
see to have both a capability and a nscapability attribute vs having
a new version of the capability attribute.

Am I missing something here?

Mimi as for generalizing the code for handling IMA I expect it makes
sense to refactor the code to have shared library functions (or whatever
it takes to generalize the code) when you are ready to implement this
kind of IMA attribute.  That way in the first implementation we can
concentrate on getting the code clean and the sematics correct.

That alone seems to be taking a while.

Eric

[toc] | [prev] | [next] | [standalone]

#1404690 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

On Fri, 2016-05-20 at 13:28 -0500, Eric W. Biederman wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
> >> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> >> > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> >
> >> > > diff --git a/fs/xattr.c b/fs/xattr.c
> >> > > index 4861322..5c0e7ae 100644
> >> > > --- a/fs/xattr.c
> >> > > +++ b/fs/xattr.c
> >> > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
> >> > >  {
> >> > >  	struct inode *inode = dentry->d_inode;
> >> > >  	int error = -EOPNOTSUPP;
> >> > > +	void *wvalue = NULL;
> >> > > +	size_t wsize = 0;
> >> > >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
> >> > >  				   XATTR_SECURITY_PREFIX_LEN);
> >> > > 
> >> > > -	if (issec)
> >> > > +	if (issec) {
> >> > >  		inode->i_flags &= ~S_NOSEC;
> >> > > +		/* if root in a non-init user_ns tries to set
> >> > > +		 * security.capability, write a security.nscapability
> >> > > +		 * in its place */
> >> > > +		if (!strcmp(name, "security.capability") &&
> >> > > +				current_user_ns() != &init_user_ns) {
> >> > > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
> >> > > +			if (!wvalue)
> >> > > +				return -EPERM;
> >> > > +			value = wvalue;
> >> > > +			size = wsize;
> >> > > +			name = "security.nscapability";
> >> > > +		}
> >> > 
> >> > The call to capable_wrt_inode_uidgid() is hidden behind
> >> > cap_setxattr_make_nscap().  Does it make sense to call it here instead,
> >> > before the security.capability test?  This would lay the foundation for
> >> > doing something similar for IMA.
> >> 
> >> Might make sense to move that.  Though looking at it with fresh eyes I wonder
> >> whether adding less code here at __vfs_setxattr_noperm(), i.e.
> >> 
> >> 		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
> >> 			return -EPERM;
> >> 
> >> would be cleaner.
> >
> > Yes, it would be cleaner,  but I'm suggesting you do all the hard work
> > making it generic.  Then the rest of us can follow your lead.  Its more
> > likely that you'll get it right.  At a high level, it might look like:
> >
> >                /* Permit root in a non-init user_ns to modify the security
> >                  * namespace xattr equivalents (eg. nscapability, ns_ima, etc). 
> >                  */
> >                 if ((current_user_ns() != &init_user_ns) &&
> >                         capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) {
> >
> > 			if  security..capability
> > 				call capability  /* set nscapability? */
> >
> > 			else if security.ima 
> > 				call ima 	/* set ns_ima? */
> > 		}
> 
> Hmm.  I am confused about this part of the strategy.
> 
> I don't understand the capability vs nscapability distinction.  It seems
> to add complexity without benefit.

Only real root can write security xattrs, which prevents root in a
namespace from writing the included security xattrs in a tar package
from being installed.  Nobody is suggesting changing this behavior.
Serge's solution is to allow an equivalent xattr to be written.  For
capabilities, it would be ns_capability.   Similarly, IMA would write
security.ns_ima.  (I'm not sure about SELinux or Smack.)

> If I am in a nested user namespace and I try to write a capability on a
> file and it has nscapability I can't be allowed to (as a more privileged
> user namespace already wrote it).
> 
> Not rewriting an existing attribute seems to be the only benefit I can
> see to have both a capability and a nscapability attribute vs having
> a new version of the capability attribute.
> 
> Am I missing something here?

Only real root in the namespace can write the equivalent security xattr.
I'm hoping this can be done without having to modify userspace apps.  (I
hope that answers your question.)

> Mimi as for generalizing the code for handling IMA I expect it makes
> sense to refactor the code to have shared library functions (or whatever
> it takes to generalize the code) when you are ready to implement this
> kind of IMA attribute.  That way in the first implementation we can
> concentrate on getting the code clean and the sematics correct.
> 
> That alone seems to be taking a while.

There should be a generic solution that works for security xattrs, not
just capabilities.

Mimi

[toc] | [prev] | [next] | [standalone]

#1404695 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Fri, 2016-05-20 at 13:28 -0500, Eric W. Biederman wrote:
>> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> 
>> > On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
>> >> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
>> >> > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
>> >
>> >> > > diff --git a/fs/xattr.c b/fs/xattr.c
>> >> > > index 4861322..5c0e7ae 100644
>> >> > > --- a/fs/xattr.c
>> >> > > +++ b/fs/xattr.c
>> >> > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
>> >> > >  {
>> >> > >  	struct inode *inode = dentry->d_inode;
>> >> > >  	int error = -EOPNOTSUPP;
>> >> > > +	void *wvalue = NULL;
>> >> > > +	size_t wsize = 0;
>> >> > >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
>> >> > >  				   XATTR_SECURITY_PREFIX_LEN);
>> >> > > 
>> >> > > -	if (issec)
>> >> > > +	if (issec) {
>> >> > >  		inode->i_flags &= ~S_NOSEC;
>> >> > > +		/* if root in a non-init user_ns tries to set
>> >> > > +		 * security.capability, write a security.nscapability
>> >> > > +		 * in its place */
>> >> > > +		if (!strcmp(name, "security.capability") &&
>> >> > > +				current_user_ns() != &init_user_ns) {
>> >> > > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
>> >> > > +			if (!wvalue)
>> >> > > +				return -EPERM;
>> >> > > +			value = wvalue;
>> >> > > +			size = wsize;
>> >> > > +			name = "security.nscapability";
>> >> > > +		}
>> >> > 
>> >> > The call to capable_wrt_inode_uidgid() is hidden behind
>> >> > cap_setxattr_make_nscap().  Does it make sense to call it here instead,
>> >> > before the security.capability test?  This would lay the foundation for
>> >> > doing something similar for IMA.
>> >> 
>> >> Might make sense to move that.  Though looking at it with fresh eyes I wonder
>> >> whether adding less code here at __vfs_setxattr_noperm(), i.e.
>> >> 
>> >> 		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
>> >> 			return -EPERM;
>> >> 
>> >> would be cleaner.
>> >
>> > Yes, it would be cleaner,  but I'm suggesting you do all the hard work
>> > making it generic.  Then the rest of us can follow your lead.  Its more
>> > likely that you'll get it right.  At a high level, it might look like:
>> >
>> >                /* Permit root in a non-init user_ns to modify the security
>> >                  * namespace xattr equivalents (eg. nscapability, ns_ima, etc). 
>> >                  */
>> >                 if ((current_user_ns() != &init_user_ns) &&
>> >                         capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) {
>> >
>> > 			if  security..capability
>> > 				call capability  /* set nscapability? */
>> >
>> > 			else if security.ima 
>> > 				call ima 	/* set ns_ima? */
>> > 		}
>> 
>> Hmm.  I am confused about this part of the strategy.
>> 
>> I don't understand the capability vs nscapability distinction.  It seems
>> to add complexity without benefit.
>
> Only real root can write security xattrs, which prevents root in a
> namespace from writing the included security xattrs in a tar package
> from being installed.  Nobody is suggesting changing this behavior.
> Serge's solution is to allow an equivalent xattr to be written.  For
> capabilities, it would be ns_capability.   Similarly, IMA would write
> security.ns_ima.  (I'm not sure about SELinux or Smack.)

nscapability is an xattr in the security namespace.  So root in a
namespace can write to a specific security xattr.

I am saying that at least at a quick examinination that having two
attributes that control the same things appears to be a mistake, as it
just makes it easy for them to get out of sync and generally be
confusing.

>> If I am in a nested user namespace and I try to write a capability on a
>> file and it has nscapability I can't be allowed to (as a more privileged
>> user namespace already wrote it).
>> 
>> Not rewriting an existing attribute seems to be the only benefit I can
>> see to have both a capability and a nscapability attribute vs having
>> a new version of the capability attribute.
>> 
>> Am I missing something here?
>
> Only real root in the namespace can write the equivalent security xattr.
> I'm hoping this can be done without having to modify userspace apps.  (I
> hope that answers your question.)

But it is possible and actually desirable to have user namespaces nested
in user namespaces nested in user namespaces.  Which means the existing
xattr must be examined before being written.

At which point I don't see a gain by having both a capability and an
nscapability attribute.

>> Mimi as for generalizing the code for handling IMA I expect it makes
>> sense to refactor the code to have shared library functions (or whatever
>> it takes to generalize the code) when you are ready to implement this
>> kind of IMA attribute.  That way in the first implementation we can
>> concentrate on getting the code clean and the sematics correct.
>> 
>> That alone seems to be taking a while.
>
> There should be a generic solution that works for security xattrs, not
> just capabilities.

However we don't have to start there.  We can think about it but we
probably should not start there.

We certainly need infrastructure to support this case.  Security modules
deliberately can be very unique so in general it is not possible to come
up with a design for all cases.  Especially as nesting of user
namespaces does not imply nesting of security module contexts.

Eric

[toc] | [prev] | [next] | [standalone]

#1404696 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

Quoting Eric W. Biederman (ebiederm@xmission.com):
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
> >> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> >> > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> >
> >> > > diff --git a/fs/xattr.c b/fs/xattr.c
> >> > > index 4861322..5c0e7ae 100644
> >> > > --- a/fs/xattr.c
> >> > > +++ b/fs/xattr.c
> >> > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
> >> > >  {
> >> > >  	struct inode *inode = dentry->d_inode;
> >> > >  	int error = -EOPNOTSUPP;
> >> > > +	void *wvalue = NULL;
> >> > > +	size_t wsize = 0;
> >> > >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
> >> > >  				   XATTR_SECURITY_PREFIX_LEN);
> >> > > 
> >> > > -	if (issec)
> >> > > +	if (issec) {
> >> > >  		inode->i_flags &= ~S_NOSEC;
> >> > > +		/* if root in a non-init user_ns tries to set
> >> > > +		 * security.capability, write a security.nscapability
> >> > > +		 * in its place */
> >> > > +		if (!strcmp(name, "security.capability") &&
> >> > > +				current_user_ns() != &init_user_ns) {
> >> > > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
> >> > > +			if (!wvalue)
> >> > > +				return -EPERM;
> >> > > +			value = wvalue;
> >> > > +			size = wsize;
> >> > > +			name = "security.nscapability";
> >> > > +		}
> >> > 
> >> > The call to capable_wrt_inode_uidgid() is hidden behind
> >> > cap_setxattr_make_nscap().  Does it make sense to call it here instead,
> >> > before the security.capability test?  This would lay the foundation for
> >> > doing something similar for IMA.
> >> 
> >> Might make sense to move that.  Though looking at it with fresh eyes I wonder
> >> whether adding less code here at __vfs_setxattr_noperm(), i.e.
> >> 
> >> 		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
> >> 			return -EPERM;
> >> 
> >> would be cleaner.
> >
> > Yes, it would be cleaner,  but I'm suggesting you do all the hard work
> > making it generic.  Then the rest of us can follow your lead.  Its more
> > likely that you'll get it right.  At a high level, it might look like:
> >
> >                /* Permit root in a non-init user_ns to modify the security
> >                  * namespace xattr equivalents (eg. nscapability, ns_ima, etc). 
> >                  */
> >                 if ((current_user_ns() != &init_user_ns) &&
> >                         capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) {
> >
> > 			if  security..capability
> > 				call capability  /* set nscapability? */
> >
> > 			else if security.ima 
> > 				call ima 	/* set ns_ima? */
> > 		}
> 
> Hmm.  I am confused about this part of the strategy.
> 
> I don't understand the capability vs nscapability distinction.  It seems
> to add complexity without benefit.

...  Well, yes, we could simply make a new version of security.capability
xattr, and make rootid == 0 mean it was written by the init_user_ns.  Is
that what you mean?

-serge

[toc] | [prev] | [next] | [standalone]

#1404699 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serge@hallyn.com> writes:
> 
> > Quoting Eric W. Biederman (ebiederm@xmission.com):
> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> >> 
> >> > On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
> >> >> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> >> >> > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> >> >
> >> >> > > diff --git a/fs/xattr.c b/fs/xattr.c
> >> >> > > index 4861322..5c0e7ae 100644
> >> >> > > --- a/fs/xattr.c
> >> >> > > +++ b/fs/xattr.c
> >> >> > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
> >> >> > >  {
> >> >> > >  	struct inode *inode = dentry->d_inode;
> >> >> > >  	int error = -EOPNOTSUPP;
> >> >> > > +	void *wvalue = NULL;
> >> >> > > +	size_t wsize = 0;
> >> >> > >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
> >> >> > >  				   XATTR_SECURITY_PREFIX_LEN);
> >> >> > > 
> >> >> > > -	if (issec)
> >> >> > > +	if (issec) {
> >> >> > >  		inode->i_flags &= ~S_NOSEC;
> >> >> > > +		/* if root in a non-init user_ns tries to set
> >> >> > > +		 * security.capability, write a security.nscapability
> >> >> > > +		 * in its place */
> >> >> > > +		if (!strcmp(name, "security.capability") &&
> >> >> > > +				current_user_ns() != &init_user_ns) {
> >> >> > > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
> >> >> > > +			if (!wvalue)
> >> >> > > +				return -EPERM;
> >> >> > > +			value = wvalue;
> >> >> > > +			size = wsize;
> >> >> > > +			name = "security.nscapability";
> >> >> > > +		}
> >> >> > 
> >> >> > The call to capable_wrt_inode_uidgid() is hidden behind
> >> >> > cap_setxattr_make_nscap().  Does it make sense to call it here instead,
> >> >> > before the security.capability test?  This would lay the foundation for
> >> >> > doing something similar for IMA.
> >> >> 
> >> >> Might make sense to move that.  Though looking at it with fresh eyes I wonder
> >> >> whether adding less code here at __vfs_setxattr_noperm(), i.e.
> >> >> 
> >> >> 		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
> >> >> 			return -EPERM;
> >> >> 
> >> >> would be cleaner.
> >> >
> >> > Yes, it would be cleaner,  but I'm suggesting you do all the hard work
> >> > making it generic.  Then the rest of us can follow your lead.  Its more
> >> > likely that you'll get it right.  At a high level, it might look like:
> >> >
> >> >                /* Permit root in a non-init user_ns to modify the security
> >> >                  * namespace xattr equivalents (eg. nscapability, ns_ima, etc). 
> >> >                  */
> >> >                 if ((current_user_ns() != &init_user_ns) &&
> >> >                         capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) {
> >> >
> >> > 			if  security..capability
> >> > 				call capability  /* set nscapability? */
> >> >
> >> > 			else if security.ima 
> >> > 				call ima 	/* set ns_ima? */
> >> > 		}
> >> 
> >> Hmm.  I am confused about this part of the strategy.
> >> 
> >> I don't understand the capability vs nscapability distinction.  It seems
> >> to add complexity without benefit.
> >
> > ...  Well, yes, we could simply make a new version of security.capability
> > xattr, and make rootid == 0 mean it was written by the init_user_ns.  Is
> > that what you mean?
> 
> Yes.
> 
> That would seem to simplify the logic to ensure the policy we enforce is
> consistent with what is on disk.

I'll give that a shot.  I think the reason I did it this way was that I'm
still kind of stuck in the not-magic way of thinking about it.  But yeah
with the kernel magically writing inthe kuid there's probably no reason not
to.

[toc] | [prev] | [next] | [standalone]

#1404768 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

On Fri, 2016-05-20 at 14:59 -0500, Serge E. Hallyn wrote:
> Quoting Eric W. Biederman (ebiederm@xmission.com):
> > "Serge E. Hallyn" <serge@hallyn.com> writes:
> > 
> > > Quoting Eric W. Biederman (ebiederm@xmission.com):
> > >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> > >> 
> > >> > On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
> > >> >> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > >> >> > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> > >> >
> > >> >> > > diff --git a/fs/xattr.c b/fs/xattr.c
> > >> >> > > index 4861322..5c0e7ae 100644
> > >> >> > > --- a/fs/xattr.c
> > >> >> > > +++ b/fs/xattr.c
> > >> >> > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
> > >> >> > >  {
> > >> >> > >  	struct inode *inode = dentry->d_inode;
> > >> >> > >  	int error = -EOPNOTSUPP;
> > >> >> > > +	void *wvalue = NULL;
> > >> >> > > +	size_t wsize = 0;
> > >> >> > >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
> > >> >> > >  				   XATTR_SECURITY_PREFIX_LEN);
> > >> >> > > 
> > >> >> > > -	if (issec)
> > >> >> > > +	if (issec) {
> > >> >> > >  		inode->i_flags &= ~S_NOSEC;
> > >> >> > > +		/* if root in a non-init user_ns tries to set
> > >> >> > > +		 * security.capability, write a security.nscapability
> > >> >> > > +		 * in its place */
> > >> >> > > +		if (!strcmp(name, "security.capability") &&
> > >> >> > > +				current_user_ns() != &init_user_ns) {
> > >> >> > > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
> > >> >> > > +			if (!wvalue)
> > >> >> > > +				return -EPERM;
> > >> >> > > +			value = wvalue;
> > >> >> > > +			size = wsize;
> > >> >> > > +			name = "security.nscapability";
> > >> >> > > +		}
> > >> >> > 
> > >> >> > The call to capable_wrt_inode_uidgid() is hidden behind
> > >> >> > cap_setxattr_make_nscap().  Does it make sense to call it here instead,
> > >> >> > before the security.capability test?  This would lay the foundation for
> > >> >> > doing something similar for IMA.
> > >> >> 
> > >> >> Might make sense to move that.  Though looking at it with fresh eyes I wonder
> > >> >> whether adding less code here at __vfs_setxattr_noperm(), i.e.
> > >> >> 
> > >> >> 		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
> > >> >> 			return -EPERM;
> > >> >> 
> > >> >> would be cleaner.
> > >> >
> > >> > Yes, it would be cleaner,  but I'm suggesting you do all the hard work
> > >> > making it generic.  Then the rest of us can follow your lead.  Its more
> > >> > likely that you'll get it right.  At a high level, it might look like:
> > >> >
> > >> >                /* Permit root in a non-init user_ns to modify the security
> > >> >                  * namespace xattr equivalents (eg. nscapability, ns_ima, etc). 
> > >> >                  */
> > >> >                 if ((current_user_ns() != &init_user_ns) &&
> > >> >                         capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) {
> > >> >
> > >> > 			if  security..capability
> > >> > 				call capability  /* set nscapability? */
> > >> >
> > >> > 			else if security.ima 
> > >> > 				call ima 	/* set ns_ima? */
> > >> > 		}
> > >> 
> > >> Hmm.  I am confused about this part of the strategy.
> > >> 
> > >> I don't understand the capability vs nscapability distinction.  It seems
> > >> to add complexity without benefit.
> > >
> > > ...  Well, yes, we could simply make a new version of security.capability
> > > xattr, and make rootid == 0 mean it was written by the init_user_ns.  Is
> > > that what you mean?
> > 
> > Yes.
> > 
> > That would seem to simplify the logic to ensure the policy we enforce is
> > consistent with what is on disk.
> 
> I'll give that a shot.  I think the reason I did it this way was that I'm
> still kind of stuck in the not-magic way of thinking about it.  But yeah
> with the kernel magically writing inthe kuid there's probably no reason not
> to.

Totally confused.  Will this method allow multiple instances of the
xattr on disk? 

Mimi

[toc] | [prev] | [next] | [standalone]

#1404771 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> On Fri, 2016-05-20 at 14:59 -0500, Serge E. Hallyn wrote:
> > Quoting Eric W. Biederman (ebiederm@xmission.com):
> > > "Serge E. Hallyn" <serge@hallyn.com> writes:
> > > 
> > > > Quoting Eric W. Biederman (ebiederm@xmission.com):
> > > >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> > > >> 
> > > >> > On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
> > > >> >> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > >> >> > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
> > > >> >
> > > >> >> > > diff --git a/fs/xattr.c b/fs/xattr.c
> > > >> >> > > index 4861322..5c0e7ae 100644
> > > >> >> > > --- a/fs/xattr.c
> > > >> >> > > +++ b/fs/xattr.c
> > > >> >> > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
> > > >> >> > >  {
> > > >> >> > >  	struct inode *inode = dentry->d_inode;
> > > >> >> > >  	int error = -EOPNOTSUPP;
> > > >> >> > > +	void *wvalue = NULL;
> > > >> >> > > +	size_t wsize = 0;
> > > >> >> > >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
> > > >> >> > >  				   XATTR_SECURITY_PREFIX_LEN);
> > > >> >> > > 
> > > >> >> > > -	if (issec)
> > > >> >> > > +	if (issec) {
> > > >> >> > >  		inode->i_flags &= ~S_NOSEC;
> > > >> >> > > +		/* if root in a non-init user_ns tries to set
> > > >> >> > > +		 * security.capability, write a security.nscapability
> > > >> >> > > +		 * in its place */
> > > >> >> > > +		if (!strcmp(name, "security.capability") &&
> > > >> >> > > +				current_user_ns() != &init_user_ns) {
> > > >> >> > > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
> > > >> >> > > +			if (!wvalue)
> > > >> >> > > +				return -EPERM;
> > > >> >> > > +			value = wvalue;
> > > >> >> > > +			size = wsize;
> > > >> >> > > +			name = "security.nscapability";
> > > >> >> > > +		}
> > > >> >> > 
> > > >> >> > The call to capable_wrt_inode_uidgid() is hidden behind
> > > >> >> > cap_setxattr_make_nscap().  Does it make sense to call it here instead,
> > > >> >> > before the security.capability test?  This would lay the foundation for
> > > >> >> > doing something similar for IMA.
> > > >> >> 
> > > >> >> Might make sense to move that.  Though looking at it with fresh eyes I wonder
> > > >> >> whether adding less code here at __vfs_setxattr_noperm(), i.e.
> > > >> >> 
> > > >> >> 		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
> > > >> >> 			return -EPERM;
> > > >> >> 
> > > >> >> would be cleaner.
> > > >> >
> > > >> > Yes, it would be cleaner,  but I'm suggesting you do all the hard work
> > > >> > making it generic.  Then the rest of us can follow your lead.  Its more
> > > >> > likely that you'll get it right.  At a high level, it might look like:
> > > >> >
> > > >> >                /* Permit root in a non-init user_ns to modify the security
> > > >> >                  * namespace xattr equivalents (eg. nscapability, ns_ima, etc). 
> > > >> >                  */
> > > >> >                 if ((current_user_ns() != &init_user_ns) &&
> > > >> >                         capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) {
> > > >> >
> > > >> > 			if  security..capability
> > > >> > 				call capability  /* set nscapability? */
> > > >> >
> > > >> > 			else if security.ima 
> > > >> > 				call ima 	/* set ns_ima? */
> > > >> > 		}
> > > >> 
> > > >> Hmm.  I am confused about this part of the strategy.
> > > >> 
> > > >> I don't understand the capability vs nscapability distinction.  It seems
> > > >> to add complexity without benefit.
> > > >
> > > > ...  Well, yes, we could simply make a new version of security.capability
> > > > xattr, and make rootid == 0 mean it was written by the init_user_ns.  Is
> > > > that what you mean?
> > > 
> > > Yes.
> > > 
> > > That would seem to simplify the logic to ensure the policy we enforce is
> > > consistent with what is on disk.
> > 
> > I'll give that a shot.  I think the reason I did it this way was that I'm
> > still kind of stuck in the not-magic way of thinking about it.  But yeah
> > with the kernel magically writing inthe kuid there's probably no reason not
> > to.
> 
> Totally confused.  Will this method allow multiple instances of the
> xattr on disk? 

No, but we don't actually want that anyway.  The current behavior for
security.capability is that it works in all user namespaces.  So we
want to continue the behavior that if root in the init_user_ns sets a
capability, that works in all namespaces.  Allowing other namespaces
to set the capability would only be confusing.

So in the patchset I had, security.capability can only be set by
init_user_ns but works in all namespaces.  security.nscapability
cannot be set if secrity.capability is set.  And security.nscapability
works in all child namespaces of the root uid which set the cap.

-serge

[toc] | [prev] | [next] | [standalone]

#1404705 — Re: [PATCH RFC] user-namespaced file capabilities - now with more magic

"Serge E. Hallyn" <serge@hallyn.com> writes:

> Quoting Eric W. Biederman (ebiederm@xmission.com):
>> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> 
>> > On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote:
>> >> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
>> >> > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote:
>> >
>> >> > > diff --git a/fs/xattr.c b/fs/xattr.c
>> >> > > index 4861322..5c0e7ae 100644
>> >> > > --- a/fs/xattr.c
>> >> > > +++ b/fs/xattr.c
>> >> > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
>> >> > >  {
>> >> > >  	struct inode *inode = dentry->d_inode;
>> >> > >  	int error = -EOPNOTSUPP;
>> >> > > +	void *wvalue = NULL;
>> >> > > +	size_t wsize = 0;
>> >> > >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
>> >> > >  				   XATTR_SECURITY_PREFIX_LEN);
>> >> > > 
>> >> > > -	if (issec)
>> >> > > +	if (issec) {
>> >> > >  		inode->i_flags &= ~S_NOSEC;
>> >> > > +		/* if root in a non-init user_ns tries to set
>> >> > > +		 * security.capability, write a security.nscapability
>> >> > > +		 * in its place */
>> >> > > +		if (!strcmp(name, "security.capability") &&
>> >> > > +				current_user_ns() != &init_user_ns) {
>> >> > > +			cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize);
>> >> > > +			if (!wvalue)
>> >> > > +				return -EPERM;
>> >> > > +			value = wvalue;
>> >> > > +			size = wsize;
>> >> > > +			name = "security.nscapability";
>> >> > > +		}
>> >> > 
>> >> > The call to capable_wrt_inode_uidgid() is hidden behind
>> >> > cap_setxattr_make_nscap().  Does it make sense to call it here instead,
>> >> > before the security.capability test?  This would lay the foundation for
>> >> > doing something similar for IMA.
>> >> 
>> >> Might make sense to move that.  Though looking at it with fresh eyes I wonder
>> >> whether adding less code here at __vfs_setxattr_noperm(), i.e.
>> >> 
>> >> 		if (!cap_setxattr_makenscap(dentry, &value, &size, &name))
>> >> 			return -EPERM;
>> >> 
>> >> would be cleaner.
>> >
>> > Yes, it would be cleaner,  but I'm suggesting you do all the hard work
>> > making it generic.  Then the rest of us can follow your lead.  Its more
>> > likely that you'll get it right.  At a high level, it might look like:
>> >
>> >                /* Permit root in a non-init user_ns to modify the security
>> >                  * namespace xattr equivalents (eg. nscapability, ns_ima, etc). 
>> >                  */
>> >                 if ((current_user_ns() != &init_user_ns) &&
>> >                         capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) {
>> >
>> > 			if  security..capability
>> > 				call capability  /* set nscapability? */
>> >
>> > 			else if security.ima 
>> > 				call ima 	/* set ns_ima? */
>> > 		}
>> 
>> Hmm.  I am confused about this part of the strategy.
>> 
>> I don't understand the capability vs nscapability distinction.  It seems
>> to add complexity without benefit.
>
> ...  Well, yes, we could simply make a new version of security.capability
> xattr, and make rootid == 0 mean it was written by the init_user_ns.  Is
> that what you mean?

Yes.

That would seem to simplify the logic to ensure the policy we enforce is
consistent with what is on disk.

Eric

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web