Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1685113 > unrolled thread
| Started by | Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> |
|---|---|
| First post | 2017-07-11 17:10 +0200 |
| Last post | 2017-07-17 23:00 +0200 |
| Articles | 20 on this page of 62 — 9 participants |
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.
[PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> - 2017-07-11 17:10 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-11 19:20 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-12 02:20 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-12 02:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-12 05:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-12 13:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-12 19:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces James Morris <jmorris@namei.org> - 2017-07-12 10:10 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-12 15:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-12 19:10 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces James Morris <jmorris@namei.org> - 2017-07-13 00:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 02:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 03:10 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 01:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-13 02:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Theodore Ts'o <tytso@mit.edu> - 2017-07-13 03:20 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 04:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 14:20 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Theodore Ts'o <tytso@mit.edu> - 2017-07-13 18:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-13 19:10 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 19:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Theodore Ts'o <tytso@mit.edu> - 2017-07-13 21:20 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 21:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 23:20 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces James Morris <jmorris@namei.org> - 2017-07-18 09:10 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 19:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-13 19:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 20:00 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 21:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 23:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-14 02:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-14 13:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-14 14:20 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-14 14:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-14 15:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-14 17:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-14 19:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-14 20:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-14 20:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-07-14 21:00 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-14 22:10 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-07-14 22:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Theodore Ts'o <tytso@mit.edu> - 2017-07-14 23:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-15 01:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-15 01:40 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-15 02:10 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Theodore Ts'o <tytso@mit.edu> - 2017-07-14 21:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-14 21:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-14 21:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-15 02:20 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-16 13:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-14 21:00 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-14 21:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 23:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 23:20 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 02:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Vivek Goyal <vgoyal@redhat.com> - 2017-07-12 20:00 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-12 21:20 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-15 01:50 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-15 23:30 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Vivek Goyal <vgoyal@redhat.com> - 2017-07-17 21:00 +0200
Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-17 23:00 +0200
Page 1 of 4 [1] 2 3 4 Next page →
| From | Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-07-11 17:10 +0200 |
| Subject | [PATCH v2] xattr: Enable security.capability in user namespaces |
| Message-ID | <u24Xv-3TD-1@gated-at.bofh.it> |
From: Stefan Berger <stefanb@linux.vnet.ibm.com>
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:
1a) 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.
1b) If security.foo@uid=<uid> is not available, the security.foo of the
parent namespace is tried to be read. This procedure is repeated up to
the init user namespace. This step only applies for reading of extended
attributes and provides the same behavior as older system where the
host's extended attributes applied to user namespaces.
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 | 509 +++++++++++++++++++++++++++++++++++++++++++++--
security/commoncap.c | 36 +++-
security/selinux/hooks.c | 9 +-
3 files changed, 523 insertions(+), 31 deletions(-)
diff --git a/fs/xattr.c b/fs/xattr.c
index 464c94b..eacad9e 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -133,20 +133,440 @@ 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 string
+ *
+ * @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) {
+ /* only rewrite those in userns_xattr[*] */
+ return name;
+ }
+
+ /* 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_contains - check whether an xattr list already contains a needle
+ *
+ * @list : 0-byte separated strings
+ * @listlen : length of the list
+ * @needle : the needle to search for
+ */
+static int
+xattr_list_contains(const char *list, size_t listlen, const char *needle)
+{
+ size_t o = 0;
+
+ while (o < listlen) {
+ if (!strcmp(&list[o], needle))
+ return true;
+ o += strlen(&list[o]) + 1;
+ }
+ return false;
+}
+
+/*
+ * 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;
+
+ if (xattr_is_userns_supported(name, false) >= 0)
+ newname = name;
+ else {
+ newname = xattr_rewrite_userns_xattr(name);
+ if (IS_ERR(newname)) {
+ d_off = PTR_ERR(newname);
+ goto out_free;
+ }
+ }
+ if (newname && !xattr_list_contains(nlist, d_off, 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:
+ * 1a) 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.
+ *
+ * 1b) If security.foo@uid=<uid> is not available, the security.foo of the
+ * parent namespace is tried to be read. This procedure is repeated up to
+ * the init user namespace. This step only applies for reading of extended
+ * attributes and provides the same behavior as older systems where the
+ * host's extended attributes applied to user namespaces.
+ *
+ * 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, except for 1b).
+ *
+ * This function returns a buffer with either the original name or the
+ * user namespace adjusted name of the extended attribute.
+ *
+ * @name: the full name of the extended attribute, e.g. security.foo
+ */
+char *
+xattr_userns_name(const char *name, struct user_namespace *userns)
+{
+ size_t buflen;
+ char *buffer, *n_uidstr;
+ kuid_t root_uid = make_kuid(userns, 0);
+ int idx, error;
+ size_t len;
+
+ /* only security.foo will be changed here - prefix match here */
+ idx = xattr_is_userns_supported(name, true);
+ if (idx < 0)
+ goto out_copy;
+
+ /* read security.foo? --> read security.foo@uid=<uid> instead */
+ len = strlen(userns_xattrs[idx]);
+ if (name[len] == 0) {
+ /*
+ * init_user_ns or userns with root mapped to uid 0
+ * may read security.foo directly
+ */
+ if (userns == &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), name);
+ if (!buffer)
+ return ERR_PTR(-ENOMEM);
+
+ return buffer;
+ }
+
+ /*
+ * We must have name[len] == '@'.
+ */
+ error = xattr_parse_uid_make_kuid(&name[len],
+ userns,
+ &n_uidstr);
+ if (error)
+ return ERR_PTR(error);
+
+ /* name[len] == '@' */
+ buflen = len + strlen(n_uidstr) + 1;
+ buffer = kmalloc(buflen, GFP_KERNEL);
+ if (!buffer) {
+ kfree(n_uidstr);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ snprintf(buffer, len + 1, "%s", name);
+ snprintf(&buffer[len], buflen - len, "%s", n_uidstr);
+ kfree(n_uidstr);
+
+ return buffer;
+
+out_copy:
+ buffer = kstrdup(name, GFP_KERNEL);
+ if (!buffer)
+ return ERR_PTR(-ENOMEM);
+
+ return buffer;
+}
+
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 *newname;
+ int ret;
+ newname = xattr_userns_name(name, current_user_ns());
+ if (IS_ERR(newname))
+ return PTR_ERR(newname);
+ name = newname;
handler = xattr_resolve_name(inode, &name);
- if (IS_ERR(handler))
- return PTR_ERR(handler);
- if (!handler->set)
- return -EOPNOTSUPP;
+ if (IS_ERR(handler)) {
+ ret = PTR_ERR(handler);
+ goto out;
+ }
+ if (!handler->set) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
if (size == 0)
value = ""; /* empty EA, do not remove */
- return handler->set(handler, dentry, inode, name, value, size, flags);
+ ret = handler->set(handler, dentry, inode, name, value, size, flags);
+
+out:
+ kfree(newname);
+ return ret;
}
EXPORT_SYMBOL(__vfs_setxattr);
@@ -301,14 +721,39 @@ ssize_t
__vfs_getxattr(struct dentry *dentry, struct inode *inode, const char *name,
void *value, size_t size)
{
- const struct xattr_handler *handler;
+ const struct xattr_handler *handler = NULL;
+ char *newname = NULL;
+ int ret, userns_supt_xattr;
+ struct user_namespace *userns = current_user_ns();
+
+ userns_supt_xattr = (xattr_is_userns_supported(name, false) >= 0);
+
+ do {
+ kfree(newname);
+
+ newname = xattr_userns_name(name, userns);
+ if (IS_ERR(newname))
+ return PTR_ERR(newname);
+
+ if (!handler) {
+ name = newname;
+ handler = xattr_resolve_name(inode, &name);
+ if (IS_ERR(handler)) {
+ ret = PTR_ERR(handler);
+ goto out;
+ }
+ if (!handler->get) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
+ }
+ ret = handler->get(handler, dentry, inode, name, value, size);
+ userns = userns->parent;
+ } while ((ret == -ENODATA) && userns && userns_supt_xattr);
- 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);
+out:
+ kfree(newname);
+ return ret;
}
EXPORT_SYMBOL(__vfs_getxattr);
@@ -328,8 +773,16 @@ vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
if (!strncmp(name, XATTR_SECURITY_PREFIX,
XATTR_SECURITY_PREFIX_LEN)) {
- const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
- int ret = xattr_getsecurity(inode, suffix, value, size);
+ int ret;
+ const char *suffix;
+ char *newname = xattr_userns_name(name, current_user_ns());
+ if (IS_ERR(newname))
+ return PTR_ERR(newname);
+
+ suffix = newname + XATTR_SECURITY_PREFIX_LEN;
+
+ ret = xattr_getsecurity(inode, suffix, value, size);
+ kfree(newname);
/*
* Only overwrite the return value if a security module
* is actually active.
@@ -360,6 +813,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 +825,28 @@ __vfs_removexattr(struct dentry *dentry, const char *name)
{
struct inode *inode = d_inode(dentry);
const struct xattr_handler *handler;
+ char *newname;
+ int ret;
+ newname = xattr_userns_name(name, current_user_ns());
+ if (IS_ERR(newname))
+ return PTR_ERR(newname);
+ name = newname;
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);
+ if (IS_ERR(handler)) {
+ ret = PTR_ERR(handler);
+ goto out;
+ }
+ if (!handler->set) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
+ ret = handler->set(handler, dentry, inode, name, NULL, 0, XATTR_REPLACE);
+
+out:
+ kfree(newname);
+
+ 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] | [next] | [standalone]
| From | "Serge E. Hallyn" <serge@hallyn.com> |
|---|---|
| Date | 2017-07-11 19:20 +0200 |
| Message-ID | <u26Zk-59q-19@gated-at.bofh.it> |
| In reply to | #1685113 |
Quoting Stefan Berger (Stefan Bergerstefanb@linux.vnet.ibm.com):
> er.kernel.org>
> X-Mailing-List: linux-kernel@vger.kernel.org
> Content-Length: 19839
> Lines: 700
> X-UID: 24770
> Status: RO
>
> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>
> 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:
>
> 1a) 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.
>
> 1b) If security.foo@uid=<uid> is not available, the security.foo of the
> parent namespace is tried to be read. This procedure is repeated up to
> the init user namespace. This step only applies for reading of extended
> attributes and provides the same behavior as older system where the
> host's extended attributes applied to user namespaces.
>
> 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 | 509 +++++++++++++++++++++++++++++++++++++++++++++--
> security/commoncap.c | 36 +++-
> security/selinux/hooks.c | 9 +-
> 3 files changed, 523 insertions(+), 31 deletions(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 464c94b..eacad9e 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -133,20 +133,440 @@ 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;
I think you here need to also check that the next char is either
'\0' or '.' (or maybe '@')
> + } 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 string
> + *
> + * @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;
Do you need to print out the prefix here?
> + else
> + sprintf(buffer, "%s@uid=%u",
> + (prefix) ? prefix : "",
> + uid);
> +
> + return buffer;
> +}
[toc] | [prev] | [next] | [standalone]
| From | Stefan Berger <stefanb@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-07-12 02:20 +0200 |
| Message-ID | <u2dxL-Mm-5@gated-at.bofh.it> |
| In reply to | #1685225 |
On 07/11/2017 01:12 PM, Serge E. Hallyn wrote:
> Quoting Stefan Berger (Stefan Bergerstefanb@linux.vnet.ibm.com):
>> er.kernel.org>
>> X-Mailing-List: linux-kernel@vger.kernel.org
>> Content-Length: 19839
>> Lines: 700
>> X-UID: 24770
>> Status: RO
>>
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>> 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:
>>
>> 1a) 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.
>>
>> 1b) If security.foo@uid=<uid> is not available, the security.foo of the
>> parent namespace is tried to be read. This procedure is repeated up to
>> the init user namespace. This step only applies for reading of extended
>> attributes and provides the same behavior as older system where the
>> host's extended attributes applied to user namespaces.
>>
>> 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 | 509 +++++++++++++++++++++++++++++++++++++++++++++--
>> security/commoncap.c | 36 +++-
>> security/selinux/hooks.c | 9 +-
>> 3 files changed, 523 insertions(+), 31 deletions(-)
>>
>> diff --git a/fs/xattr.c b/fs/xattr.c
>> index 464c94b..eacad9e 100644
>> --- a/fs/xattr.c
>> +++ b/fs/xattr.c
>> @@ -133,20 +133,440 @@ 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;
> I think you here need to also check that the next char is either
> '\0' or '.' (or maybe '@')
I have the checks for '@' and '\0' done by the caller. With the current
support of only security.capability I don't think we need to check for '.'.
>
>> + } 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 string
>> + *
>> + * @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;
> Do you need to print out the prefix here?
Right. Fixed.
>
>> + else
>> + sprintf(buffer, "%s@uid=%u",
>> + (prefix) ? prefix : "",
>> + uid);
>> +
>> + return buffer;
>> +}
Thanks.
Stefan
[toc] | [prev] | [next] | [standalone]
| From | "Serge E. Hallyn" <serge@hallyn.com> |
|---|---|
| Date | 2017-07-12 02:50 +0200 |
| Message-ID | <u2e0N-Yk-3@gated-at.bofh.it> |
| In reply to | #1685413 |
Quoting Stefan Berger (stefanb@linux.vnet.ibm.com):
> On 07/11/2017 01:12 PM, Serge E. Hallyn wrote:
> >>diff --git a/fs/xattr.c b/fs/xattr.c
> >>index 464c94b..eacad9e 100644
> >>--- a/fs/xattr.c
> >>+++ b/fs/xattr.c
> >>@@ -133,20 +133,440 @@ 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;
> >I think you here need to also check that the next char is either
> >'\0' or '.' (or maybe '@')
>
> I have the checks for '@' and '\0' done by the caller. With the
> current support of only security.capability I don't think we need to
> check for '.'.
Ah - ok, thanks.
[toc] | [prev] | [next] | [standalone]
| From | "Serge E. Hallyn" <serge@hallyn.com> |
|---|---|
| Date | 2017-07-12 05:50 +0200 |
| Message-ID | <u2gOZ-2Ol-3@gated-at.bofh.it> |
| In reply to | #1685113 |
Quoting Stefan Berger (Stefan Bergerstefanb@linux.vnet.ibm.com):
> +/*
> + * 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;
> +
> + if (xattr_is_userns_supported(name, false) >= 0)
> + newname = name;
> + else {
> + newname = xattr_rewrite_userns_xattr(name);
Why are you doing this here? If we get here it means that
xattr_is_userns_supported() returned < 0, meaning name is
not userns-supported. So xattr_rewrite_userns_xattr() will
just return name. Am I missing something?
> + if (IS_ERR(newname)) {
> + d_off = PTR_ERR(newname);
> + goto out_free;
> + }
> + }
> + if (newname && !xattr_list_contains(nlist, d_off, newname)) {
Now here, if name was recalculated to @newname, and @newname is
found in the nlist, that should raise an error right? Something
fishy is going on?
> + nlen = strlen(newname);
> +
> + if (nlist) {
> + if (nlen + 1 > list_maxlen)
d_off needs to be set to -ERANGE here.
> + 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;
> +}
[toc] | [prev] | [next] | [standalone]
| From | Stefan Berger <stefanb@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-07-12 13:40 +0200 |
| Message-ID | <u2o9P-7G6-1@gated-at.bofh.it> |
| In reply to | #1685495 |
On 07/11/2017 11:45 PM, Serge E. Hallyn wrote:
> Quoting Stefan Berger (Stefan Bergerstefanb@linux.vnet.ibm.com):
>> +/*
>> + * 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;
>> +
>> + if (xattr_is_userns_supported(name, false) >= 0)
>> + newname = name;
>> + else {
>> + newname = xattr_rewrite_userns_xattr(name);
> Why are you doing this here? If we get here it means that
> xattr_is_userns_supported() returned < 0, meaning name is
> not userns-supported. So xattr_rewrite_userns_xattr() will
> just return name. Am I missing something?
xattr_is_userns_support(name, false) does a _full string match_ rather
than a prefix match and will only return >= 0 for security.capability.
This case handles the hosts's security.capability which 'shines
through' for read and needs to be listed. Only in this case we set
newname=name.
In the else branch we handle security.capability@uid=1000 and rewrite
that to security.capability for root mapping to uid=1000.
>
>> + if (IS_ERR(newname)) {
>> + d_off = PTR_ERR(newname);
>> + goto out_free;
>> + }
>> + }
>> + if (newname && !xattr_list_contains(nlist, d_off, newname)) {
> Now here, if name was recalculated to @newname, and @newname is
> found in the nlist, that should raise an error right? Something
> fishy is going on?
If security.capability is set on a file but the container doesn't have
security.capability@uid=1000, we still need to list the former here.
However, we end up with duplicates if security.capability is there and
security.capability@uid=1000 is also there and root is mapped to
uid=1000. Both would be shown as security.capability inside the
container. In this case we need to filter.
I think the code is correct. More problematic is a memory leak in the
error case. Will fix that.
>
>> + nlen = strlen(newname);
>> +
>> + if (nlist) {
>> + if (nlen + 1 > list_maxlen)
> d_off needs to be set to -ERANGE here.
Fixed.
>
>> + 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;
>> +}
[toc] | [prev] | [next] | [standalone]
| From | "Serge E. Hallyn" <serge@hallyn.com> |
|---|---|
| Date | 2017-07-12 19:40 +0200 |
| Message-ID | <u2tMd-2LF-3@gated-at.bofh.it> |
| In reply to | #1685712 |
Quoting Stefan Berger (stefanb@linux.vnet.ibm.com):
> On 07/11/2017 11:45 PM, Serge E. Hallyn wrote:
> >Quoting Stefan Berger (Stefan Bergerstefanb@linux.vnet.ibm.com):
> >>+/*
> >>+ * 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;
> >>+
> >>+ if (xattr_is_userns_supported(name, false) >= 0)
> >>+ newname = name;
> >>+ else {
> >>+ newname = xattr_rewrite_userns_xattr(name);
> >Why are you doing this here? If we get here it means that
> >xattr_is_userns_supported() returned < 0, meaning name is
> >not userns-supported. So xattr_rewrite_userns_xattr() will
> >just return name. Am I missing something?
>
> xattr_is_userns_support(name, false) does a _full string match_
> rather than a prefix match and will only return >= 0 for
> security.capability. This case handles the hosts's
> security.capability which 'shines through' for read and needs to be
> listed. Only in this case we set newname=name.
Ah, right.
I think it would be worth #defining XATTR_PREFIX_SEARCH and
XATTR_FULLNAME_SEARCH or something. Or maybe not, maybe I was
just being dense.
> In the else branch we handle security.capability@uid=1000 and
> rewrite that to security.capability for root mapping to uid=1000.
>
> >
> >>+ if (IS_ERR(newname)) {
> >>+ d_off = PTR_ERR(newname);
> >>+ goto out_free;
> >>+ }
> >>+ }
> >>+ if (newname && !xattr_list_contains(nlist, d_off, newname)) {
> >Now here, if name was recalculated to @newname, and @newname is
> >found in the nlist, that should raise an error right? Something
> >fishy is going on?
>
> If security.capability is set on a file but the container doesn't
> have security.capability@uid=1000, we still need to list the former
> here. However, we end up with duplicates if security.capability is
> there and security.capability@uid=1000 is also there and root is
> mapped to uid=1000. Both would be shown as security.capability
> inside the container. In this case we need to filter.
Gotcha, thanks.
> I think the code is correct. More problematic is a memory leak in
> the error case. Will fix that.
Great.
> >
> >>+ nlen = strlen(newname);
> >>+
> >>+ if (nlist) {
> >>+ if (nlen + 1 > list_maxlen)
> >d_off needs to be set to -ERANGE here.
>
> Fixed.
Great, thanks.
-serge
[toc] | [prev] | [next] | [standalone]
| From | James Morris <jmorris@namei.org> |
|---|---|
| Date | 2017-07-12 10:10 +0200 |
| Subject | Re: [PATCH v2] xattr: Enable security.capability in user namespaces |
| Message-ID | <u2kSC-5FA-11@gated-at.bofh.it> |
| In reply to | #1685113 |
On Tue, 11 Jul 2017, Stefan Berger wrote:
> + buflen = sizeof("@uid=") - 1 + sizeof("4294967295") - 1 + 1;
Why not strlen() here?
--
James Morris
<jmorris@namei.org>
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-07-12 15:40 +0200 |
| Message-ID | <u2q1Y-mq-3@gated-at.bofh.it> |
| In reply to | #1685113 |
Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> writes:
> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>
> 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:
>
> 1a) 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.
>
> 1b) If security.foo@uid=<uid> is not available, the security.foo of the
> parent namespace is tried to be read. This procedure is repeated up to
> the init user namespace. This step only applies for reading of extended
> attributes and provides the same behavior as older system where the
> host's extended attributes applied to user namespaces.
>
> 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>
It doesn't look like this is coming through Serge so I don't see how
the Signed-off-by tag is legtimate.
From the replies to this it doesn't look like Serge has reviewed this
version either.
I am disappointed that all of my concerns about technical feasibility
remain unaddressed.
I hope my reading and review of the code goes better than my reading of
it's introduction.
Eric
> ---
> fs/xattr.c | 509 +++++++++++++++++++++++++++++++++++++++++++++--
> security/commoncap.c | 36 +++-
> security/selinux/hooks.c | 9 +-
> 3 files changed, 523 insertions(+), 31 deletions(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 464c94b..eacad9e 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -133,20 +133,440 @@ 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 string
> + *
> + * @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) {
> + /* only rewrite those in userns_xattr[*] */
> + return name;
> + }
> +
> + /* 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_contains - check whether an xattr list already contains a needle
> + *
> + * @list : 0-byte separated strings
> + * @listlen : length of the list
> + * @needle : the needle to search for
> + */
> +static int
> +xattr_list_contains(const char *list, size_t listlen, const char *needle)
> +{
> + size_t o = 0;
> +
> + while (o < listlen) {
> + if (!strcmp(&list[o], needle))
> + return true;
> + o += strlen(&list[o]) + 1;
> + }
> + return false;
> +}
> +
> +/*
> + * 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;
> +
> + if (xattr_is_userns_supported(name, false) >= 0)
> + newname = name;
> + else {
> + newname = xattr_rewrite_userns_xattr(name);
> + if (IS_ERR(newname)) {
> + d_off = PTR_ERR(newname);
> + goto out_free;
> + }
> + }
> + if (newname && !xattr_list_contains(nlist, d_off, 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:
> + * 1a) 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.
> + *
> + * 1b) If security.foo@uid=<uid> is not available, the security.foo of the
> + * parent namespace is tried to be read. This procedure is repeated up to
> + * the init user namespace. This step only applies for reading of extended
> + * attributes and provides the same behavior as older systems where the
> + * host's extended attributes applied to user namespaces.
> + *
> + * 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, except for 1b).
> + *
> + * This function returns a buffer with either the original name or the
> + * user namespace adjusted name of the extended attribute.
> + *
> + * @name: the full name of the extended attribute, e.g. security.foo
> + */
> +char *
> +xattr_userns_name(const char *name, struct user_namespace *userns)
> +{
> + size_t buflen;
> + char *buffer, *n_uidstr;
> + kuid_t root_uid = make_kuid(userns, 0);
> + int idx, error;
> + size_t len;
> +
> + /* only security.foo will be changed here - prefix match here */
> + idx = xattr_is_userns_supported(name, true);
> + if (idx < 0)
> + goto out_copy;
> +
> + /* read security.foo? --> read security.foo@uid=<uid> instead */
> + len = strlen(userns_xattrs[idx]);
> + if (name[len] == 0) {
> + /*
> + * init_user_ns or userns with root mapped to uid 0
> + * may read security.foo directly
> + */
> + if (userns == &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), name);
> + if (!buffer)
> + return ERR_PTR(-ENOMEM);
> +
> + return buffer;
> + }
> +
> + /*
> + * We must have name[len] == '@'.
> + */
> + error = xattr_parse_uid_make_kuid(&name[len],
> + userns,
> + &n_uidstr);
> + if (error)
> + return ERR_PTR(error);
> +
> + /* name[len] == '@' */
> + buflen = len + strlen(n_uidstr) + 1;
> + buffer = kmalloc(buflen, GFP_KERNEL);
> + if (!buffer) {
> + kfree(n_uidstr);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + snprintf(buffer, len + 1, "%s", name);
> + snprintf(&buffer[len], buflen - len, "%s", n_uidstr);
> + kfree(n_uidstr);
> +
> + return buffer;
> +
> +out_copy:
> + buffer = kstrdup(name, GFP_KERNEL);
> + if (!buffer)
> + return ERR_PTR(-ENOMEM);
> +
> + return buffer;
> +}
> +
> 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 *newname;
> + int ret;
>
> + newname = xattr_userns_name(name, current_user_ns());
> + if (IS_ERR(newname))
> + return PTR_ERR(newname);
> + name = newname;
> handler = xattr_resolve_name(inode, &name);
> - if (IS_ERR(handler))
> - return PTR_ERR(handler);
> - if (!handler->set)
> - return -EOPNOTSUPP;
> + if (IS_ERR(handler)) {
> + ret = PTR_ERR(handler);
> + goto out;
> + }
> + if (!handler->set) {
> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> if (size == 0)
> value = ""; /* empty EA, do not remove */
> - return handler->set(handler, dentry, inode, name, value, size, flags);
> + ret = handler->set(handler, dentry, inode, name, value, size, flags);
> +
> +out:
> + kfree(newname);
> + return ret;
> }
> EXPORT_SYMBOL(__vfs_setxattr);
>
> @@ -301,14 +721,39 @@ ssize_t
> __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char *name,
> void *value, size_t size)
> {
> - const struct xattr_handler *handler;
> + const struct xattr_handler *handler = NULL;
> + char *newname = NULL;
> + int ret, userns_supt_xattr;
> + struct user_namespace *userns = current_user_ns();
> +
> + userns_supt_xattr = (xattr_is_userns_supported(name, false) >= 0);
> +
> + do {
> + kfree(newname);
> +
> + newname = xattr_userns_name(name, userns);
> + if (IS_ERR(newname))
> + return PTR_ERR(newname);
> +
> + if (!handler) {
> + name = newname;
> + handler = xattr_resolve_name(inode, &name);
> + if (IS_ERR(handler)) {
> + ret = PTR_ERR(handler);
> + goto out;
> + }
> + if (!handler->get) {
> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> + }
> + ret = handler->get(handler, dentry, inode, name, value, size);
> + userns = userns->parent;
> + } while ((ret == -ENODATA) && userns && userns_supt_xattr);
>
> - 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);
> +out:
> + kfree(newname);
> + return ret;
> }
> EXPORT_SYMBOL(__vfs_getxattr);
>
> @@ -328,8 +773,16 @@ vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
>
> if (!strncmp(name, XATTR_SECURITY_PREFIX,
> XATTR_SECURITY_PREFIX_LEN)) {
> - const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> - int ret = xattr_getsecurity(inode, suffix, value, size);
> + int ret;
> + const char *suffix;
> + char *newname = xattr_userns_name(name, current_user_ns());
> + if (IS_ERR(newname))
> + return PTR_ERR(newname);
> +
> + suffix = newname + XATTR_SECURITY_PREFIX_LEN;
> +
> + ret = xattr_getsecurity(inode, suffix, value, size);
> + kfree(newname);
> /*
> * Only overwrite the return value if a security module
> * is actually active.
> @@ -360,6 +813,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 +825,28 @@ __vfs_removexattr(struct dentry *dentry, const char *name)
> {
> struct inode *inode = d_inode(dentry);
> const struct xattr_handler *handler;
> + char *newname;
> + int ret;
>
> + newname = xattr_userns_name(name, current_user_ns());
> + if (IS_ERR(newname))
> + return PTR_ERR(newname);
> + name = newname;
> 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);
> + if (IS_ERR(handler)) {
> + ret = PTR_ERR(handler);
> + goto out;
> + }
> + if (!handler->set) {
> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> + ret = handler->set(handler, dentry, inode, name, NULL, 0, XATTR_REPLACE);
> +
> +out:
> + kfree(newname);
> +
> + 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.
[toc] | [prev] | [next] | [standalone]
| From | "Serge E. Hallyn" <serge@hallyn.com> |
|---|---|
| Date | 2017-07-12 19:10 +0200 |
| Message-ID | <u2tjc-2BT-9@gated-at.bofh.it> |
| In reply to | #1685779 |
Quoting Eric W. Biederman (ebiederm@xmission.com): > Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> writes: > > 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> > > It doesn't look like this is coming through Serge so I don't see how > the Signed-off-by tag is legtimate. This is mostly explained by the fact that there have been a *lot* of changes, many of them discussed in private emails. > >From the replies to this it doesn't look like Serge has reviewed this > version either. > > I am disappointed that all of my concerns about technical feasibility > remain unaddressed. Can you re-state those, or give a link to them? I'd really like to get to a point where unprivileged containers can start using filecaps - at this point if that means having an extra temporary file format based on my earlier patchset while we hash this out, that actually seems worthwhile. But it would of course be ideal if we could do the name based caps right in the first place. -serge
[toc] | [prev] | [next] | [standalone]
| From | James Morris <jmorris@namei.org> |
|---|---|
| Date | 2017-07-13 00:30 +0200 |
| Subject | Re: [PATCH v2] xattr: Enable security.capability in user namespaces |
| Message-ID | <u2yiR-5DX-3@gated-at.bofh.it> |
| In reply to | #1685916 |
On Wed, 12 Jul 2017, Serge E. Hallyn wrote: > Quoting Eric W. Biederman (ebiederm@xmission.com): > > Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> writes: > > > 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> > > > > It doesn't look like this is coming through Serge so I don't see how > > the Signed-off-by tag is legtimate. > > This is mostly explained by the fact that there have been a *lot* of > changes, many of them discussed in private emails. Please try and keep technical discussions public or at least document them when reposting the patches. -- James Morris <jmorris@namei.org>
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-07-13 02:50 +0200 |
| Message-ID | <u2Aul-6Uv-9@gated-at.bofh.it> |
| In reply to | #1686075 |
James Morris <jmorris@namei.org> writes: > On Wed, 12 Jul 2017, Serge E. Hallyn wrote: > >> Quoting Eric W. Biederman (ebiederm@xmission.com): >> > Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> writes: >> > > 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> >> > >> > It doesn't look like this is coming through Serge so I don't see how >> > the Signed-off-by tag is legtimate. >> >> This is mostly explained by the fact that there have been a *lot* of >> changes, many of them discussed in private emails. > > Please try and keep technical discussions public or at least document them > when reposting the patches. Yes please. A public discussion helps to understand what the challenges are. Eric
[toc] | [prev] | [next] | [standalone]
| From | "Serge E. Hallyn" <serge@hallyn.com> |
|---|---|
| Date | 2017-07-13 03:10 +0200 |
| Message-ID | <u2ANH-7ig-5@gated-at.bofh.it> |
| In reply to | #1686154 |
Quoting Eric W. Biederman (ebiederm@xmission.com): > James Morris <jmorris@namei.org> writes: > > > On Wed, 12 Jul 2017, Serge E. Hallyn wrote: > > > >> Quoting Eric W. Biederman (ebiederm@xmission.com): > >> > Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> writes: > >> > > 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> > >> > > >> > It doesn't look like this is coming through Serge so I don't see how > >> > the Signed-off-by tag is legtimate. > >> > >> This is mostly explained by the fact that there have been a *lot* of > >> changes, many of them discussed in private emails. I wasn't clear here. There were a lot of changes between the first mention of the approach and the posting of v1. My point was that I did in fact agree to Reviewed-by, and the fact that I've found a few more things to point out only reflects my missing them before. I don't think my name is being mis-used. > > Please try and keep technical discussions public or at least document them > > when reposting the patches. > > Yes please. > > A public discussion helps to understand what the challenges are. Yes, all discussion (that I can find in my mbox) since v1 has been public. -serge
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-07-13 01:30 +0200 |
| Message-ID | <u2zeV-6fP-11@gated-at.bofh.it> |
| In reply to | #1685916 |
"Serge E. Hallyn" <serge@hallyn.com> writes: > Quoting Eric W. Biederman (ebiederm@xmission.com): >> Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> writes: >> > 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> >> >> It doesn't look like this is coming through Serge so I don't see how >> the Signed-off-by tag is legtimate. > > This is mostly explained by the fact that there have been a *lot* of > changes, many of them discussed in private emails. > >> >From the replies to this it doesn't look like Serge has reviewed this >> version either. >> >> I am disappointed that all of my concerns about technical feasibility >> remain unaddressed. > > Can you re-state those, or give a link to them? Well I only posted about one substantive comment on the last round so it should be easy to find that said. The big question is how does this intereact with filesystems xattr implementations? There is the potential that we create many more security xattrs this way. How does that scale? With more names etc. What happens if we have one xattr per uid for 1000+ uids? How does this interact with filesystems optimization of xattr names? For some filesystems they optmize the xattr names, and don't store the entire thing. > I'd really like to get to a point where unprivileged containers can start > using filecaps - at this point if that means having an extra temporary > file format based on my earlier patchset while we hash this out, that > actually seems worthwhile. But it would of course be ideal if we could > do the name based caps right in the first place. This whole new version has set my review back to square one unfortunately. Eric
[toc] | [prev] | [next] | [standalone]
| From | Stefan Berger <stefanb@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-07-13 02:50 +0200 |
| Message-ID | <u2Aul-6Uv-1@gated-at.bofh.it> |
| In reply to | #1686120 |
On 07/12/2017 07:13 PM, Eric W. Biederman wrote:
> "Serge E. Hallyn" <serge@hallyn.com> writes:
>
>> Quoting Eric W. Biederman (ebiederm@xmission.com):
>>> Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> writes:
>>>> 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>
>>> It doesn't look like this is coming through Serge so I don't see how
>>> the Signed-off-by tag is legtimate.
>> This is mostly explained by the fact that there have been a *lot* of
>> changes, many of them discussed in private emails.
>>
>>> >From the replies to this it doesn't look like Serge has reviewed this
>>> version either.
>>>
>>> I am disappointed that all of my concerns about technical feasibility
>>> remain unaddressed.
>> Can you re-state those, or give a link to them?
> Well I only posted about one substantive comment on the last round
> so it should be easy to find that said.
>
> The big question is how does this intereact with filesystems
> xattr implementations?
>
> There is the potential that we create many more security xattrs this
> way. How does that scale? With more names etc.
It doesn't scale. Shared filesystems are a problem if many containers
use them.
'man listxattr' also mentions this here as a BUG:
" As noted in xattr(7), the VFS imposes a limit of 64 kB on the size of
the extended attribute name list returned by listxattr(7). If the
total size of attribute names attached to a file exceeds this limit,
it is no longer possible to retrieve the list of attribute names."
A simple test on ext4:
#> touch foo
#> for ((i = 0; i < 200; i++)); do setfattr -n user.foo${i} -v hello
foo; done
user.foo126 was the last one created...
Depending on the size of the data the xattrs are writing, the limit is
reached sooner. Writing 'hellohello' only goes up to 'user.foo112'.
Maybe one could try to encode the data more efficiently or as Serge did
write the uid on the xattr value side, but either way, it won't scale
due to that VFS limit.
Stefan
> What happens if we have one xattr per uid for 1000+ uids?
>
> How does this interact with filesystems optimization of xattr names?
> For some filesystems they optmize the xattr names, and don't store the
> entire thing.
>
>> I'd really like to get to a point where unprivileged containers can start
>> using filecaps - at this point if that means having an extra temporary
>> file format based on my earlier patchset while we hash this out, that
>> actually seems worthwhile. But it would of course be ideal if we could
>> do the name based caps right in the first place.
> This whole new version has set my review back to square one
> unfortunately.
>
> Eric
>
[toc] | [prev] | [next] | [standalone]
| From | Theodore Ts'o <tytso@mit.edu> |
|---|---|
| Date | 2017-07-13 03:20 +0200 |
| Message-ID | <u2AXn-7n9-3@gated-at.bofh.it> |
| In reply to | #1686153 |
I'm really confused what problem that is trying to be solved, here, but it **feels** really, really wrong. Why do we need to store all of this state on a per-file basis, instead of some kind of per-file system or per-container data structure? And how many of these security.foo@uid=bar xattrs do you expect there to be? How many "foo", and how many "bar"? Maybe I missed the full write up, in which case please send me a link to the full writeup --- ideally in the form of a design doc that explains the problem statement, gives some examples of how it's going to be used, what were the other alternatives that were considered, and why they were rejected, etc. Thanks, - Ted
[toc] | [prev] | [next] | [standalone]
| From | "Serge E. Hallyn" <serge@hallyn.com> |
|---|---|
| Date | 2017-07-13 04:40 +0200 |
| Message-ID | <u2CcO-87x-7@gated-at.bofh.it> |
| In reply to | #1686164 |
Quoting Theodore Ts'o (tytso@mit.edu): > I'm really confused what problem that is trying to be solved, here, > but it **feels** really, really wrong. Hi, The intro to my original patch might help (or maybe not), as it has a different motivating text: http://lkml.org/lkml/2016/11/19/158 We want file capabilities to be supported in unprivileged containers, so that a piece of software can count on them being available rather than having to supporting multiple ways of getting+dropping privilege (for instance, being installed as uid 1000 with cap_net_raw=pe, versus being installed setuid-root and being expected to do PR_SET_KEEPCAPS and setuid). If subuids 10000-20000 are delegated to uid 1001 on the host, and uid 1001 sets up a container with subuid 100000 mapped to container uid 0, then the container root should be able to write file capabilities which affect (that is, delegate container root's privilege to) all ids over which it has privilege (all uids mapped into the container), but should not have privilege over any uids not mapped into the container. With regular file capabilities, this is impossible, since any filecap he writes can then be exercised on the host by uid 1000. The point of this set (and the ones before it) is to make it so that the filecap written by the container root is tagged on disk as belonging to subuid 100000. > Why do we need to store all of this state on a per-file basis, instead > of some kind of per-file system or per-container data structure? This needs to be writeable by an unprivileged user, with no help from the admin. AFAICS that rules out per-fs data structure. Note we are not assuming a filesystem per container. The typical case is (for instance) ~/.local/share/lxc/c1/rootfs being the root of container c1's filesystem. Mounting a filesystem from inside a user namespace is still mostly science fiction today. > And how many of these security.foo@uid=bar xattrs do you expect there > to be? How many "foo", and how many "bar"? For now I'm expecting two foos - security and ima. The '@uid=bar' is generic enough that it *can* be re-used for a different kind of property if we decide to later, but I have no intention of adding anything. Casey has mentioned 'smack=', but i think only to keep the option open. I don't believe he has concrete plans. > Maybe I missed the full write up, in which case please send me a link > to the full writeup --- ideally in the form of a design doc that > explains the problem statement, gives some examples of how it's going > to be used, what were the other alternatives that were considered, and > why they were rejected, etc. As I'd mentioned in an even older patch, http://lkml.org/lkml/2016/5/18/622 , I had considered using a completely separate xattr name, but that would have required invasive userspace changes. There's no design doc as such, mainly a progressive series of patches to lkml. I am very seriously considering writing a paper to detail both this design and the user ns design in general, as it has become clear (in unrelated conversations) there is still a lot of confusiong out there regarding uid namespaces and targeted capabilities. But it's not written yet. -serge
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-07-13 14:20 +0200 |
| Message-ID | <u2Lg8-5xB-43@gated-at.bofh.it> |
| In reply to | #1686164 |
Theodore Ts'o <tytso@mit.edu> writes: > I'm really confused what problem that is trying to be solved, here, > but it **feels** really, really wrong. > > Why do we need to store all of this state on a per-file basis, instead > of some kind of per-file system or per-container data structure? > > And how many of these security.foo@uid=bar xattrs do you expect there > to be? How many "foo", and how many "bar"? > > Maybe I missed the full write up, in which case please send me a link > to the full writeup --- ideally in the form of a design doc that > explains the problem statement, gives some examples of how it's going > to be used, what were the other alternatives that were considered, and > why they were rejected, etc. The concise summary: Today we have the xattr security.capable that holds a set of capabilities that an application gains when executed. AKA setuid root exec without actually being setuid root. User namespaces have the concept of capabilities that are not global but are limited to their user namespace. We do not currently have filesystem support for this concept. We currently have two proposals on the table. One is to bump the revision number of security.capable and add more information in that xattr. The other is to use a sligthly different capability name. We are currently evaluating between the two proposals. Given that it appears the IMA xattrs will want similar treatment coming up with a good pattern to follow is part of the analysis here. Eric
[toc] | [prev] | [next] | [standalone]
| From | Theodore Ts'o <tytso@mit.edu> |
|---|---|
| Date | 2017-07-13 18:50 +0200 |
| Message-ID | <u2Ptn-813-7@gated-at.bofh.it> |
| In reply to | #1686496 |
On Thu, Jul 13, 2017 at 07:11:36AM -0500, Eric W. Biederman wrote: > The concise summary: > > Today we have the xattr security.capable that holds a set of > capabilities that an application gains when executed. AKA setuid root exec > without actually being setuid root. > > User namespaces have the concept of capabilities that are not global but > are limited to their user namespace. We do not currently have > filesystem support for this concept. So correct me if I am wrong; in general, there will only be one variant of the form: security.foo@uid=15000 It's not like there will be: security.foo@uid=1000 security.foo@uid=2000 Except.... if you have an Distribution root directory which is shared by many containers, you would need to put the xattrs in the overlay inodes. Worse, each time you launch a new container, with a new subuid allocation, you will have to iterate over all files with capabilities and do a copy-up operations on the xattrs in overlayfs. So that's actually a bit of a disaster. So for distribution overlays, you will need to do things a different way, which is to map the distro subdirectory so you know that the capability with the global uid 0 should be used for the container "root" uid, right? So this hack of using security.foo@uid=1000 is *only* useful when the subcontainer root wants to create the privileged executable. You still have to do things the other way. So can we make perhaps the assertion that *either*: security.foo exists, *or* security.foo@uid=BAR exists, but never both? And there BAR is exclusive to only one instances? Otherwise, I suspect that the architecture is going to turn around and bite us in the *ss eventually, because someone will want to do something crazy and the solution will not be scalable. -Ted
[toc] | [prev] | [next] | [standalone]
| From | Stefan Berger <stefanb@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-07-13 19:10 +0200 |
| Message-ID | <u2PMK-8mJ-9@gated-at.bofh.it> |
| In reply to | #1686779 |
On 07/13/2017 12:40 PM, Theodore Ts'o wrote:
> On Thu, Jul 13, 2017 at 07:11:36AM -0500, Eric W. Biederman wrote:
>> The concise summary:
>>
>> Today we have the xattr security.capable that holds a set of
>> capabilities that an application gains when executed. AKA setuid root exec
>> without actually being setuid root.
>>
>> User namespaces have the concept of capabilities that are not global but
>> are limited to their user namespace. We do not currently have
>> filesystem support for this concept.
> So correct me if I am wrong; in general, there will only be one
> variant of the form:
>
> security.foo@uid=15000
>
> It's not like there will be:
>
> security.foo@uid=1000
> security.foo@uid=2000
A file shared by 2 containers, one mapping root to uid=1000, the other
mapping root to uid=2000, will show these two xattrs on the host
(init_user_ns) once these containers set xattrs on that file.
>
> Except.... if you have an Distribution root directory which is shared
> by many containers, you would need to put the xattrs in the overlay
> inodes. Worse, each time you launch a new container, with a new
> subuid allocation, you will have to iterate over all files with
> capabilities and do a copy-up operations on the xattrs in overlayfs.
> So that's actually a bit of a disaster.
Note that we do keep compatibility to existing behavior. The
security.foo of the host is visible inside any container for as long as
the container root user doesn't set its own security.foo on that file,
which then hides it. Does that address this concern?
>
> So for distribution overlays, you will need to do things a different
> way, which is to map the distro subdirectory so you know that the
> capability with the global uid 0 should be used for the container
> "root" uid, right?
>
> So this hack of using security.foo@uid=1000 is *only* useful when the
> subcontainer root wants to create the privileged executable. You
> still have to do things the other way.
>
> So can we make perhaps the assertion that *either*:
>
> security.foo
>
> exists, *or*
>
> security.foo@uid=BAR
>
> exists, but never both? And there BAR is exclusive to only one
> instances?
In the current implementation BAR is visible inside of any instance that
'covers' this uid with the mapping range. Above example of
security.foo@uid=1000 appears as security.foo inside the container with
root mapping to uid 1000 (@uid=0 is suppressed) but also appears as
security.foo@uid=100 with root uid mapping to 900 (and range of at least
101).
>
> Otherwise, I suspect that the architecture is going to turn around and
> bite us in the *ss eventually, because someone will want to do
> something crazy and the solution will not be scalable.
Can you define what 'scalable' means for you in this context?
From what I can see sharing a filesystem between multiple containers
doesn't 'scale well' for virtualizing the xattrs primarily because of
size limitations of xattrs per file.
Stefan
>
> -Ted
>
[toc] | [prev] | [next] | [standalone]
Page 1 of 4 [1] 2 3 4 Next page →
Back to top | Article view | linux.kernel
csiph-web