lkml.org 
[lkml]   [2017]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH v2] xattr: Enable security.capability in user namespaces
    From
    Date
    On 07/18/2017 07:48 AM, Vivek Goyal wrote:
    > On Mon, Jul 17, 2017 at 04:50:22PM -0400, Stefan Berger wrote:
    >> On 07/17/2017 02:58 PM, Vivek Goyal wrote:
    >>> On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote:
    >>>
    >>> [..]
    >>>> +/*
    >>>> + * 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)
    >>> size will never be less than 0 here. Only caller calls this function only
    >>> if size is >0. So can we remove this?
    >> Correct.
    >>
    >>> What about case of "!list". So if user space called listxattr(foo, NULL,
    >>> 0), we want to return the size of buffer as if all the xattrs will be
    >>> returned to user space. But in practice we probably will filter out some
    >>> xattrs so actually returned string will be smaller than size reported
    >>> previously.
    >> This case of size=0 is a problem in userns. Depending on the mapping of the
    >> userid's the list can expand. A security.foo@uid=100 can become
    >> security.foo@uid=100000, if the mapping is set up so that uid 100 on the
    >> host becomes uid 100000 inside the container. So for now we only have
    >> security.capability and the way I solved this is by allocating a 65k buffer
    >> when calling from a userns. In this buffer where we gather the xattr names
    >> and then walk them to determine the size that's needed for the buffer by
    >> simulating the rewriting. It's not nice but I don't know of any other
    >> solution.
    > Hi Stefan,
    >
    > For the case of size==0, why don't we iterate through all the xattr,
    > filter them, remap them and then return the size to process in user
    > namespace. That should fix this? I thought that's what


    For the size==0 we need a temp. buffer where the raw xattr names are
    written to so that the xattr_list_userns_rewrite() can actually rewrite
    what the filesystem drivers returned. Not knowing exactly how big that
    buffer should be, I allocate 65k for it. From what I read there is a 64k
    limit on the vfs layer for xattrs, probably including xattr values. So
    65k would for sure be enough also if each one of the xattr names becomes
    bigger.

    @@ -922,10 +947,20 @@ vfs_listxattr(struct dentry *dentry, char *list,
    size_t size, bool rewrite)
    {
    struct inode *inode = d_inode(dentry);
    ssize_t error;
    + bool getsize = false;

    error = security_inode_listxattr(dentry);
    if (error)
    return error;
    +
    + if (!size) {
    + if (current_user_ns() != &init_user_ns) {
    + size = 65 * 1024;
    + list = kmalloc(size, GFP_KERNEL);
    + }
    + getsize = true;
    + }
    +
    if (inode->i_op->listxattr && (inode->i_opflags & IOP_XATTR)) {
    error = -EOPNOTSUPP;
    error = inode->i_op->listxattr(dentry, list, size);
    @@ -937,6 +972,9 @@ vfs_listxattr(struct dentry *dentry, char *list,
    size_t size, bool rewrite)
    if (error > 0 && rewrite)
    error = xattr_list_userns_rewrite(list, error, size);

    + if (getsize)
    + kfree(list);
    +
    return error;
    }
    EXPORT_SYMBOL_GPL(vfs_listxattr);

    Stefan

    > xattr_list_userns_rewrite() was doing. But looks like this logic will not
    > kick in for the case of size==0 due to "!list" condition.
    >
    > Also we could probably replace "!list" with "!size" wheverever required.
    > Its little easy to read and understand.
    >
    > For the other case where some xattrs can get filtered out and we report
    > a buffer size bigger than actually needed, I am hoping that its acceptable
    > and none of the existing users are broken.
    >
    > Thanks
    > Vivek
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    >

    \
     
     \ /
      Last update: 2017-07-18 14:06    [W:6.733 / U:0.604 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site