linux-kernel - Re: [PATCH v6 09/40] xattr: handle idmapped mounts

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, 3 Mar 2021 14:05:22 +0000
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     David Howells <dhowells@...hat.com>
Cc:     Tycho Andersen <tycho@...ho.pizza>,
        Tycho Andersen <tycho@...ho.ws>,
        James Morris <jmorris@...ei.org>,
        linux-fsdevel@...r.kernel.org,
        containers@...ts.linux-foundation.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 09/40] xattr: handle idmapped mounts

On Wed, Mar 03, 2021 at 01:24:02PM +0000, David Howells wrote:
> Christian Brauner <christian.brauner@...ntu.com> wrote:
> 
> > diff --git a/fs/cachefiles/xattr.c b/fs/cachefiles/xattr.c
> > index 72e42438f3d7..a591b5e09637 100644
> > --- a/fs/cachefiles/xattr.c
> > +++ b/fs/cachefiles/xattr.c
> > @@ -39,8 +39,8 @@ int cachefiles_check_object_type(struct cachefiles_object *object)
> >  	_enter("%p{%s}", object, type);
> >  
> >  	/* attempt to install a type label directly */
> > -	ret = vfs_setxattr(dentry, cachefiles_xattr_cache, type, 2,
> > -			   XATTR_CREATE);
> > +	ret = vfs_setxattr(&init_user_ns, dentry, cachefiles_xattr_cache, type,
> > +			   2, XATTR_CREATE);
> 

Hey David,

(Ok, recovered from my run-in with the swapfile bug. I even managed to
get my emails back.)

> Actually, on further consideration, this might be the wrong thing to do in
> cachefiles.  The creds are (or should be) overridden when accesses to the
> underlying filesystem are being made.
> 
> I wonder if this should be using current_cred()->user_ns or
> cache->cache_cred->user_ns instead.

Before I go into the second question please note that this is a no-op
change. So if this is wrong it was wrong before. Which is your point, I
guess.

Please also note that the mnt_userns is _never_ used for (capability)
permission checking, only for idmapping vfs objects and permission
checks based on the i_uid and i_gid. So if your argument about passing
one of those two user namespaces above has anything to do with
permission checking on caps it's most likely wrong. :)

In order to answer this more confidently I need to know a bit more about
how cachefiles are supposed to work.

>From what I gather here it seemed what this code is trying to set here
is an internal "CacheFiles.cache" extended attribute on the indode. This
extended attribute doesn't store any uids and gids or filesystem
capabilities so the user namespace isn't relevant for that since there
doesn't need to be any conversion.

What I need to know is what information do you use for cachefiles to
determine whether someone can set that "Cachefiles.cache" extended
attribute on the inode:
- Is it the mnt_userns of a/the mount of the filesystem you're caching for?
- The mnt_userns of the mnt of struct cachefiles_cache?
- Or the stashed or current creds of the caller?

Christian