Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1685113 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> - 2017-07-11 17:10 +0200
    Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-11 19:20 +0200
      Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-12 02:20 +0200
        Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-12 02:50 +0200
    Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-12 05:50 +0200
      Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-12 13:40 +0200
        Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-12 19:40 +0200
    Re: [PATCH v2] xattr: Enable security.capability in user  namespaces James Morris <jmorris@namei.org> - 2017-07-12 10:10 +0200
    Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-12 15:40 +0200
      Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-12 19:10 +0200
        Re: [PATCH v2] xattr: Enable security.capability in user  namespaces James Morris <jmorris@namei.org> - 2017-07-13 00:30 +0200
          Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 02:50 +0200
            Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 03:10 +0200
        Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 01:30 +0200
          Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-13 02:50 +0200
            Re: [PATCH v2] xattr: Enable security.capability in user namespaces Theodore Ts'o <tytso@mit.edu> - 2017-07-13 03:20 +0200
              Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 04:40 +0200
              Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 14:20 +0200
                Re: [PATCH v2] xattr: Enable security.capability in user namespaces Theodore Ts'o <tytso@mit.edu> - 2017-07-13 18:50 +0200
                  Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-13 19:10 +0200
                    Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 19:50 +0200
                      Re: [PATCH v2] xattr: Enable security.capability in user namespaces Theodore Ts'o <tytso@mit.edu> - 2017-07-13 21:20 +0200
                        Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 21:50 +0200
                      Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 23:20 +0200
                    Re: [PATCH v2] xattr: Enable security.capability in user  namespaces James Morris <jmorris@namei.org> - 2017-07-18 09:10 +0200
                      Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-18 14:20 +0200
                        Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-18 15:40 +0200
                          Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-19 01:20 +0200
                  Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 19:30 +0200
                    Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-13 19:40 +0200
                      Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 20:00 +0200
                        Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 21:50 +0200
                          Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-13 23:30 +0200
                        Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-14 02:50 +0200
                          Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-14 13:40 +0200
                            Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-14 14:20 +0200
                              Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-14 14:50 +0200
                            Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-14 15:40 +0200
                              Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-14 17:30 +0200
                                Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-14 19:40 +0200
                                  Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-14 20:30 +0200
                                  Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-14 20:50 +0200
                                    Re: [PATCH v2] xattr: Enable security.capability in user namespaces James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-07-14 21:00 +0200
                                      Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-14 22:10 +0200
                                        Re: [PATCH v2] xattr: Enable security.capability in user namespaces James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-07-14 22:50 +0200
                                          Re: [PATCH v2] xattr: Enable security.capability in user namespaces Theodore Ts'o <tytso@mit.edu> - 2017-07-14 23:40 +0200
                                            Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-15 01:40 +0200
                                          Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-15 01:40 +0200
                                          Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-15 02:10 +0200
                                    Re: [PATCH v2] xattr: Enable security.capability in user namespaces Theodore Ts'o <tytso@mit.edu> - 2017-07-14 21:30 +0200
                                      Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-14 21:50 +0200
                                  Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-14 21:30 +0200
                                    Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-15 02:20 +0200
                                    Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-16 13:30 +0200
                                    Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-26 05:10 +0200
                                      Re: [PATCH v2] xattr: Enable security.capability in user namespaces Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-26 16:00 +0200
                                Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-14 21:00 +0200
                                  Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-14 21:30 +0200
                      Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 23:30 +0200
                  Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 23:20 +0200
          Re: [PATCH v2] xattr: Enable security.capability in user namespaces "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-13 02:50 +0200
    Re: [PATCH v2] xattr: Enable security.capability in user namespaces Vivek Goyal <vgoyal@redhat.com> - 2017-07-12 20:00 +0200
      Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-12 21:20 +0200
    Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-15 01:50 +0200
      Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-15 23:30 +0200
    Re: [PATCH v2] xattr: Enable security.capability in user namespaces Vivek Goyal <vgoyal@redhat.com> - 2017-07-17 21:00 +0200
      Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-17 23:00 +0200
        Re: [PATCH v2] xattr: Enable security.capability in user namespaces Vivek Goyal <vgoyal@redhat.com> - 2017-07-18 13:50 +0200
          Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-18 14:10 +0200
            Re: [PATCH v2] xattr: Enable security.capability in user namespaces Vivek Goyal <vgoyal@redhat.com> - 2017-07-18 14:40 +0200
              Re: [PATCH v2] xattr: Enable security.capability in user namespaces Vivek Goyal <vgoyal@redhat.com> - 2017-07-18 14:40 +0200
                Re: [PATCH v2] xattr: Enable security.capability in user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2017-07-18 15:40 +0200
              Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-18 15:30 +0200
                Re: [PATCH v2] xattr: Enable security.capability in user namespaces Vivek Goyal <vgoyal@redhat.com> - 2017-07-18 17:00 +0200
                  Re: [PATCH v2] xattr: Enable security.capability in user namespaces Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-18 18:20 +0200

Page 4 of 4 — ← Prev page 1 2 3 [4]

#1686156

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serge@hallyn.com> writes:
> 
> > Quoting Eric W. Biederman (ebiederm@xmission.com):
> >> Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> writes:
> >> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> > Signed-off-by: Serge Hallyn <serge@hallyn.com>
> >> > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> >> 
> >> It doesn't look like this is coming through Serge so I don't see how
> >> the Signed-off-by tag is legtimate.
> >
> > This is mostly explained by the fact that there have been a *lot* of
> > changes, many of them discussed in private emails.
> >
> >> >From the replies to this it doesn't look like Serge has reviewed this
> >> version either.
> >> 
> >> I am disappointed that all of my concerns about technical feasibility
> >> remain unaddressed.
> >
> > Can you re-state those, or give a link to them?
> 
> Well I only posted about one substantive comment on the last round
> so it should be easy to find that said.

Ok so you are likely referring to http://lkml.org/lkml/2017/6/23/551 ,
thanks.  I had actually read that differently when you sent it, and
thought it was more to do with the suggestion of putting the nsid
tags in the middle of the xattr name versus putting it on the end.
As far as that is concerned, note that no other tags besides uid=
are currently supported, and only security.capability is being
namespaced.

> The big question is how does this intereact with filesystems
> xattr implementations?
> 
> There is the potential that we create many more security xattrs this
> way.  How does that scale?  With more names etc.
> What happens if we have one xattr per uid for 1000+ uids?

Well, that's not the intent here.  The goal is *not* to make one fs image
that satisfies 200k possible uid mappings.  The goal is to reconcile the
support for an unprivileged user to set uid agnostic (within the
container) file capabilities with the uid namespace's design goals -
namely root in a container is privileged over the container but
completely unprivileged wrt the host.

This is in part why in my previous version I only allowed a single
namespaced fscap.  But I don't think that we have to enforce a
single fscap - I think it's fair to tell users "go ahead and shoot
yourself in the foot" performance-wise, if they insist on doing
this.

The goal of now putting the root kuid in the name is not to support
multiple containers, but to have common code supporting
security.capability and security.ima, and maybe a few more.

> How does this interact with filesystems optimization of xattr names?
> For some filesystems they optmize the xattr names, and don't store the
> entire thing.

This I have no idea on.  Stefan, have you looked at this?

> > I'd really like to get to a point where unprivileged containers can start
> > using filecaps - at this point if that means having an extra temporary
> > file format based on my earlier patchset while we hash this out, that
> > actually seems worthwhile.  But it would of course be ideal if we could
> > do the name based caps right in the first place.
> 
> This whole new version has set my review back to square one
> unfortunately.

Well it is a whole new approach and whole new patch, so of course that's
to be expected :(

-serge

[toc] | [prev] | [next] | [standalone]

#1685937

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. 

> +		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.

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.

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@uid=1000. 

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

[toc] | [prev] | [next] | [standalone]

#1685986

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@uid=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
>

[toc] | [prev] | [next] | [standalone]

#1687725

Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> writes:

> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>
> This patch enables security.capability in user namespaces but also
> takes a more general approach to enabling extended attributes in user
> namespaces.
>
> The following rules describe the approach using security.foo as a
> 'user namespace enabled' extended attribute:
>
> Reading of extended attributes:
>
> 1a) Reading security.foo from a user namespace will read
>     security.foo@uid=<uid> of the parent user namespace instead with uid
>     being the mapping of root in that parent user namespace. An
>     exception is if root is mapped to uid 0 on the host, and in this case
>     we will read security.foo directly.
>     --> reading security.foo will read security.foo@uid=1000 for uid
>         mapping of root to 1000.
>
> 1b) If security.foo@uid=<uid> is not available, the security.foo of the
>     parent namespace is tried to be read. This procedure is repeated up to
>     the init user namespace. This step only applies for reading of extended
>     attributes and provides the same behavior as older system where the
>     host's extended attributes applied to user namespaces.
>
> 2) All security.foo@uid=<uid> with valid uid mapping in the user namespace
>    can be read. The uid within the user namespace will be mapped to the
>    corresponding uid on the host and that uid will be used in the name of
>    the extended attribute.
>    -> reading security.foo@uid=1 will read security.foo@uid=1001 for uid
>       mapping of root to 1000, size of at least 2.
>
>    All security.foo@uid=<uid> can be read (by root) on the host with values
>    of <uid> also being subject to checking for valid mappings.
>
> 3) No other security.foo* can be read.
>
> The same rules for reading apply to writing and removing of user
> namespace enabled extended attributes.
>
> When listing extended attributes of a file, only those are presented
> to the user namespace that have a valid mapping. Besides that, names
> of the extended attributes are adjusted to represent the mapping.
> This means that if root is mapped to uid 1000 on the host, the
> security.foo@uid=1000 will be listed as security.foo in the user
> namespace, security.foo@uid=1001 becomes security.foo@uid=1 and so on.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Serge Hallyn <serge@hallyn.com>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
> ---
>  fs/xattr.c               | 509 +++++++++++++++++++++++++++++++++++++++++++++--
>  security/commoncap.c     |  36 +++-
>  security/selinux/hooks.c |   9 +-
>  3 files changed, 523 insertions(+), 31 deletions(-)

I am just going to quickly and publicly point out that as designed this
patch breaks evm inode metadata signing.  As evm_config_xattrnames is not
updated.

While not completely insurmountable that seems like a strong limitation of
this design.

Eric

[toc] | [prev] | [next] | [standalone]

#1688013

On 07/14/2017 07:41 PM, Eric W. Biederman wrote:
> Stefan Berger <"Stefan Bergerstefanb"@linux.vnet.ibm.com> writes:
>
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>> This patch enables security.capability in user namespaces but also
>> takes a more general approach to enabling extended attributes in user
>> namespaces.
>>
>> The following rules describe the approach using security.foo as a
>> 'user namespace enabled' extended attribute:
>>
>> Reading of extended attributes:
>>
>> 1a) Reading security.foo from a user namespace will read
>>      security.foo@uid=<uid> of the parent user namespace instead with uid
>>      being the mapping of root in that parent user namespace. An
>>      exception is if root is mapped to uid 0 on the host, and in this case
>>      we will read security.foo directly.
>>      --> reading security.foo will read security.foo@uid=1000 for uid
>>          mapping of root to 1000.
>>
>> 1b) If security.foo@uid=<uid> is not available, the security.foo of the
>>      parent namespace is tried to be read. This procedure is repeated up to
>>      the init user namespace. This step only applies for reading of extended
>>      attributes and provides the same behavior as older system where the
>>      host's extended attributes applied to user namespaces.
>>
>> 2) All security.foo@uid=<uid> with valid uid mapping in the user namespace
>>     can be read. The uid within the user namespace will be mapped to the
>>     corresponding uid on the host and that uid will be used in the name of
>>     the extended attribute.
>>     -> reading security.foo@uid=1 will read security.foo@uid=1001 for uid
>>        mapping of root to 1000, size of at least 2.
>>
>>     All security.foo@uid=<uid> can be read (by root) on the host with values
>>     of <uid> also being subject to checking for valid mappings.
>>
>> 3) No other security.foo* can be read.
>>
>> The same rules for reading apply to writing and removing of user
>> namespace enabled extended attributes.
>>
>> When listing extended attributes of a file, only those are presented
>> to the user namespace that have a valid mapping. Besides that, names
>> of the extended attributes are adjusted to represent the mapping.
>> This means that if root is mapped to uid 1000 on the host, the
>> security.foo@uid=1000 will be listed as security.foo in the user
>> namespace, security.foo@uid=1001 becomes security.foo@uid=1 and so on.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Signed-off-by: Serge Hallyn <serge@hallyn.com>
>> Reviewed-by: Serge Hallyn <serge@hallyn.com>
>> ---
>>   fs/xattr.c               | 509 +++++++++++++++++++++++++++++++++++++++++++++--
>>   security/commoncap.c     |  36 +++-
>>   security/selinux/hooks.c |   9 +-
>>   3 files changed, 523 insertions(+), 31 deletions(-)
> I am just going to quickly and publicly point out that as designed this
> patch breaks evm inode metadata signing.  As evm_config_xattrnames is not
> updated.
>
> While not completely insurmountable that seems like a strong limitation of
> this design.

EVM could be converted to get the list of xattrs and prefix-compare it 
against the evm_config_xattrnames to do what it does now.

    Stefan

[toc] | [prev] | [next] | [standalone]

#1689361

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?

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.

Looks like that's the intent of "!list" condition here. Just wanted to
make sure, hence asking.

BTW, I am testing this with overlayfs and trying to figure out if
switching of creds will create issues. Simple operations like listxattr
and getxattr and setxattr so far worked for me. And reason seems to be
that name transformation we are doing in top layer based on creds of
caller (and not based on creds of mounter). 

Vivek

[toc] | [prev] | [next] | [standalone]

#1689462

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.


>
> Looks like that's the intent of "!list" condition here. Just wanted to
> make sure, hence asking.

Thanks for asking. I thought I had this case covered, but obviously I 
did not.

>
> BTW, I am testing this with overlayfs and trying to figure out if
> switching of creds will create issues. Simple operations like listxattr
> and getxattr and setxattr so far worked for me. And reason seems to be
> that name transformation we are doing in top layer based on creds of
> caller (and not based on creds of mounter).
>
> Vivek
>

    Stefan

[toc] | [prev] | [next] | [standalone]

#1690197

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
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

[toc] | [prev] | [next] | [standalone]

#1690206

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
>

[toc] | [prev] | [next] | [standalone]

#1690231

On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote:
> 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.

I am probably missing something, but for the case of size==0, we don't
have to copy all xattrs. We just need to determine size. So we can walk
through each xattr, remap it and add to the size. I mean there should not
be a need to allocate this 65K buffer. Just enough space needed to be
able to store remapped xattr.

You are already doing it in xattr_parse_uid_from_kuid(). It returns the
buffer containing remapped xattr. So we should be able to just determine
the size and free the buffer. And do it for all the xattrs returned by
filesystem.

What am I missing?

Vivek

> 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
> > 

[toc] | [prev] | [next] | [standalone]

#1690233

On Tue, Jul 18, 2017 at 08:30:09AM -0400, Vivek Goyal wrote:
> On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote:
> > 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.
> 
> I am probably missing something, but for the case of size==0, we don't
> have to copy all xattrs. We just need to determine size. So we can walk
> through each xattr, remap it and add to the size. I mean there should not
> be a need to allocate this 65K buffer. Just enough space needed to be
> able to store remapped xattr.
> 
> You are already doing it in xattr_parse_uid_from_kuid(). It returns the
> buffer containing remapped xattr. So we should be able to just determine
> the size and free the buffer. And do it for all the xattrs returned by
> filesystem.
> 
> What am I missing?

Oh, I think I get it. If I don't pass a buffer to underlying driver, then
it will just return the size (and not actual list). So that's why you are
allocating that big buffer and getting the whole list internally, doing
mapping and returning size to user space. Hmm...

Vivek

[toc] | [prev] | [next] | [standalone]

#1690267

Vivek Goyal <vgoyal@redhat.com> writes:

> On Tue, Jul 18, 2017 at 08:30:09AM -0400, Vivek Goyal wrote:
>> On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote:
>> > 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.
>> 
>> I am probably missing something, but for the case of size==0, we don't
>> have to copy all xattrs. We just need to determine size. So we can walk
>> through each xattr, remap it and add to the size. I mean there should not
>> be a need to allocate this 65K buffer. Just enough space needed to be
>> able to store remapped xattr.
>> 
>> You are already doing it in xattr_parse_uid_from_kuid(). It returns the
>> buffer containing remapped xattr. So we should be able to just determine
>> the size and free the buffer. And do it for all the xattrs returned by
>> filesystem.
>> 
>> What am I missing?
>
> Oh, I think I get it. If I don't pass a buffer to underlying driver, then
> it will just return the size (and not actual list). So that's why you are
> allocating that big buffer and getting the whole list internally, doing
> mapping and returning size to user space. Hmm...

A valid reason to be leary of storing attributs in the xattrs.

Eric

[toc] | [prev] | [next] | [standalone]

#1690266

On 07/18/2017 08:30 AM, Vivek Goyal wrote:
> On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote:
>> 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.
> I am probably missing something, but for the case of size==0, we don't
> have to copy all xattrs. We just need to determine size. So we can walk
> through each xattr, remap it and add to the size. I mean there should not
> be a need to allocate this 65K buffer. Just enough space needed to be
> able to store remapped xattr.
>
> You are already doing it in xattr_parse_uid_from_kuid(). It returns the
> buffer containing remapped xattr. So we should be able to just determine
> the size and free the buffer. And do it for all the xattrs returned by
> filesystem.
>
> What am I missing?

The problem is that each filesystem has a function that collects the 
xattr names. These functions only return the needed size if size==0 and 
don't write anything into a buffer. If the buffer is empty or there is 
no buffer, I have nothing to remap and calculate size for. So I pass a 
buffer large enough to hold the xattr names to the filesystem functions 
so I can then subsequently walk the xattrs and remap them. The remapping 
only needs to be done in non-init_user_ns since there the uid parts 
(@uid=1000) may need to be rewritten and most importantly, the size of 
the needed buffer can increase, depending on how the uid mappings are.

I don't want to extend every filesystem's xattr name gathering function...


    Stefan



>
> Vivek
>
>> 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
>>>

[toc] | [prev] | [next] | [standalone]

#1690346

On Tue, Jul 18, 2017 at 09:21:22AM -0400, Stefan Berger wrote:
> On 07/18/2017 08:30 AM, Vivek Goyal wrote:
> > On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote:
> > > 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.
> > I am probably missing something, but for the case of size==0, we don't
> > have to copy all xattrs. We just need to determine size. So we can walk
> > through each xattr, remap it and add to the size. I mean there should not
> > be a need to allocate this 65K buffer. Just enough space needed to be
> > able to store remapped xattr.
> > 
> > You are already doing it in xattr_parse_uid_from_kuid(). It returns the
> > buffer containing remapped xattr. So we should be able to just determine
> > the size and free the buffer. And do it for all the xattrs returned by
> > filesystem.
> > 
> > What am I missing?
> 
> The problem is that each filesystem has a function that collects the xattr
> names. These functions only return the needed size if size==0 and don't
> write anything into a buffer. If the buffer is empty or there is no buffer,
> I have nothing to remap and calculate size for.

How about calling listxattr() twice. In the first call you will get the
size of buffer to allocate. Allocate that buffer and call ->listxattr()
again, this time passing that buffer? That way you will not have to
hardcode the size of buffer.

Vivek

[toc] | [prev] | [next] | [standalone]

#1690413

On 07/18/2017 10:57 AM, Vivek Goyal wrote:
> On Tue, Jul 18, 2017 at 09:21:22AM -0400, Stefan Berger wrote:
>> On 07/18/2017 08:30 AM, Vivek Goyal wrote:
>>> On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote:
>>>> 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.
>>> I am probably missing something, but for the case of size==0, we don't
>>> have to copy all xattrs. We just need to determine size. So we can walk
>>> through each xattr, remap it and add to the size. I mean there should not
>>> be a need to allocate this 65K buffer. Just enough space needed to be
>>> able to store remapped xattr.
>>>
>>> You are already doing it in xattr_parse_uid_from_kuid(). It returns the
>>> buffer containing remapped xattr. So we should be able to just determine
>>> the size and free the buffer. And do it for all the xattrs returned by
>>> filesystem.
>>>
>>> What am I missing?
>> The problem is that each filesystem has a function that collects the xattr
>> names. These functions only return the needed size if size==0 and don't
>> write anything into a buffer. If the buffer is empty or there is no buffer,
>> I have nothing to remap and calculate size for.
> How about calling listxattr() twice. In the first call you will get the
> size of buffer to allocate. Allocate that buffer and call ->listxattr()
> again, this time passing that buffer? That way you will not have to
> hardcode the size of buffer.

Good idea. I modified the code to do this now. Thanks.

    Stefan

[toc] | [prev] | [standalone]

Page 4 of 4 — ← Prev page 1 2 3 [4]

Back to top | Article view | linux.kernel

csiph-web