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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 12 Jul 2017 07:35:49 -0400
From:   Stefan Berger <stefanb@...ux.vnet.ibm.com>
To:     "Serge E. Hallyn" <serge@...lyn.com>
Cc:     ebiederm@...ssion.com, containers@...ts.linux-foundation.org,
        lkp@...org, linux-kernel@...r.kernel.org, zohar@...ux.vnet.ibm.com,
        tycho@...ker.com, James.Bottomley@...senPartnership.com,
        vgoyal@...hat.com, christian.brauner@...lbox.org,
        amir73il@...il.com, linux-security-module@...r.kernel.org,
        casey@...aufler-ca.com
Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces

On 07/11/2017 11:45 PM, Serge E. Hallyn wrote:
> Quoting Stefan Berger (Stefan Bergerstefanb@...ux.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@...=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@...=1000, we still need to list the former here. 
However, we end up with duplicates if security.capability is there and 
security.capability@...=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;
>> +}