lkml.org 
[lkml]   [2017]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2] xattr: Enable security.capability in user namespaces
    From
    Date
    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;
    >> +}


    \
     
     \ /
      Last update: 2017-07-12 13:36    [W:2.081 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site