Re: [PATCH v2] xattr: Enable security.capability in user namespaces

[Stefan Berger]

On 07/11/2017 11:45 PM, Serge E. Hallyn wrote:
> Quoting Stefan Berger (Stefan Bergerstefanb@xxxxxxxxxxxxxxxxxx):
> > +/*
> > + * 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;
> > +}