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]

Message-Id: <3368afe9-1e0c-6f6a-aba6-9ce26d2e45e4@linux.vnet.ibm.com>
Date:   Wed, 12 Jul 2017 15:19:46 -0400
From:   Stefan Berger <stefanb@...ux.vnet.ibm.com>
To:     Vivek Goyal <vgoyal@...hat.com>,
        Stefan Berger <StefanBergerstefanb@...ux.vnet.ibm.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, serge@...lyn.com,
        James.Bottomley@...senPartnership.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/12/2017 01:53 PM, Vivek Goyal wrote:
> On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote:
>
> [..]
>> @@ -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);
>> +
> Hi Stephan,
>
>> +	do {
>> +		kfree(newname);
>> +
>> +		newname = xattr_userns_name(name, userns);
> 					    ^^^
> Will name be pointing to a freed string in second iteration of loop.

Fixing for v3.

>
>> +		if (IS_ERR(newname))
>> +			return PTR_ERR(newname);
>> +
>> +		if (!handler) {
>> +			name = newname;
> Here we assign name and at the beginning of second iteration we free
> newname.
>
> Also I am not sure why do we do this assignment only if handler is NULL.

The handler shouldn't change but this optimization isn't helpful. Fixed 
through this patch:

https://github.com/stefanberger/linux/commit/10828401b29a13f8c56f8fad0c0fb2690e4af878


>
> BTW, I set cap_sys_admin on a file outside usernamespace and then launched
> user namespace (mapping 1000 to 0). And then tried to do getcap on file
> and I am not seeing security.capability set by host. Not sure what am I
> doing wrong. getxattr() seems to return -ENODATA. Still debugging it.

This was a regression due to the bug in the loop. I didn't have a test 
case (with runc) for it, now I do.

>
> Also, have we resovled the question of stacked filesystem like overlayfs.
> There we are switching creds to mounter's creds when doing operations on
> underlying filesystem. I am concenrned does that mean, we will get and
> return security.capability to caller in usernamespace instead of
> security.capability@...=1000.

I would have to test this, otherwise I don't know. I'll try it out with 
Docker.

    Stefan

>
> Vivek
>
>> +			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);
>>   
> Thanks
> Vivek
>