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

Groups > linux.kernel > #1672964 > unrolled thread

[PATCH 0/3] Enable namespaced file capabilities

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] Enable namespaced file capabilities Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-06-22 21:10 +0200
    [PATCH 1/3] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-06-22 21:10 +0200
      [PATCH] xattr: fix kstrdup.cocci warnings kbuild test robot <lkp@intel.com> - 2017-06-24 23:10 +0200
      Re: [PATCH 1/3] xattr: Enable security.capability in user namespaces kbuild test robot <lkp@intel.com> - 2017-06-24 23:10 +0200
    Re: [PATCH 0/3] Enable namespaced file capabilities Casey Schaufler <casey@schaufler-ca.com> - 2017-06-22 22:00 +0200
      Re: [PATCH 0/3] Enable namespaced file capabilities Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-06-22 22:20 +0200
        Re: [PATCH 0/3] Enable namespaced file capabilities Casey Schaufler <casey@schaufler-ca.com> - 2017-06-22 22:40 +0200
          Re: [PATCH 0/3] Enable namespaced file capabilities Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-06-22 23:10 +0200
          Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-22 23:10 +0200
            Re: [PATCH 0/3] Enable namespaced file capabilities Casey Schaufler <casey@schaufler-ca.com> - 2017-06-23 00:50 +0200
              Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 01:10 +0200
    Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 01:40 +0200
      Re: [PATCH 0/3] Enable namespaced file capabilities James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-06-23 02:20 +0200
        Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 03:30 +0200
        Re: [PATCH 0/3] Enable namespaced file capabilities ebiederm@xmission.com (Eric W. Biederman) - 2017-06-23 19:50 +0200
          Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 20:40 +0200
    Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 01:40 +0200
    Re: [PATCH 0/3] Enable namespaced file capabilities James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-06-23 01:40 +0200
    Re: [PATCH 0/3] Enable namespaced file capabilities Amir Goldstein <amir73il@gmail.com> - 2017-06-23 09:10 +0200
      Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 18:10 +0200
        Re: [PATCH 0/3] Enable namespaced file capabilities Casey Schaufler <casey@schaufler-ca.com> - 2017-06-23 18:20 +0200
          Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 18:40 +0200
            Re: [PATCH 0/3] Enable namespaced file capabilities Casey Schaufler <casey@schaufler-ca.com> - 2017-06-23 19:00 +0200
              Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 19:10 +0200
                Re: [PATCH 0/3] Enable namespaced file capabilities ebiederm@xmission.com (Eric W. Biederman) - 2017-06-23 20:00 +0200
                  Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 20:40 +0200
            Re: [PATCH 0/3] Enable namespaced file capabilities James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-06-23 19:10 +0200
              Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 19:30 +0200
              Re: [PATCH 0/3] Enable namespaced file capabilities Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-06-23 19:40 +0200
                Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 20:40 +0200
          Re: [PATCH 0/3] Enable namespaced file capabilities Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-06-23 20:10 +0200
            Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 20:40 +0200
              Re: [PATCH 0/3] Enable namespaced file capabilities Casey Schaufler <casey@schaufler-ca.com> - 2017-06-23 22:40 +0200
              Re: [PATCH 0/3] Enable namespaced file capabilities Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-06-24 01:10 +0200
                Re: [PATCH 0/3] Enable namespaced file capabilities Casey Schaufler <casey@schaufler-ca.com> - 2017-06-24 02:00 +0200
      Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-28 07:50 +0200
        Re: [PATCH 0/3] Enable namespaced file capabilities Amir Goldstein <amir73il@gmail.com> - 2017-06-28 09:20 +0200
          Re: [PATCH 0/3] Enable namespaced file capabilities Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-06-28 16:10 +0200
          Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-28 16:30 +0200
    Re: [PATCH 0/3] Enable namespaced file capabilities Vivek Goyal <vgoyal@redhat.com> - 2017-06-23 22:20 +0200
      Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 22:20 +0200
        Re: [PATCH 0/3] Enable namespaced file capabilities Vivek Goyal <vgoyal@redhat.com> - 2017-06-23 22:40 +0200
          Re: [PATCH 0/3] Enable namespaced file capabilities "Serge E. Hallyn" <serge@hallyn.com> - 2017-06-23 23:00 +0200

Page 1 of 3  [1] 2 3  Next page →

#1672964 — [PATCH 0/3] Enable namespaced file capabilities

This series of patches primary goal is to enable file capabilities
in user namespaces without affecting the file capabilities that are
effective on the host. This is to prevent that any unprivileged user
on the host maps his own uid to root in a private namespace, writes
the xattr, and executes the file with privilege on the host.

We achieve this goal by writing extended attributes with a different
name when a user namespace is used. If for example the root user
in a user namespace writes the security.capability xattr, the name
of the xattr that is actually written is encoded as
security.capability@uid=1000 for root mapped to uid 1000 on the host.
When listing the xattrs on the host, the existing security.capability
as well as the security.capability@uid=1000 will be shown. Inside the
namespace only 'security.capability', with the value of
security.capability@uid=1000, is visible.

To maintain compatibility with existing behavior, the value of
security.capability of the host is shown inside the user namespace
once the security.capability of the user namespace has been removed
(which really removes security.capability@uid=1000). Writing to
an extended attribute inside a user namespace effectively hides the
extended attribute of the host.

The general framework that is established with these patches can
be applied to other extended attributes as well, such as security.ima
or the 'trusted.' prefix . Another extended attribute that needed to
be enabled here is 'security.selinux,' since otherwise this extended
attribute would not be shown anymore inside a user namespace.

Regards,
   Stefan & Serge


Stefan Berger (3):
  xattr: Enable security.capability in user namespaces
  Enable capabilities of files from shared filesystem
  Enable security.selinux in user namespaces

 fs/xattr.c               | 472 ++++++++++++++++++++++++++++++++++++++++++++++-
 security/commoncap.c     |  36 +++-
 security/selinux/hooks.c |   9 +-
 3 files changed, 501 insertions(+), 16 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1672971 — [PATCH 1/3] xattr: Enable security.capability in user namespaces

This patch enables security.capability in user namespaces but also
takes a more general approach to enabling extended attributes in user
namespaces.

The following rules describe the approach using security.foo as a
'user namespace enabled' extended attribute:

Reading of extended attributes:

1) Reading security.foo from a user namespace will read
   security.foo@uid=<uid> of the parent user namespace instead with uid
   being the mapping of root in that parent user namespace. An
   exception is if root is mapped to uid 0 on the host, and in this case
   we will read security.foo directly.
   --> reading security.foo will read security.foo@uid=1000 for uid
       mapping of root to 1000.

2) All security.foo@uid=<uid> with valid uid mapping in the user namespace
   can be read. The uid within the user namespace will be mapped to the
   corresponding uid on the host and that uid will be used in the name of
   the extended attribute.
   -> reading security.foo@uid=1 will read security.foo@uid=1001 for uid
      mapping of root to 1000, size of at least 2.

   All security.foo@uid=<uid> can be read (by root) on the host with values
   of <uid> also being subject to checking for valid mappings.

3) No other security.foo* can be read.

The same rules for reading apply to writing and removing of user
namespace enabled extended attributes.

When listing extended attributes of a file, only those are presented
to the user namespace that have a valid mapping. Besides that, names
of the extended attributes are adjusted to represent the mapping.
This means that if root is mapped to uid 1000 on the host, the
security.foo@uid=1000 will be listed as security.foo in the user
namespace, security.foo@uid=1001 becomes security.foo@uid=1 and so on.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 fs/xattr.c               | 433 ++++++++++++++++++++++++++++++++++++++++++++++-
 security/commoncap.c     |  36 ++--
 security/selinux/hooks.c |   9 +-
 3 files changed, 462 insertions(+), 16 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 464c94b..64c4b40 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -133,11 +133,405 @@ xattr_permission(struct inode *inode, const char *name, int mask)
 	return inode_permission(inode, mask);
 }
 
+/*
+ * A list of extended attributes that are supported in user namespaces
+ */
+static const char *const userns_xattrs[] = {
+	XATTR_NAME_CAPS,
+	NULL
+};
+
+/*
+ * xattrs_is_userns_supported - Check whether an xattr is supported in userns
+ *
+ * @name:   full name of the extended attribute
+ * @prefix: do a prefix match (true) or a full match (false)
+ *
+ * This function returns < 0 if not supported, an index into userns_xattrs[]
+ * otherwise.
+ */
+static int
+xattr_is_userns_supported(const char *name, int prefix)
+{
+	int i;
+
+	if (!name)
+		return -1;
+
+	for (i = 0; userns_xattrs[i]; i++) {
+		if (prefix) {
+			if (!strncmp(userns_xattrs[i], name,
+				     strlen(userns_xattrs[i])))
+				return i;
+		} else {
+			if (!strcmp(userns_xattrs[i], name))
+				return i;
+		}
+	}
+	return -1;
+}
+
+/*
+ * xattr_write_uid - print a string in the format of "%s@uid=%u", which
+ *                   includes a prefix strig
+ *
+ * @uid:     the uid
+ * @prefix:  prefix string; may be NULL
+ *
+ * This function returns a buffer with the string, or a NULL pointer in
+ * case of out-of-memory error.
+ */
+static char *
+xattr_write_uid(uid_t uid, const char *prefix)
+{
+	size_t buflen;
+	char *buffer;
+
+	buflen = sizeof("@uid=") - 1 + sizeof("4294967295") - 1 + 1;
+	if (prefix)
+		buflen += strlen(prefix);
+
+	buffer = kmalloc(buflen, GFP_KERNEL);
+	if (!buffer)
+		return NULL;
+
+	if (uid == 0)
+		*buffer = 0;
+	else
+		sprintf(buffer, "%s@uid=%u",
+			(prefix) ? prefix : "",
+			uid);
+
+	return buffer;
+}
+
+/*
+ * xattr_parse_uid_from_kuid - parse string in the format @uid=<uid>; consider
+ *                             user namespaces and check mappings
+ *
+ * @uidstr   : string in the format "@uid=<uid>"
+ * @userns   : the user namespace to consult for uid mappings
+ * @n_uidstr : returned pointer holding the rewritten @uid=<uid> string with
+ *             the uid remapped
+ *
+ * This function returns an error code or 0 in case of success. In case
+ * of success, 'n_uidstr' will hold a valid string.
+ */
+static int
+xattr_parse_uid_from_kuid(const char *uidstr, struct user_namespace *userns,
+			  char **n_uidstr)
+{
+	int n;
+	uid_t muid, p_uid;
+	char d;
+	kuid_t tuid;
+
+	*n_uidstr = NULL;
+
+	n = sscanf(uidstr, "@uid=%u%c", &p_uid, &d);
+	if (n != 1)
+		return -EINVAL;
+
+	/* do we have a mapping of the uid? */
+	tuid = KUIDT_INIT(p_uid);
+	muid = from_kuid(userns, tuid);
+	if (muid == -1)
+		return -ENOENT;
+
+	*n_uidstr = xattr_write_uid(muid, NULL);
+	if (!*n_uidstr)
+		return -ENOMEM;
+
+	return 0;
+}
+
+/*
+ * xattr_parse_uid_make_kuid - parse string in the format @uid=<uid>; consider
+ *                             user namespaces and check mappings
+ *
+ * @uidstr   : string in the format "@uid=<uid>"
+ * @userns   : the user namespace to consult for uid mappings
+ * @N_uidstr : returned pointer holding the rewritten @uid=<uid> string with
+ *             the uid remapped
+ *
+ * This function returns an error code or 0 in case of success. In case
+ * of success, 'n_uidstr' will hold a valid string.
+ */
+static int
+xattr_parse_uid_make_kuid(const char *uidstr, struct user_namespace *userns,
+			  char **n_uidstr)
+{
+	int n;
+	uid_t p_uid;
+	char d;
+	kuid_t tuid;
+
+	*n_uidstr = NULL;
+
+	n = sscanf(uidstr, "@uid=%u%c", &p_uid, &d);
+	if (n != 1)
+		return -EINVAL;
+
+	tuid = make_kuid(userns, p_uid);
+	if (!uid_valid(tuid))
+		return -ENOENT;
+
+	*n_uidstr = xattr_write_uid(__kuid_val(tuid), NULL);
+	if (!*n_uidstr)
+		return -ENOMEM;
+
+	return 0;
+}
+
+/*
+ * xattr_rewrite_userns_xattr - Rewrite and filter an extended attribute
+ *                              considering user namespace uid mappings and
+ *                              user namespace support extended attributes
+ *
+ * @name: full name of the extended attribute
+ *
+ * This function returns NULL if the name is to be filtered. Otherwise it can
+ * return the input buffer or a new buffer that the caller needs to free. The
+ * new buffer contains a rewritten extended attribute whose string length may
+ * exceed that of the given name.
+ */
+static char *
+xattr_rewrite_userns_xattr(char *name)
+{
+	int idx, error;
+	size_t len = 0, buflen;
+	char *buffer, *n_uidstr;
+
+	/* prefix-match name against supported attributes */
+	idx = xattr_is_userns_supported(name, true);
+	if (idx < 0)
+		return NULL;
+
+	/* exact match ? */
+	len = strlen(userns_xattrs[idx]);
+	if (name[len] == 0)
+		return NULL;
+
+	/*
+	 * We must have a name[len] == '@'.
+	 */
+	error = xattr_parse_uid_from_kuid(&name[len], current_user_ns(),
+					  &n_uidstr);
+	if (error)
+		return NULL;
+
+	buflen = len + strlen(n_uidstr) + 1;
+	buffer = kmalloc(buflen, GFP_KERNEL);
+	if (!buffer) {
+		kfree(n_uidstr);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	name[len] = 0;
+
+	snprintf(buffer, buflen, "%s%s", name, n_uidstr);
+
+	name[len] = '@';
+
+	kfree(n_uidstr);
+
+	return buffer;
+}
+
+/*
+ * xattr_list_userns_rewrite - Rewrite list of xattr names for user namespaces
+ *                             or determine needed size for attribute list
+ *                             in case size == 0
+ *
+ * In a user namespace we do not present all extended attributes to the
+ * user. We filter out those that are in the list of userns supported xattr.
+ * Besides that we filter out those with @uid=<uid> when there is no mapping
+ * for that uid in the current user namespace.
+ *
+ * @list:        list of 0-byte separated xattr names
+ * @size:        the size of the list; may be 0 to determine needed list size
+ * @list_maxlen: allocated buffer size of list
+ */
+static ssize_t
+xattr_list_userns_rewrite(char *list, ssize_t size, size_t list_maxlen)
+{
+	char *nlist = NULL;
+	size_t s_off, len, nlen;
+	ssize_t d_off;
+	char *name, *newname;
+
+	if (!list || size < 0 || current_user_ns() == &init_user_ns)
+		return size;
+
+	if (size) {
+		nlist = kmalloc(list_maxlen, GFP_KERNEL);
+		if (!nlist)
+			return -ENOMEM;
+	}
+
+	s_off = d_off = 0;
+	while (s_off < size || size == 0) {
+		name = &list[s_off];
+
+		len = strlen(name);
+		if (!len)
+			break;
+
+		newname = xattr_rewrite_userns_xattr(name);
+		if (IS_ERR(newname)) {
+			d_off = PTR_ERR(newname);
+			goto out_free;
+		}
+		if (newname) {
+			nlen = strlen(newname);
+
+			if (nlist) {
+				if (nlen + 1 > list_maxlen)
+					break;
+				strcpy(&nlist[d_off], newname);
+			}
+
+			d_off += nlen + 1;
+			if (newname != name)
+				kfree(newname);
+		}
+		s_off += len + 1;
+	}
+	if (nlist)
+		memcpy(list, nlist, d_off);
+out_free:
+	kfree(nlist);
+
+	return d_off;
+}
+
+/*
+ * xattr_userns_name - modify the name of a user namespace supported
+ *                     extended attribute
+ *
+ * In a user namespace we prevent read/write accesses to the host's
+ * security.foo to protect these extended attributes.
+ *
+ * Reading:
+ * 1) Reading security.foo from a user namespace will read
+ *    security.foo@uid=<uid> of the parent user namespace instead with uid
+ *    being the mapping of root in that parent user namespace. An
+ *    exception is if root is mapped to uid 0 on the host, and in this case
+ *    we will read security.foo directly.
+ *    -> reading security.foo will read security.foo@uid=1000 for a uid
+ *       mapping of root to 1000.
+ *
+ * 2) All security.foo@uid=<uid> with valid uid mappings in the user namespace
+ *    an be read. The uid within the user namespace will be mapped to the
+ *    corresponding uid on the host and that uid will be used in the name of
+ *    the extended attribute.
+ *    -> reading security.foo@uid=1 will read security.foo@uid=1001 for a uid
+ *       mapping of root to 1000, size of at least 2.
+ *
+ *    All security.foo@uid=<uid> can be read (by root) on the host with values
+ *    of <uid> also being subject to checking for valid mappings.
+ *
+ * 3) No other security.foo* can be read.
+ *
+ * Writing and removing:
+ * The same rules for reading apply to writing and removing.
+ *
+ * This function returns a buffer with either the original name or the
+ * user namespace adjusted name of the extended attribute.
+ *
+ * @fullname: the full name of the extended attribute, e.g. security.foo
+ * @suffix:   the suffix of the extended attribute, e.g. foo
+ * @is_write: whether this is for writing an xattr
+ */
+char *
+xattr_userns_name(const char *fullname, const char *suffix)
+{
+	size_t buflen;
+	char *buffer, *ptr, *n_uidstr;
+	kuid_t root_uid = make_kuid(current_user_ns(), 0);
+	int idx, error;
+	size_t len = 0, slen;
+
+	if (!suffix)
+		return ERR_PTR(-EINVAL);
+
+	/* only security.foo will be changed here - prefix match here */
+	idx = xattr_is_userns_supported(fullname, true);
+	if (idx == -1)
+		goto out_copy;
+
+	/* read security.foo? --> read security.foo@uid=<uid> instead */
+	len = strlen(userns_xattrs[idx]);
+	if (fullname[len] == 0) {
+		/*
+		 * init_user_ns or userns with root mapped to uid 0
+		 * may read security.foo directly
+		 */
+		if (current_user_ns() == &init_user_ns ||
+		    __kuid_val(root_uid) == 0)
+			goto out_copy;
+
+		if (!uid_valid(root_uid))
+			return ERR_PTR(-EINVAL);
+
+		buffer = xattr_write_uid(__kuid_val(root_uid), suffix);
+		if (!buffer)
+			return ERR_PTR(-ENOMEM);
+
+		return buffer;
+	}
+
+	/*
+	 * We must have fullname[len] == '@'.
+	 */
+	error = xattr_parse_uid_make_kuid(&fullname[len],
+					  current_user_ns(),
+					  &n_uidstr);
+	if (error)
+		return ERR_PTR(error);
+
+	/* suffix of fullname must have '@' */
+	ptr = strchr(suffix, '@');
+	if (!ptr) {
+		kfree(n_uidstr);
+		goto err_eperm;
+	}
+	slen = ptr - suffix;
+
+	/* suffix[slen] = '@' */
+	buflen = strlen(suffix) + strlen(n_uidstr) + 1;
+	buffer = kmalloc(buflen, GFP_KERNEL);
+	if (!buffer) {
+		kfree(n_uidstr);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	snprintf(buffer, slen + 1, "%s", suffix);
+	snprintf(&buffer[slen], buflen - slen, "%s", n_uidstr);
+	kfree(n_uidstr);
+
+	return buffer;
+
+out_copy:
+	buffer = kmalloc(strlen(suffix) + 1, GFP_KERNEL);
+	if (!buffer)
+		return ERR_PTR(-ENOMEM);
+	strcpy(buffer, suffix);
+
+	return buffer;
+
+err_eperm:
+	return ERR_PTR(-EPERM);
+}
+
 int
 __vfs_setxattr(struct dentry *dentry, struct inode *inode, const char *name,
 	       const void *value, size_t size, int flags)
 {
 	const struct xattr_handler *handler;
+	char *nsuffix;
+	const char *fullname = name;
+	int ret;
 
 	handler = xattr_resolve_name(inode, &name);
 	if (IS_ERR(handler))
@@ -146,7 +540,12 @@ __vfs_setxattr(struct dentry *dentry, struct inode *inode, const char *name,
 		return -EOPNOTSUPP;
 	if (size == 0)
 		value = "";  /* empty EA, do not remove */
-	return handler->set(handler, dentry, inode, name, value, size, flags);
+	nsuffix = xattr_userns_name(fullname, name);
+	if (IS_ERR(nsuffix))
+		return PTR_ERR(nsuffix);
+	ret = handler->set(handler, dentry, inode, nsuffix, value, size, flags);
+	kfree(nsuffix);
+	return ret;
 }
 EXPORT_SYMBOL(__vfs_setxattr);
 
@@ -302,13 +701,21 @@ __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char *name,
 	       void *value, size_t size)
 {
 	const struct xattr_handler *handler;
+	char *nsuffix;
+	const char *fullname = name;
+	int ret;
 
 	handler = xattr_resolve_name(inode, &name);
 	if (IS_ERR(handler))
 		return PTR_ERR(handler);
 	if (!handler->get)
 		return -EOPNOTSUPP;
-	return handler->get(handler, dentry, inode, name, value, size);
+	nsuffix = xattr_userns_name(fullname, name);
+	if (IS_ERR(nsuffix))
+		return PTR_ERR(nsuffix);
+	ret = handler->get(handler, dentry, inode, nsuffix, value, size);
+	kfree(nsuffix);
+	return ret;
 }
 EXPORT_SYMBOL(__vfs_getxattr);
 
@@ -328,8 +735,14 @@ vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
 
 	if (!strncmp(name, XATTR_SECURITY_PREFIX,
 				XATTR_SECURITY_PREFIX_LEN)) {
+		int ret;
 		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
-		int ret = xattr_getsecurity(inode, suffix, value, size);
+		char *nsuffix = xattr_userns_name(name, suffix);
+
+		if (IS_ERR(nsuffix))
+			return PTR_ERR(nsuffix);
+		ret = xattr_getsecurity(inode, nsuffix, value, size);
+		kfree(nsuffix);
 		/*
 		 * Only overwrite the return value if a security module
 		 * is actually active.
@@ -360,6 +773,9 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size)
 		if (size && error > size)
 			error = -ERANGE;
 	}
+	if (error > 0)
+		error = xattr_list_userns_rewrite(list, error, size);
+
 	return error;
 }
 EXPORT_SYMBOL_GPL(vfs_listxattr);
@@ -369,13 +785,22 @@ __vfs_removexattr(struct dentry *dentry, const char *name)
 {
 	struct inode *inode = d_inode(dentry);
 	const struct xattr_handler *handler;
+	char *nsuffix;
+	const char *fullname = name;
+	int ret;
 
 	handler = xattr_resolve_name(inode, &name);
 	if (IS_ERR(handler))
 		return PTR_ERR(handler);
 	if (!handler->set)
 		return -EOPNOTSUPP;
-	return handler->set(handler, dentry, inode, name, NULL, 0, XATTR_REPLACE);
+	nsuffix = xattr_userns_name(fullname, name);
+	if (IS_ERR(nsuffix))
+		return PTR_ERR(nsuffix);
+	ret = handler->set(handler, dentry, inode, nsuffix, NULL, 0,
+			   XATTR_REPLACE);
+	kfree(nsuffix);
+	return ret;
 }
 EXPORT_SYMBOL(__vfs_removexattr);
 
diff --git a/security/commoncap.c b/security/commoncap.c
index 7abebd7..c842690 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -660,15 +660,23 @@ int cap_bprm_secureexec(struct linux_binprm *bprm)
 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 (!capable(CAP_SETFCAP))
+	if (strncmp(name, XATTR_SECURITY_PREFIX,
+		    sizeof(XATTR_SECURITY_PREFIX) - 1) != 0)
+		return 0;
+
+	if (strncmp(name, XATTR_NAME_CAPS,
+		    sizeof(XATTR_NAME_CAPS) - 1) == 0) {
+		struct inode *inode = d_backing_inode(dentry);
+
+		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))
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return 0;
 }
@@ -686,15 +694,23 @@ int cap_inode_setxattr(struct dentry *dentry, const char *name,
  */
 int cap_inode_removexattr(struct dentry *dentry, const char *name)
 {
-	if (!strcmp(name, XATTR_NAME_CAPS)) {
-		if (!capable(CAP_SETFCAP))
+	if (strncmp(name, XATTR_SECURITY_PREFIX,
+		    sizeof(XATTR_SECURITY_PREFIX) - 1) != 0)
+		return 0;
+
+	if (strncmp(name, XATTR_NAME_CAPS,
+		    sizeof(XATTR_NAME_CAPS) - 1) == 0) {
+		struct inode *inode = d_backing_inode(dentry);
+
+		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))
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return 0;
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 819fd68..702c225 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3091,8 +3091,13 @@ static int selinux_inode_setotherxattr(struct dentry *dentry, const char *name)
 
 	if (!strncmp(name, XATTR_SECURITY_PREFIX,
 		     sizeof XATTR_SECURITY_PREFIX - 1)) {
-		if (!strcmp(name, XATTR_NAME_CAPS)) {
-			if (!capable(CAP_SETFCAP))
+		if (!strncmp(name, XATTR_NAME_CAPS,
+			     sizeof(XATTR_NAME_CAPS) - 1)) {
+			struct inode *inode = d_backing_inode(dentry);
+
+			if (!inode)
+				return -EINVAL;
+			if (!capable_wrt_inode_uidgid(inode, CAP_SETFCAP))
 				return -EPERM;
 		} else if (!capable(CAP_SYS_ADMIN)) {
 			/* A different attribute in the security namespace.
-- 
2.7.4

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

#1674146 — [PATCH] xattr: fix kstrdup.cocci warnings

fs/xattr.c:516:10-17: WARNING opportunity for kstrdep (strcpy on line 519)

 Use kstrdup rather than duplicating its implementation

Generated by: scripts/coccinelle/api/kstrdup.cocci

CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 xattr.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -513,10 +513,9 @@ xattr_userns_name(const char *fullname,
 	return buffer;
 
 out_copy:
-	buffer = kmalloc(strlen(suffix) + 1, GFP_KERNEL);
+	buffer = kstrdup(suffix, GFP_KERNEL);
 	if (!buffer)
 		return ERR_PTR(-ENOMEM);
-	strcpy(buffer, suffix);
 
 	return buffer;
 

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

#1674147 — Re: [PATCH 1/3] xattr: Enable security.capability in user namespaces

Hi Stefan,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.12-rc6 next-20170623]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Stefan-Berger/Enable-namespaced-file-capabilities/20170625-001722


coccinelle warnings: (new ones prefixed by >>)

>> fs/xattr.c:516:10-17: WARNING opportunity for kstrdep (strcpy on line 519)

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

#1673013

On 6/22/2017 11:59 AM, Stefan Berger wrote:
> This series of patches primary goal is to enable file capabilities
> in user namespaces without affecting the file capabilities that are
> effective on the host. This is to prevent that any unprivileged user
> on the host maps his own uid to root in a private namespace, writes
> the xattr, and executes the file with privilege on the host.
>
> We achieve this goal by writing extended attributes with a different
> name when a user namespace is used. If for example the root user
> in a user namespace writes the security.capability xattr, the name
> of the xattr that is actually written is encoded as
> security.capability@uid=1000 for root mapped to uid 1000 on the host.

You need to identify the instance of the user namespace for
this to work right on a system with multiple user namespaces.
If I have a shared filesystem mounted in two different user
namespaces a change by one will affect the other.

... unless I'm missing something obvious about namespace behavior.

> When listing the xattrs on the host, the existing security.capability
> as well as the security.capability@uid=1000 will be shown. Inside the
> namespace only 'security.capability', with the value of
> security.capability@uid=1000, is visible.
>
> To maintain compatibility with existing behavior, the value of
> security.capability of the host is shown inside the user namespace
> once the security.capability of the user namespace has been removed
> (which really removes security.capability@uid=1000). Writing to
> an extended attribute inside a user namespace effectively hides the
> extended attribute of the host.
>
> The general framework that is established with these patches can
> be applied to other extended attributes as well, such as security.ima
> or the 'trusted.' prefix . Another extended attribute that needed to
> be enabled here is 'security.selinux,' since otherwise this extended
> attribute would not be shown anymore inside a user namespace.
>
> Regards,
>    Stefan & Serge
>
>
> Stefan Berger (3):
>   xattr: Enable security.capability in user namespaces
>   Enable capabilities of files from shared filesystem
>   Enable security.selinux in user namespaces
>
>  fs/xattr.c               | 472 ++++++++++++++++++++++++++++++++++++++++++++++-
>  security/commoncap.c     |  36 +++-
>  security/selinux/hooks.c |   9 +-
>  3 files changed, 501 insertions(+), 16 deletions(-)
>

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

#1673023

On 06/22/2017 03:59 PM, Casey Schaufler wrote:
> On 6/22/2017 11:59 AM, Stefan Berger wrote:
>> This series of patches primary goal is to enable file capabilities
>> in user namespaces without affecting the file capabilities that are
>> effective on the host. This is to prevent that any unprivileged user
>> on the host maps his own uid to root in a private namespace, writes
>> the xattr, and executes the file with privilege on the host.
>>
>> We achieve this goal by writing extended attributes with a different
>> name when a user namespace is used. If for example the root user
>> in a user namespace writes the security.capability xattr, the name
>> of the xattr that is actually written is encoded as
>> security.capability@uid=1000 for root mapped to uid 1000 on the host.
> You need to identify the instance of the user namespace for
> this to work right on a system with multiple user namespaces.
> If I have a shared filesystem mounted in two different user
> namespaces a change by one will affect the other.

Two different user namespaces with different uid mappings will not 
affect each other.

If root in userns1 mapped to uid 1000 (size 1000) writes 
security.capability, it will write security.capability@uid=1000 into the fs.
If root in userns2 mapped to uid 2000 (size 1000) writes 
security.capability, it will write security.capability@uid=2000 into the fs.

Neither of the two will see each other's security.capability, but each 
will see their own 'security.capability'.

Assume now userns1 has a size of 2000, so overlapping with userns2, it 
will now see userns2's security.capability@uid=1000 as well as its own 
'security.capability'. security.capability@uid=1000 (of userns2) in 
userns1 will not have an effect on effective file capabilities.

> ... unless I'm missing something obvious about namespace behavior.
>
>> When listing the xattrs on the host, the existing security.capability
>> as well as the security.capability@uid=1000 will be shown. Inside the
>> namespace only 'security.capability', with the value of
>> security.capability@uid=1000, is visible.
>>
>> To maintain compatibility with existing behavior, the value of
>> security.capability of the host is shown inside the user namespace
>> once the security.capability of the user namespace has been removed
>> (which really removes security.capability@uid=1000). Writing to
>> an extended attribute inside a user namespace effectively hides the
>> extended attribute of the host.
>>
>> The general framework that is established with these patches can
>> be applied to other extended attributes as well, such as security.ima
>> or the 'trusted.' prefix . Another extended attribute that needed to
>> be enabled here is 'security.selinux,' since otherwise this extended
>> attribute would not be shown anymore inside a user namespace.
>>
>> Regards,
>>     Stefan & Serge
>>
>>
>> Stefan Berger (3):
>>    xattr: Enable security.capability in user namespaces
>>    Enable capabilities of files from shared filesystem
>>    Enable security.selinux in user namespaces
>>
>>   fs/xattr.c               | 472 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   security/commoncap.c     |  36 +++-
>>   security/selinux/hooks.c |   9 +-
>>   3 files changed, 501 insertions(+), 16 deletions(-)
>>

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

#1673033

On 6/22/2017 1:12 PM, Stefan Berger wrote:
> On 06/22/2017 03:59 PM, Casey Schaufler wrote:
>> On 6/22/2017 11:59 AM, Stefan Berger wrote:
>>> This series of patches primary goal is to enable file capabilities
>>> in user namespaces without affecting the file capabilities that are
>>> effective on the host. This is to prevent that any unprivileged user
>>> on the host maps his own uid to root in a private namespace, writes
>>> the xattr, and executes the file with privilege on the host.
>>>
>>> We achieve this goal by writing extended attributes with a different
>>> name when a user namespace is used. If for example the root user
>>> in a user namespace writes the security.capability xattr, the name
>>> of the xattr that is actually written is encoded as
>>> security.capability@uid=1000 for root mapped to uid 1000 on the host.
>> You need to identify the instance of the user namespace for
>> this to work right on a system with multiple user namespaces.
>> If I have a shared filesystem mounted in two different user
>> namespaces a change by one will affect the other.
>
> Two different user namespaces with different uid mappings will not affect each other.

But two namespaces with the same uid mapping will, and I
don't think this meets the principle of least astonishment.
I also object to associating capabilities with UIDs. The
whole point of capabilities is to disassociate UID 0 from
privilege. What you've done is explicitly associate a UID
with the ability to have privilege. That's an architectural
regression.

>
> If root in userns1 mapped to uid 1000 (size 1000) writes security.capability, it will write security.capability@uid=1000 into the fs.
> If root in userns2 mapped to uid 2000 (size 1000) writes security.capability, it will write security.capability@uid=2000 into the fs.
>
> Neither of the two will see each other's security.capability, but each will see their own 'security.capability'.
>
> Assume now userns1 has a size of 2000, so overlapping with userns2, it will now see userns2's security.capability@uid=1000 as well as its own 'security.capability'. security.capability@uid=1000 (of userns2) in userns1 will not have an effect on effective file capabilities.
>
>> ... unless I'm missing something obvious about namespace behavior.
>>
>>> When listing the xattrs on the host, the existing security.capability
>>> as well as the security.capability@uid=1000 will be shown. Inside the
>>> namespace only 'security.capability', with the value of
>>> security.capability@uid=1000, is visible.
>>>
>>> To maintain compatibility with existing behavior, the value of
>>> security.capability of the host is shown inside the user namespace
>>> once the security.capability of the user namespace has been removed
>>> (which really removes security.capability@uid=1000). Writing to
>>> an extended attribute inside a user namespace effectively hides the
>>> extended attribute of the host.
>>>
>>> The general framework that is established with these patches can
>>> be applied to other extended attributes as well, such as security.ima
>>> or the 'trusted.' prefix . Another extended attribute that needed to
>>> be enabled here is 'security.selinux,' since otherwise this extended
>>> attribute would not be shown anymore inside a user namespace.
>>>
>>> Regards,
>>>     Stefan & Serge
>>>
>>>
>>> Stefan Berger (3):
>>>    xattr: Enable security.capability in user namespaces
>>>    Enable capabilities of files from shared filesystem
>>>    Enable security.selinux in user namespaces
>>>
>>>   fs/xattr.c               | 472 ++++++++++++++++++++++++++++++++++++++++++++++-
>>>   security/commoncap.c     |  36 +++-
>>>   security/selinux/hooks.c |   9 +-
>>>   3 files changed, 501 insertions(+), 16 deletions(-)
>>>
>
>

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

#1673043

On 06/22/2017 04:33 PM, Casey Schaufler wrote:
> On 6/22/2017 1:12 PM, Stefan Berger wrote:
>> On 06/22/2017 03:59 PM, Casey Schaufler wrote:
>>> On 6/22/2017 11:59 AM, Stefan Berger wrote:
>>>> This series of patches primary goal is to enable file capabilities
>>>> in user namespaces without affecting the file capabilities that are
>>>> effective on the host. This is to prevent that any unprivileged user
>>>> on the host maps his own uid to root in a private namespace, writes
>>>> the xattr, and executes the file with privilege on the host.
>>>>
>>>> We achieve this goal by writing extended attributes with a different
>>>> name when a user namespace is used. If for example the root user
>>>> in a user namespace writes the security.capability xattr, the name
>>>> of the xattr that is actually written is encoded as
>>>> security.capability@uid=1000 for root mapped to uid 1000 on the host.
>>> You need to identify the instance of the user namespace for
>>> this to work right on a system with multiple user namespaces.
>>> If I have a shared filesystem mounted in two different user
>>> namespaces a change by one will affect the other.
>> Two different user namespaces with different uid mappings will not affect each other.
> But two namespaces with the same uid mapping will, and I
> don't think this meets the principle of least astonishment.
> I also object to associating capabilities with UIDs. The
> whole point of capabilities is to disassociate UID 0 from
> privilege. What you've done is explicitly associate a UID
> with the ability to have privilege. That's an architectural
> regression.

It has privilege within the bounding set of the capabilities that it is 
given. Afaik, a process cannot gain additional capabilities through file 
capabilities. Allowing to set a process's file capabilities allows one 
to _restrict_ what it can do, which is useful for shared filesystems 
where I can now set my ping capabilities to cap_net_raw, overriding the 
ones one the host which could be cap_net_admin+cap_net_raw. So I don't 
need to extend my bounding set with cap_net_admin or mess with xattrs on 
the host.


>
>> If root in userns1 mapped to uid 1000 (size 1000) writes security.capability, it will write security.capability@uid=1000 into the fs.
>> If root in userns2 mapped to uid 2000 (size 1000) writes security.capability, it will write security.capability@uid=2000 into the fs.
>>
>> Neither of the two will see each other's security.capability, but each will see their own 'security.capability'.
>>
>> Assume now userns1 has a size of 2000, so overlapping with userns2, it will now see userns2's security.capability@uid=1000 as well as its own 'security.capability'. security.capability@uid=1000 (of userns2) in userns1 will not have an effect on effective file capabilities.
>>
>>> ... unless I'm missing something obvious about namespace behavior.
>>>
>>>> When listing the xattrs on the host, the existing security.capability
>>>> as well as the security.capability@uid=1000 will be shown. Inside the
>>>> namespace only 'security.capability', with the value of
>>>> security.capability@uid=1000, is visible.
>>>>
>>>> To maintain compatibility with existing behavior, the value of
>>>> security.capability of the host is shown inside the user namespace
>>>> once the security.capability of the user namespace has been removed
>>>> (which really removes security.capability@uid=1000). Writing to
>>>> an extended attribute inside a user namespace effectively hides the
>>>> extended attribute of the host.
>>>>
>>>> The general framework that is established with these patches can
>>>> be applied to other extended attributes as well, such as security.ima
>>>> or the 'trusted.' prefix . Another extended attribute that needed to
>>>> be enabled here is 'security.selinux,' since otherwise this extended
>>>> attribute would not be shown anymore inside a user namespace.
>>>>
>>>> Regards,
>>>>      Stefan & Serge
>>>>
>>>>
>>>> Stefan Berger (3):
>>>>     xattr: Enable security.capability in user namespaces
>>>>     Enable capabilities of files from shared filesystem
>>>>     Enable security.selinux in user namespaces
>>>>
>>>>    fs/xattr.c               | 472 ++++++++++++++++++++++++++++++++++++++++++++++-
>>>>    security/commoncap.c     |  36 +++-
>>>>    security/selinux/hooks.c |   9 +-
>>>>    3 files changed, 501 insertions(+), 16 deletions(-)
>>>>
>>

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

#1673045

Quoting Casey Schaufler (casey@schaufler-ca.com):
> On 6/22/2017 1:12 PM, Stefan Berger wrote:
> > On 06/22/2017 03:59 PM, Casey Schaufler wrote:
> >> On 6/22/2017 11:59 AM, Stefan Berger wrote:
> >>> This series of patches primary goal is to enable file capabilities
> >>> in user namespaces without affecting the file capabilities that are
> >>> effective on the host. This is to prevent that any unprivileged user
> >>> on the host maps his own uid to root in a private namespace, writes
> >>> the xattr, and executes the file with privilege on the host.
> >>>
> >>> We achieve this goal by writing extended attributes with a different
> >>> name when a user namespace is used. If for example the root user
> >>> in a user namespace writes the security.capability xattr, the name
> >>> of the xattr that is actually written is encoded as
> >>> security.capability@uid=1000 for root mapped to uid 1000 on the host.
> >> You need to identify the instance of the user namespace for
> >> this to work right on a system with multiple user namespaces.
> >> If I have a shared filesystem mounted in two different user
> >> namespaces a change by one will affect the other.
> >
> > Two different user namespaces with different uid mappings will not affect each other.
> 
> But two namespaces with the same uid mapping will, and I
> don't think this meets the principle of least astonishment.

It does.  If you have one filesystem shared among multiple
containers, then it needs to be either read-only, or you
need to know what you're doing.

> I also object to associating capabilities with UIDs. The
> whole point of capabilities is to disassociate UID 0 from
> privilege. What you've done is explicitly associate a UID
> with the ability to have privilege. That's an architectural
> regression.

IMO this is looking at it the wrong way.  From inside the container's
viewpoint, the capabilities are not associated with a uid.  Any
task, regardles off uid, in the container, which executes the file,
gets the privilege.  IMO that satisfies the intent of file capabilities.

-serge

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

#1673113

On 6/22/2017 2:09 PM, Serge E. Hallyn wrote:
> Quoting Casey Schaufler (casey@schaufler-ca.com):
>> On 6/22/2017 1:12 PM, Stefan Berger wrote:
>>> On 06/22/2017 03:59 PM, Casey Schaufler wrote:
>>>> On 6/22/2017 11:59 AM, Stefan Berger wrote:
>>>>> This series of patches primary goal is to enable file capabilities
>>>>> in user namespaces without affecting the file capabilities that are
>>>>> effective on the host. This is to prevent that any unprivileged user
>>>>> on the host maps his own uid to root in a private namespace, writes
>>>>> the xattr, and executes the file with privilege on the host.
>>>>>
>>>>> We achieve this goal by writing extended attributes with a different
>>>>> name when a user namespace is used. If for example the root user
>>>>> in a user namespace writes the security.capability xattr, the name
>>>>> of the xattr that is actually written is encoded as
>>>>> security.capability@uid=1000 for root mapped to uid 1000 on the host.
>>>> You need to identify the instance of the user namespace for
>>>> this to work right on a system with multiple user namespaces.
>>>> If I have a shared filesystem mounted in two different user
>>>> namespaces a change by one will affect the other.
>>> Two different user namespaces with different uid mappings will not affect each other.
>> But two namespaces with the same uid mapping will, and I
>> don't think this meets the principle of least astonishment.
> It does.  If you have one filesystem shared among multiple
> containers, then it needs to be either read-only, or you
> need to know what you're doing.

Joe's a junior devop who has been given a container
template which he tweaks for various nefarious purposes.
He doesn't know much about what he's doing. He isn't
changing the UIDs the template uses because, quite frankly,
he doesn't know a UID from an entrenching tool. He has
changed a filesystem from RO to RW because he read on a
forum somewhere that doing so would fix a problem he had
once. He doesn't want to have that problem again, so he
left the change in the template.

Containers are being sold as a way to make things easier.
This sort of side effect is dangerous in an environment
where users are being told that they don't have to worry
so much, the environment will take care of them. 

>> I also object to associating capabilities with UIDs. The
>> whole point of capabilities is to disassociate UID 0 from
>> privilege. What you've done is explicitly associate a UID
>> with the ability to have privilege. That's an architectural
>> regression.
> IMO this is looking at it the wrong way.

The right way to look at the problem is to identify the
capabilities the program ought to have and set the file
capabilities and UID/GID properly on the program on the
base system. If you have to fix the program so it works
right under those conditions, so much the better for
everyone. If you're running with different capabilities
in a container to prevent the program from doing damage
to the base system, maybe the program needs fixing instead.

> From inside the container's
> viewpoint, the capabilities are not associated with a uid.  Any
> task, regardles off uid, in the container, which executes the file,
> gets the privilege.  IMO that satisfies the intent of file capabilities.

The UID is the wrong association. The namespace is the correct association.
You're using the UID because it's something that's different in the
namespace than in the base system. You can detect it. What you need is a
non-volatile namespace id to attach to the file rather than using the
UID mapping (which may not be unique) that the namespace uses.

> -serge

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

#1673126

Quoting Casey Schaufler (casey@schaufler-ca.com):
> On 6/22/2017 2:09 PM, Serge E. Hallyn wrote:
> > Quoting Casey Schaufler (casey@schaufler-ca.com):
> >> On 6/22/2017 1:12 PM, Stefan Berger wrote:
> >>> On 06/22/2017 03:59 PM, Casey Schaufler wrote:
> >>>> On 6/22/2017 11:59 AM, Stefan Berger wrote:
> >>>>> This series of patches primary goal is to enable file capabilities
> >>>>> in user namespaces without affecting the file capabilities that are
> >>>>> effective on the host. This is to prevent that any unprivileged user
> >>>>> on the host maps his own uid to root in a private namespace, writes
> >>>>> the xattr, and executes the file with privilege on the host.
> >>>>>
> >>>>> We achieve this goal by writing extended attributes with a different
> >>>>> name when a user namespace is used. If for example the root user
> >>>>> in a user namespace writes the security.capability xattr, the name
> >>>>> of the xattr that is actually written is encoded as
> >>>>> security.capability@uid=1000 for root mapped to uid 1000 on the host.
> >>>> You need to identify the instance of the user namespace for
> >>>> this to work right on a system with multiple user namespaces.
> >>>> If I have a shared filesystem mounted in two different user
> >>>> namespaces a change by one will affect the other.
> >>> Two different user namespaces with different uid mappings will not affect each other.
> >> But two namespaces with the same uid mapping will, and I
> >> don't think this meets the principle of least astonishment.
> > It does.  If you have one filesystem shared among multiple
> > containers, then it needs to be either read-only, or you
> > need to know what you're doing.
> 
> Joe's a junior devop who has been given a container
> template which he tweaks for various nefarious purposes.
> He doesn't know much about what he's doing. He isn't
> changing the UIDs the template uses because, quite frankly,
> he doesn't know a UID from an entrenching tool. He has
> changed a filesystem from RO to RW because he read on a
> forum somewhere that doing so would fix a problem he had
> once. He doesn't want to have that problem again, so he
> left the change in the template.
> 
> Containers are being sold as a way to make things easier.
> This sort of side effect is dangerous in an environment
> where users are being told that they don't have to worry
> so much, the environment will take care of them. 
> 
> >> I also object to associating capabilities with UIDs. The
> >> whole point of capabilities is to disassociate UID 0 from
> >> privilege. What you've done is explicitly associate a UID
> >> with the ability to have privilege. That's an architectural
> >> regression.
> > IMO this is looking at it the wrong way.
> 
> The right way to look at the problem is to identify the
> capabilities the program ought to have and set the file
> capabilities and UID/GID properly on the program on the
> base system.

No.

Absolutely not.

That would require me to be given CAP_SETFCAP on the host in
order to control the resources I've been delegated in a user
namespace.  That's not how it works.

Using only /usr/bin/newuidmap and /usr/bin/newgidmap, which
allow me to map the subuids which I have been delegated through
/etc/subuid and /etc/subgid, I can, as an unprivileged user, and
with no other privilege, create a full container image, start it
up, and administer it.

The fact that I cannot also install software with file capabilities
is a shortcoming.

>  If you have to fix the program so it works
> right under those conditions, so much the better for
> everyone. If you're running with different capabilities
> in a container to prevent the program from doing damage
> to the base system, maybe the program needs fixing instead.

That is not the reason to do this.

Root in the container is assigning file capabilities for the
usual reason - to allow the file to be executed, by anyone,
regardless of uid (mapped into the namespace), with certain privilege.

The privilege which root in the container is allowed to
delegate is only the privilege which it *has* in the container.

If we allow root in the container to assign a 'global'
security.capability, then we are allow root in the container
to hand privilege to an unprivileged user on the host, against
host resources.

> > From inside the container's
> > viewpoint, the capabilities are not associated with a uid.  Any
> > task, regardles off uid, in the container, which executes the file,
> > gets the privilege.  IMO that satisfies the intent of file capabilities.
> 
> The UID is the wrong association. The namespace is the correct association.

That's a pleasant but impractical thought.  Namespaces do not have any
persistent ids.  (And if we tried, we'd be told no because it would
require a namespace of namespaces).

> You're using the UID because it's something that's different in the
> namespace than in the base system.

I'm using the uid because that is the subject which was granted privilege
over all other ids mapped into its user namespace.

>  You can detect it. What you need is a
> non-volatile namespace id to attach to the file rather than using the
> UID mapping (which may not be unique) that the namespace uses.

That's what we were trying to do in 2010.  It didn't work.  Which is
how we have the uid namespace as it exists.

-serge

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

#1673131

Quoting James Bottomley (James.Bottomley@HansenPartnership.com):
> On Thu, 2017-06-22 at 14:59 -0400, Stefan Berger wrote:
> > This series of patches primary goal is to enable file capabilities
> > in user namespaces without affecting the file capabilities that are
> > effective on the host. This is to prevent that any unprivileged user
> > on the host maps his own uid to root in a private namespace, writes
> > the xattr, and executes the file with privilege on the host.
> > 
> > We achieve this goal by writing extended attributes with a different
> > name when a user namespace is used. If for example the root user
> > in a user namespace writes the security.capability xattr, the name
> > of the xattr that is actually written is encoded as
> > security.capability@uid=1000 for root mapped to uid 1000 on the host.
> > When listing the xattrs on the host, the existing security.capability
> > as well as the security.capability@uid=1000 will be shown. Inside the
> > namespace only 'security.capability', with the value of
> > security.capability@uid=1000, is visible.
> 
> I'm a bit bothered by the @uid=1000 suffix.  What if I want to use this
> capability but am dynamically mapping the namespaces (i.e. I know I
> want unprivileged root, but I'm going to dynamically select the range
> to map based on what's currently available on the orchestration
> system).  If we stick with the @uid=X suffix, then dynamic mapping
> won't work because X is potentially different each time and there'll be
> a name mismatch in my xattrs.  Why not just make the suffix @uid, which
> means if root is mapped to any unprivileged uid then we pick this up
> otherwise we go with the unsuffixed property?
> 
> As far as I can see there's no real advantage to discriminating userns
> specific xattrs based on where root is mapped to, unless there's a use
> case I'm missing?

Yes, the use case is: to allow root in the container to set the
privilege itself, without endangering any resources not owned by
that root.  If you're going to have a root owned host-wide
orchestration system setting up the rootfs, then you don't
necessary need this at all.

As you say a @uid to say "any unprivileged userns" might be useful.
The implication is that root on the host doesn't trust the image
enough to write a real global file capability, but trusts it enough
to 'endanger' all containers on the host.  If that's the case, I have
no objection to adding this as a feature.

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

#1673146

On Thu, 2017-06-22 at 18:36 -0500, Serge E. Hallyn wrote:
> Quoting James Bottomley (James.Bottomley@HansenPartnership.com):
> > On Thu, 2017-06-22 at 14:59 -0400, Stefan Berger wrote:
> > > This series of patches primary goal is to enable file 
> > > capabilities in user namespaces without affecting the file 
> > > capabilities that are effective on the host. This is to prevent 
> > > that any unprivileged user on the host maps his own uid to root 
> > > in a private namespace, writes the xattr, and executes the file
> > > with privilege on the host.
> > > 
> > > We achieve this goal by writing extended attributes with a 
> > > different name when a user namespace is used. If for example the 
> > > root user in a user namespace writes the security.capability 
> > > xattr, the name of the xattr that is actually written is encoded 
> > > as security.capability@uid=1000 for root mapped to uid 1000 on 
> > > the host. When listing the xattrs on the host, the existing
> > > security.capability as well as the security.capability@uid=1000 
> > > will be shown. Inside the namespace only 'security.capability', 
> > > with the value of security.capability@uid=1000, is visible.
> > 
> > I'm a bit bothered by the @uid=1000 suffix.  What if I want to use 
> > this capability but am dynamically mapping the namespaces (i.e. I 
> > know I want unprivileged root, but I'm going to dynamically select 
> > the range to map based on what's currently available on the 
> > orchestration system).  If we stick with the @uid=X suffix, then 
> > dynamic mapping won't work because X is potentially different each 
> > time and there'll be a name mismatch in my xattrs.  Why not just 
> > make the suffix @uid, which means if root is mapped to any 
> > unprivileged uid then we pick this up otherwise we go with the
> > unsuffixed property?
> > 
> > As far as I can see there's no real advantage to discriminating 
> > userns specific xattrs based on where root is mapped to, unless 
> > there's a use case I'm missing?
> 
> Yes, the use case is: to allow root in the container to set the
> privilege itself, without endangering any resources not owned by
> that root.

OK, so you envisage the same filesystem being mounted in different user
namespaces and being able to see their own value for the xattr.  It
still seems a bit weird that they'd be able to change file contents and
have that seen by the other userns but not xattrs.

>   If you're going to have a root owned host-wide
> orchestration system setting up the rootfs, then you don't
> necessary need this at all.

I wasn't thinking it would be root owned, just that it would have a
predefined range of allowed uids and be able to map multiple containers
to subsets of these.

> As you say a @uid to say "any unprivileged userns" might be useful.
> The implication is that root on the host doesn't trust the image
> enough to write a real global file capability, but trusts it enough
> to 'endanger' all containers on the host.  If that's the case, I have
> no objection to adding this as a feature.

Yes, precisely.  The filesystem is certified as permitted to override
the xattr whatever unprivileged mapping for root is in place.

How would we effect the switch?  I suppose some global flag because I
can't see we'd be mixing use cases in a physical system.

James

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

#1673166

Quoting James Bottomley (James.Bottomley@HansenPartnership.com):
> On Thu, 2017-06-22 at 18:36 -0500, Serge E. Hallyn wrote:
> > Yes, the use case is: to allow root in the container to set the
> > privilege itself, without endangering any resources not owned by
> > that root.
> 
> OK, so you envisage the same filesystem being mounted in different user
> namespaces

Well no - in lxd we have a separate filesystem for each container.
The filesystems are not shared.

>  and being able to see their own value for the xattr.  It
> still seems a bit weird that they'd be able to change file contents and
> have that seen by the other userns but not xattrs.

Not sure what you mean.  If they have privilege over the inode, they
can write a xattr targeted at their own root userid.

> >   If you're going to have a root owned host-wide
> > orchestration system setting up the rootfs, then you don't
> > necessary need this at all.
> 
> I wasn't thinking it would be root owned, just that it would have a
> predefined range of allowed uids and be able to map multiple containers
> to subsets of these.

Hm. In that case they should not be allowed to write your proposed
'security.capability@uid' capability, because that would also grant
capabilities over subuids which they were not delegated.

(but see below)

> > As you say a @uid to say "any unprivileged userns" might be useful.
> > The implication is that root on the host doesn't trust the image
> > enough to write a real global file capability, but trusts it enough
> > to 'endanger' all containers on the host.  If that's the case, I have
> > no objection to adding this as a feature.
> 
> Yes, precisely.  The filesystem is certified as permitted to override
> the xattr whatever unprivileged mapping for root is in place.
> 
> How would we effect the switch?  I suppose some global flag because I
> can't see we'd be mixing use cases in a physical system.

I might be confused.  But thought CAP_SETFCAP against init_user_ns would
be required to set 'security.capability@uid'.  That, or you could create
a user namespace mapping [ 1 - 4294967295 ] to [ 0 = 4294967294 ], and
have CAP_SETFCAP against that namespace.  Which would allow you to run
without host root privilege.

-serge

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

#1673725

James Bottomley <James.Bottomley@HansenPartnership.com> writes:

> On Thu, 2017-06-22 at 18:36 -0500, Serge E. Hallyn wrote:
>> Quoting James Bottomley (James.Bottomley@HansenPartnership.com):
>> > On Thu, 2017-06-22 at 14:59 -0400, Stefan Berger wrote:
>> > > This series of patches primary goal is to enable file 
>> > > capabilities in user namespaces without affecting the file 
>> > > capabilities that are effective on the host. This is to prevent 
>> > > that any unprivileged user on the host maps his own uid to root 
>> > > in a private namespace, writes the xattr, and executes the file
>> > > with privilege on the host.
>> > > 
>> > > We achieve this goal by writing extended attributes with a 
>> > > different name when a user namespace is used. If for example the 
>> > > root user in a user namespace writes the security.capability 
>> > > xattr, the name of the xattr that is actually written is encoded 
>> > > as security.capability@uid=1000 for root mapped to uid 1000 on 
>> > > the host. When listing the xattrs on the host, the existing
>> > > security.capability as well as the security.capability@uid=1000 
>> > > will be shown. Inside the namespace only 'security.capability', 
>> > > with the value of security.capability@uid=1000, is visible.
>> > 
>> > I'm a bit bothered by the @uid=1000 suffix.  What if I want to use 
>> > this capability but am dynamically mapping the namespaces (i.e. I 
>> > know I want unprivileged root, but I'm going to dynamically select 
>> > the range to map based on what's currently available on the 
>> > orchestration system).  If we stick with the @uid=X suffix, then 
>> > dynamic mapping won't work because X is potentially different each 
>> > time and there'll be a name mismatch in my xattrs.  Why not just 
>> > make the suffix @uid, which means if root is mapped to any 
>> > unprivileged uid then we pick this up otherwise we go with the
>> > unsuffixed property?
>> > 
>> > As far as I can see there's no real advantage to discriminating 
>> > userns specific xattrs based on where root is mapped to, unless 
>> > there's a use case I'm missing?
>> 
>> Yes, the use case is: to allow root in the container to set the
>> privilege itself, without endangering any resources not owned by
>> that root.
>
> OK, so you envisage the same filesystem being mounted in different user
> namespaces and being able to see their own value for the xattr.  It
> still seems a bit weird that they'd be able to change file contents and
> have that seen by the other userns but not xattrs.

When you dynamically talk about selecting a range based what is
currently available in an orchestration system I don't know exactly what
you mean.  If it is something like what adduser does, assigning a
container a persistent association with uids and gids, that makes sense
to me.  If it is picking an association just for the lifetime of the
conainer processes it makes me nervous.

Fundamentally storage is persistent and writing data into it is
persistent.

Which means that when dealing with storage we need to make things safe
by default and not depend upon an assumption that the container tools
carefully keeps files separate from each other.

From previous conversations I am happy with and generally expect only a
capability xattr per file.

Even with one xattr of any type there is something appealing about
putting the logic that limits that xattr to a namespace in the name.  As
that is trivially backwards compatible.  As that does not require reving
the on disk file format based upon containers.

>> As you say a @uid to say "any unprivileged userns" might be useful.
>> The implication is that root on the host doesn't trust the image
>> enough to write a real global file capability, but trusts it enough
>> to 'endanger' all containers on the host.  If that's the case, I have
>> no objection to adding this as a feature.
>
> Yes, precisely.  The filesystem is certified as permitted to override
> the xattr whatever unprivileged mapping for root is in place.
>
> How would we effect the switch?  I suppose some global flag because I
> can't see we'd be mixing use cases in a physical system.

Mixing use cases in a filesystem almost always happens.  At least if we
are talking an ordinary multi-user system.  Multi-user systems are rarer
than they once were because machines are cheap, and security is hard,
but that should be what we are designing for.  Anything else is just
asking for trouble.

James when you talk about a global flag and mixing use cases in a
physical system it sounds a lot like you are talking about a base
filesystem for shiftfs.

My gut feel is that if this gets down to something like the shiftfs use
case.  I would assume either everything is shifted slightly so that all
uids are say shifted by 100,000 even the capability names of the
capability xattrs.  So that shiftfs or some part of the vfs would need
to shift the names of the xattrs as well.

Certainly I expect filesystems that are mounted with s_user_ns !=
&init_user_ns to be shifting the names of the security xattrs when
queried from &init_user_ns if we go with general design.

Eric

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

#1673764

Quoting Eric W. Biederman (ebiederm@xmission.com):
> Even with one xattr of any type there is something appealing about
> putting the logic that limits that xattr to a namespace in the name.  As

Exactly.  That's the idea - from Stefan - that I thought was a worthwhile
improvement over my own previous patch, which puts the logic in the value.
Most of the complaints raised so far about this patchset are just as valid (or
invalid) against my previous patch, but I was particularly interested in
thoughts on this approach versus mine.

thanks,
-serge

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

#1673133

Quoting James Bottomley (James.Bottomley@HansenPartnership.com):
> On Thu, 2017-06-22 at 14:59 -0400, Stefan Berger wrote:
> > This series of patches primary goal is to enable file capabilities
> > in user namespaces without affecting the file capabilities that are
> > effective on the host. This is to prevent that any unprivileged user
> > on the host maps his own uid to root in a private namespace, writes
> > the xattr, and executes the file with privilege on the host.
> > 
> > We achieve this goal by writing extended attributes with a different
> > name when a user namespace is used. If for example the root user
> > in a user namespace writes the security.capability xattr, the name
> > of the xattr that is actually written is encoded as
> > security.capability@uid=1000 for root mapped to uid 1000 on the host.
> > When listing the xattrs on the host, the existing security.capability
> > as well as the security.capability@uid=1000 will be shown. Inside the
> > namespace only 'security.capability', with the value of
> > security.capability@uid=1000, is visible.
> 
> I'm a bit bothered by the @uid=1000 suffix.  What if I want to use this
> capability but am dynamically mapping the namespaces (i.e. I know I
> want unprivileged root, but I'm going to dynamically select the range
> to map based on what's currently available on the orchestration
> system).  If we stick with the @uid=X suffix, then dynamic mapping
> won't work because X is potentially different each time and there'll be

Note that if you just set a 'security.capability' xattr, it will apply
to all namespaces.

Does that address your concern?

> a name mismatch in my xattrs.  Why not just make the suffix @uid, which
> means if root is mapped to any unprivileged uid then we pick this up
> otherwise we go with the unsuffixed property?
> 
> As far as I can see there's no real advantage to discriminating userns
> specific xattrs based on where root is mapped to, unless there's a use
> case I'm missing?
> 
> James
> 

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

#1673134

On Thu, 2017-06-22 at 14:59 -0400, Stefan Berger wrote:
> This series of patches primary goal is to enable file capabilities
> in user namespaces without affecting the file capabilities that are
> effective on the host. This is to prevent that any unprivileged user
> on the host maps his own uid to root in a private namespace, writes
> the xattr, and executes the file with privilege on the host.
> 
> We achieve this goal by writing extended attributes with a different
> name when a user namespace is used. If for example the root user
> in a user namespace writes the security.capability xattr, the name
> of the xattr that is actually written is encoded as
> security.capability@uid=1000 for root mapped to uid 1000 on the host.
> When listing the xattrs on the host, the existing security.capability
> as well as the security.capability@uid=1000 will be shown. Inside the
> namespace only 'security.capability', with the value of
> security.capability@uid=1000, is visible.

I'm a bit bothered by the @uid=1000 suffix.  What if I want to use this
capability but am dynamically mapping the namespaces (i.e. I know I
want unprivileged root, but I'm going to dynamically select the range
to map based on what's currently available on the orchestration
system).  If we stick with the @uid=X suffix, then dynamic mapping
won't work because X is potentially different each time and there'll be
a name mismatch in my xattrs.  Why not just make the suffix @uid, which
means if root is mapped to any unprivileged uid then we pick this up
otherwise we go with the unsuffixed property?

As far as I can see there's no real advantage to discriminating userns
specific xattrs based on where root is mapped to, unless there's a use
case I'm missing?

James

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

#1673304

On Thu, Jun 22, 2017 at 9:59 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> This series of patches primary goal is to enable file capabilities
> in user namespaces without affecting the file capabilities that are
> effective on the host. This is to prevent that any unprivileged user
> on the host maps his own uid to root in a private namespace, writes
> the xattr, and executes the file with privilege on the host.
>
> We achieve this goal by writing extended attributes with a different
> name when a user namespace is used. If for example the root user
> in a user namespace writes the security.capability xattr, the name
> of the xattr that is actually written is encoded as
> security.capability@uid=1000 for root mapped to uid 1000 on the host.
> When listing the xattrs on the host, the existing security.capability
> as well as the security.capability@uid=1000 will be shown. Inside the
> namespace only 'security.capability', with the value of
> security.capability@uid=1000, is visible.
>

Am I the only one who thinks that suffix is perhaps not the best grammar
to use for this namespace?
xattrs are clearly namespaced by prefix, so it seems right to me to keep
it that way - define a new special xattr namespace "ns" and only if that
prefix exists, the @uid suffix will be parsed.
This could be either  ns.security.capability@uid=1000 or
ns@uid=1000.security.capability. The latter seems more correct to me,
because then we will be able to namespace any xattr without having to
protect from "unprivileged xattr injection", i.e.:
setfattr -n "user.whatever.foo@uid=0"

Amir.

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

#1673659

Quoting Amir Goldstein (amir73il@gmail.com):
> On Thu, Jun 22, 2017 at 9:59 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
> > This series of patches primary goal is to enable file capabilities
> > in user namespaces without affecting the file capabilities that are
> > effective on the host. This is to prevent that any unprivileged user
> > on the host maps his own uid to root in a private namespace, writes
> > the xattr, and executes the file with privilege on the host.
> >
> > We achieve this goal by writing extended attributes with a different
> > name when a user namespace is used. If for example the root user
> > in a user namespace writes the security.capability xattr, the name
> > of the xattr that is actually written is encoded as
> > security.capability@uid=1000 for root mapped to uid 1000 on the host.
> > When listing the xattrs on the host, the existing security.capability
> > as well as the security.capability@uid=1000 will be shown. Inside the
> > namespace only 'security.capability', with the value of
> > security.capability@uid=1000, is visible.
> >
> 
> Am I the only one who thinks that suffix is perhaps not the best grammar
> to use for this namespace?

You're the only one to have mentioned it so far.

> xattrs are clearly namespaced by prefix, so it seems right to me to keep
> it that way - define a new special xattr namespace "ns" and only if that
> prefix exists, the @uid suffix will be parsed.
> This could be either  ns.security.capability@uid=1000 or
> ns@uid=1000.security.capability. The latter seems more correct to me,
> because then we will be able to namespace any xattr without having to
> protect from "unprivileged xattr injection", i.e.:
> setfattr -n "user.whatever.foo@uid=0"

I like it for simplifying the parser code.  One concern I have is that,
since ns.* is currently not gated, one could write ns.* on an older
kernel and then exploit it on a newer one.

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web