sunrpc: Use current_real_cred() when looking up rpc credentials
diff mbox

Message ID 1481821992-77583-1-git-send-email-seth.forshee@canonical.com
State New
Headers show

Commit Message

Seth Forshee Dec. 15, 2016, 5:13 p.m. UTC
Since 4.8 follow_automount() overrides the credentials with
&init_cred before calling d_automount(). When
rpcauth_lookupcred() is called in this context it is now using
fs[ug]id from the override creds instead of from the user's
creds, which can cause authentication to fail. To fix this, take
the ids from current_real_cred() instead.

Cc: stable@vger.kernel.org # v4.8+
CC: Eric W. Biederman <ebiederm@xmission.com>
Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems creds")
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 net/sunrpc/auth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Trond Myklebust Dec. 15, 2016, 11:01 p.m. UTC | #1
On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
> Since 4.8 follow_automount() overrides the credentials with

> &init_cred before calling d_automount(). When

> rpcauth_lookupcred() is called in this context it is now using

> fs[ug]id from the override creds instead of from the user's

> creds, which can cause authentication to fail. To fix this, take

> the ids from current_real_cred() instead.

> 

> Cc: stable@vger.kernel.org # v4.8+

> CC: Eric W. Biederman <ebiederm@xmission.com>

> Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems

> creds")

> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>

> ---

>  net/sunrpc/auth.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c

> index 2bff63a73cf8..e6197b2bda86 100644

> --- a/net/sunrpc/auth.c

> +++ b/net/sunrpc/auth.c

> @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int

> flags)

>  {

>  	struct auth_cred acred;

>  	struct rpc_cred *ret;

> -	const struct cred *cred = current_cred();

> +	const struct cred *cred = current_real_cred();

>  

>  	dprintk("RPC:       looking up %s cred\n",

>  		auth->au_ops->au_name);


Among other things, this will break the access() syscall. It's
completely the wrong level in which to override credentials.

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com
Seth Forshee Dec. 16, 2016, 1:06 p.m. UTC | #2
On Thu, Dec 15, 2016 at 11:01:41PM +0000, Trond Myklebust wrote:
> On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
> > Since 4.8 follow_automount() overrides the credentials with
> > &init_cred before calling d_automount(). When
> > rpcauth_lookupcred() is called in this context it is now using
> > fs[ug]id from the override creds instead of from the user's
> > creds, which can cause authentication to fail. To fix this, take
> > the ids from current_real_cred() instead.
> > 
> > Cc: stable@vger.kernel.org # v4.8+
> > CC: Eric W. Biederman <ebiederm@xmission.com>
> > Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
> > creds")
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> >  net/sunrpc/auth.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> > index 2bff63a73cf8..e6197b2bda86 100644
> > --- a/net/sunrpc/auth.c
> > +++ b/net/sunrpc/auth.c
> > @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int
> > flags)
> >  {
> >  	struct auth_cred acred;
> >  	struct rpc_cred *ret;
> > -	const struct cred *cred = current_cred();
> > +	const struct cred *cred = current_real_cred();
> >  
> >  	dprintk("RPC:       looking up %s cred\n",
> >  		auth->au_ops->au_name);
> 
> Among other things, this will break the access() syscall.

Okay, I see that now.

> It's completely the wrong level in which to override credentials.

The reason for it is that sget() now has a capability check which will
fail on automount if current doesn't have CAP_SYS_ADMIN. So what are the
alternatives? A few ideas:

 - Instead of using a completely differnet set of creds, we could copy
   the current creds and raise CAP_SYS_ADMIN. This won't work if
   curreent is in a different user ns however.

 - Filesystems could get around the capability check by using
   sget_userns() during automount.

 - We could add a mount flag, say MS_AUTOMOUNT, and skip the capability
   check if that is set.

Any opinions or other ideas?

Thanks,
Seth
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seth Forshee Jan. 10, 2017, 2:55 p.m. UTC | #3
On Fri, Dec 16, 2016 at 07:06:09AM -0600, Seth Forshee wrote:
> On Thu, Dec 15, 2016 at 11:01:41PM +0000, Trond Myklebust wrote:
> > On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
> > > Since 4.8 follow_automount() overrides the credentials with
> > > &init_cred before calling d_automount(). When
> > > rpcauth_lookupcred() is called in this context it is now using
> > > fs[ug]id from the override creds instead of from the user's
> > > creds, which can cause authentication to fail. To fix this, take
> > > the ids from current_real_cred() instead.
> > > 
> > > Cc: stable@vger.kernel.org # v4.8+
> > > CC: Eric W. Biederman <ebiederm@xmission.com>
> > > Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
> > > creds")
> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > > ---
> > >  net/sunrpc/auth.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> > > index 2bff63a73cf8..e6197b2bda86 100644
> > > --- a/net/sunrpc/auth.c
> > > +++ b/net/sunrpc/auth.c
> > > @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int
> > > flags)
> > >  {
> > >  	struct auth_cred acred;
> > >  	struct rpc_cred *ret;
> > > -	const struct cred *cred = current_cred();
> > > +	const struct cred *cred = current_real_cred();
> > >  
> > >  	dprintk("RPC:       looking up %s cred\n",
> > >  		auth->au_ops->au_name);
> > 
> > Among other things, this will break the access() syscall.
> 
> Okay, I see that now.
> 
> > It's completely the wrong level in which to override credentials.
> 
> The reason for it is that sget() now has a capability check which will
> fail on automount if current doesn't have CAP_SYS_ADMIN. So what are the
> alternatives? A few ideas:
> 
>  - Instead of using a completely differnet set of creds, we could copy
>    the current creds and raise CAP_SYS_ADMIN. This won't work if
>    curreent is in a different user ns however.
> 
>  - Filesystems could get around the capability check by using
>    sget_userns() during automount.
> 
>  - We could add a mount flag, say MS_AUTOMOUNT, and skip the capability
>    check if that is set.
> 
> Any opinions or other ideas?

I haven't seen any responses, possibly just got lost in the shuffle
during the holidays (I know it slipped my mind for a while).

Eric, what do you think about the last option above? From what I can see
looking up rpc credentials just isn't going to work with current_cred
overridden as we're doing for automount.

Thanks,
Seth
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman Jan. 11, 2017, 12:21 a.m. UTC | #4
Seth Forshee <seth.forshee@canonical.com> writes:

> On Fri, Dec 16, 2016 at 07:06:09AM -0600, Seth Forshee wrote:
>> On Thu, Dec 15, 2016 at 11:01:41PM +0000, Trond Myklebust wrote:
>> > On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
>> > > Since 4.8 follow_automount() overrides the credentials with
>> > > &init_cred before calling d_automount(). When
>> > > rpcauth_lookupcred() is called in this context it is now using
>> > > fs[ug]id from the override creds instead of from the user's
>> > > creds, which can cause authentication to fail. To fix this, take
>> > > the ids from current_real_cred() instead.
>> > > 
>> > > Cc: stable@vger.kernel.org # v4.8+
>> > > CC: Eric W. Biederman <ebiederm@xmission.com>
>> > > Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
>> > > creds")
>> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>> > > ---
>> > >  net/sunrpc/auth.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > 
>> > > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
>> > > index 2bff63a73cf8..e6197b2bda86 100644
>> > > --- a/net/sunrpc/auth.c
>> > > +++ b/net/sunrpc/auth.c
>> > > @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int
>> > > flags)
>> > >  {
>> > >  	struct auth_cred acred;
>> > >  	struct rpc_cred *ret;
>> > > -	const struct cred *cred = current_cred();
>> > > +	const struct cred *cred = current_real_cred();
>> > >  
>> > >  	dprintk("RPC:       looking up %s cred\n",
>> > >  		auth->au_ops->au_name);
>> > 
>> > Among other things, this will break the access() syscall.
>> 
>> Okay, I see that now.
>> 
>> > It's completely the wrong level in which to override credentials.
>> 
>> The reason for it is that sget() now has a capability check which will
>> fail on automount if current doesn't have CAP_SYS_ADMIN. So what are the
>> alternatives? A few ideas:
>> 
>>  - Instead of using a completely differnet set of creds, we could copy
>>    the current creds and raise CAP_SYS_ADMIN. This won't work if
>>    curreent is in a different user ns however.
>> 
>>  - Filesystems could get around the capability check by using
>>    sget_userns() during automount.
>> 
>>  - We could add a mount flag, say MS_AUTOMOUNT, and skip the capability
>>    check if that is set.
>> 
>> Any opinions or other ideas?
>
> I haven't seen any responses, possibly just got lost in the shuffle
> during the holidays (I know it slipped my mind for a while).
>
> Eric, what do you think about the last option above? From what I can see
> looking up rpc credentials just isn't going to work with current_cred
> overridden as we're doing for automount.

I got as far as there wasn't a correct thing to apply, and I have been
bogged down in enough other things that I haven't gotten back to this
one.

My gut feel is that we propbably want to do a little more reworking on
the autmount path.  But I don't exactly have a concrete proposal for
you at the moment.  I just found another 10 year old bug in the mount
code...

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seth Forshee Jan. 24, 2017, 3:17 p.m. UTC | #5
On Wed, Jan 11, 2017 at 01:21:57PM +1300, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > On Fri, Dec 16, 2016 at 07:06:09AM -0600, Seth Forshee wrote:
> >> On Thu, Dec 15, 2016 at 11:01:41PM +0000, Trond Myklebust wrote:
> >> > On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
> >> > > Since 4.8 follow_automount() overrides the credentials with
> >> > > &init_cred before calling d_automount(). When
> >> > > rpcauth_lookupcred() is called in this context it is now using
> >> > > fs[ug]id from the override creds instead of from the user's
> >> > > creds, which can cause authentication to fail. To fix this, take
> >> > > the ids from current_real_cred() instead.
> >> > > 
> >> > > Cc: stable@vger.kernel.org # v4.8+
> >> > > CC: Eric W. Biederman <ebiederm@xmission.com>
> >> > > Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
> >> > > creds")
> >> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> >> > > ---
> >> > >  net/sunrpc/auth.c | 2 +-
> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > > 
> >> > > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> >> > > index 2bff63a73cf8..e6197b2bda86 100644
> >> > > --- a/net/sunrpc/auth.c
> >> > > +++ b/net/sunrpc/auth.c
> >> > > @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int
> >> > > flags)
> >> > >  {
> >> > >  	struct auth_cred acred;
> >> > >  	struct rpc_cred *ret;
> >> > > -	const struct cred *cred = current_cred();
> >> > > +	const struct cred *cred = current_real_cred();
> >> > >  
> >> > >  	dprintk("RPC:       looking up %s cred\n",
> >> > >  		auth->au_ops->au_name);
> >> > 
> >> > Among other things, this will break the access() syscall.
> >> 
> >> Okay, I see that now.
> >> 
> >> > It's completely the wrong level in which to override credentials.
> >> 
> >> The reason for it is that sget() now has a capability check which will
> >> fail on automount if current doesn't have CAP_SYS_ADMIN. So what are the
> >> alternatives? A few ideas:
> >> 
> >>  - Instead of using a completely differnet set of creds, we could copy
> >>    the current creds and raise CAP_SYS_ADMIN. This won't work if
> >>    curreent is in a different user ns however.
> >> 
> >>  - Filesystems could get around the capability check by using
> >>    sget_userns() during automount.
> >> 
> >>  - We could add a mount flag, say MS_AUTOMOUNT, and skip the capability
> >>    check if that is set.
> >> 
> >> Any opinions or other ideas?
> >
> > I haven't seen any responses, possibly just got lost in the shuffle
> > during the holidays (I know it slipped my mind for a while).
> >
> > Eric, what do you think about the last option above? From what I can see
> > looking up rpc credentials just isn't going to work with current_cred
> > overridden as we're doing for automount.
> 
> I got as far as there wasn't a correct thing to apply, and I have been
> bogged down in enough other things that I haven't gotten back to this
> one.
> 
> My gut feel is that we propbably want to do a little more reworking on
> the autmount path.  But I don't exactly have a concrete proposal for
> you at the moment.  I just found another 10 year old bug in the mount
> code...

With automounts we're mounting based on the credentials used when
mounting the parent. That's what your patch "fs: Call d_automount with
the filesystems creds" did, but it's overriding the credentials too
early in the call stack and causing these rpcauth problems.

But I think we should also be setting the submount's s_user_ns to be the
same as the parent's, not to current_user_ns. At that point we'd be
using the same credentials and user ns as when mounting the parent super
block, so we know the capability check would pass (since it passed for
the original mount) and don't really need to do it.

So if we could pass down through the call stack that a given mount
request is an automount from super block s (or at dentry d) then the fix
would be trivial. I don't see any way of passing that information
through currently though, without doing something undesirable like
adding arguments to the mount filesystem op.

Seth
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman Jan. 24, 2017, 10:55 p.m. UTC | #6
Seth Forshee <seth.forshee@canonical.com> writes:

> On Wed, Jan 11, 2017 at 01:21:57PM +1300, Eric W. Biederman wrote:
>> Seth Forshee <seth.forshee@canonical.com> writes:
>> 
>> > On Fri, Dec 16, 2016 at 07:06:09AM -0600, Seth Forshee wrote:
>> >> On Thu, Dec 15, 2016 at 11:01:41PM +0000, Trond Myklebust wrote:
>> >> > On Thu, 2016-12-15 at 11:13 -0600, Seth Forshee wrote:
>> >> > > Since 4.8 follow_automount() overrides the credentials with
>> >> > > &init_cred before calling d_automount(). When
>> >> > > rpcauth_lookupcred() is called in this context it is now using
>> >> > > fs[ug]id from the override creds instead of from the user's
>> >> > > creds, which can cause authentication to fail. To fix this, take
>> >> > > the ids from current_real_cred() instead.
>> >> > > 
>> >> > > Cc: stable@vger.kernel.org # v4.8+
>> >> > > CC: Eric W. Biederman <ebiederm@xmission.com>
>> >> > > Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems
>> >> > > creds")
>> >> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>> >> > > ---
>> >> > >  net/sunrpc/auth.c | 2 +-
>> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> > > 
>> >> > > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
>> >> > > index 2bff63a73cf8..e6197b2bda86 100644
>> >> > > --- a/net/sunrpc/auth.c
>> >> > > +++ b/net/sunrpc/auth.c
>> >> > > @@ -622,7 +622,7 @@ rpcauth_lookupcred(struct rpc_auth *auth, int
>> >> > > flags)
>> >> > >  {
>> >> > >  	struct auth_cred acred;
>> >> > >  	struct rpc_cred *ret;
>> >> > > -	const struct cred *cred = current_cred();
>> >> > > +	const struct cred *cred = current_real_cred();
>> >> > >  
>> >> > >  	dprintk("RPC:       looking up %s cred\n",
>> >> > >  		auth->au_ops->au_name);
>> >> > 
>> >> > Among other things, this will break the access() syscall.
>> >> 
>> >> Okay, I see that now.
>> >> 
>> >> > It's completely the wrong level in which to override credentials.
>> >> 
>> >> The reason for it is that sget() now has a capability check which will
>> >> fail on automount if current doesn't have CAP_SYS_ADMIN. So what are the
>> >> alternatives? A few ideas:
>> >> 
>> >>  - Instead of using a completely differnet set of creds, we could copy
>> >>    the current creds and raise CAP_SYS_ADMIN. This won't work if
>> >>    curreent is in a different user ns however.
>> >> 
>> >>  - Filesystems could get around the capability check by using
>> >>    sget_userns() during automount.
>> >> 
>> >>  - We could add a mount flag, say MS_AUTOMOUNT, and skip the capability
>> >>    check if that is set.
>> >> 
>> >> Any opinions or other ideas?
>> >
>> > I haven't seen any responses, possibly just got lost in the shuffle
>> > during the holidays (I know it slipped my mind for a while).
>> >
>> > Eric, what do you think about the last option above? From what I can see
>> > looking up rpc credentials just isn't going to work with current_cred
>> > overridden as we're doing for automount.
>> 
>> I got as far as there wasn't a correct thing to apply, and I have been
>> bogged down in enough other things that I haven't gotten back to this
>> one.
>> 
>> My gut feel is that we propbably want to do a little more reworking on
>> the autmount path.  But I don't exactly have a concrete proposal for
>> you at the moment.  I just found another 10 year old bug in the mount
>> code...
>
> With automounts we're mounting based on the credentials used when
> mounting the parent. That's what your patch "fs: Call d_automount with
> the filesystems creds" did, but it's overriding the credentials too
> early in the call stack and causing these rpcauth problems.
>
> But I think we should also be setting the submount's s_user_ns to be the
> same as the parent's, not to current_user_ns. At that point we'd be
> using the same credentials and user ns as when mounting the parent super
> block, so we know the capability check would pass (since it passed for
> the original mount) and don't really need to do it.

If we special case the submount code that would be fine.  Normal sget
needs to continue to use current_user_ns.

> So if we could pass down through the call stack that a given mount
> request is an automount from super block s (or at dentry d) then the fix
> would be trivial. I don't see any way of passing that information
> through currently though, without doing something undesirable like
> adding arguments to the mount filesystem op.

So here are the pieces I have in my thinking about this.
1) We need to capture the cred of the mounter of the filesystem
   like overlayfs does but in general.  Call it s_cred or possibly
   s_mounter_cred.

   That would allow us to remove the hard coded cred in the automount
   path, and would allow unprivileged mounts to eventually use this
   functionality.

2) Al Viro mentioned to me that it would be very nice if the automount
   could coud run in a separate stack because this is one place where
   the stack can get very deep in the vfs.

My gut feel is that solving for those two constraints: the code running
in a separate thread, and the capturing not just s_user_ns but the cred
of the mounter should give us enough constraints to figure out how to
structure the code for long term maintenance.

I especially think the part about using the mounters creds will likely
solve the sunrpc problem.  I could of course be wrong but using the
creds of the process that happened to walk across the mount point seems
completely the wrong thing.  In general it feels wrong to expose which
user triggered the automount to me.  As that means the semantics can
very per user.

Now when we looked at autofs the semantics were in fact varying per
user (ugh).  So NFS may have the same legacy requirements.

But as a general maintainability principle I don't like vfs state that
varies depending upon the user.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman Jan. 24, 2017, 11:28 p.m. UTC | #7
With respect to nfs and automounts.

Does NFS have different automount behavior based on the user
performing the automount?

If NFS does not have different automount behavior depending on the user
we just use the creds of the original mounter of NFS?

If NFS does have different automount behavior depending on the user
(ouch!) we need to go through the call path and see where it makes
sense to over ride things and where it does not.



Seth the fundamental problem with your patch was that you were patching
a location that is used for more just mounts.

I am strongly wishing that we could just change follow_automount from:


	old_cred = override_creds(&init_cred);
	mnt = path->dentry->d_op->d_automount(path);
	revert_creds(old_cred);

to:

	old_cred = override_creds(path->mnt->mnt_sb->s_cred);
	mnt = path->dentry->d_op->d_automount(path);
	revert_creds(old_cred);

And all will be well with nfs.  That does remain possible.

But looking at the code path you touched it seems to lookup the cred
based purely on the local uid, gid, and groups.  Which suggests to
me that even the original mounters creds may not be enough :(

At which point I am not certain of the solution.  But I fear that like
autofs NFS actually cares which user is transition the magic mountpoint,
and may return different data depending on who transitions the
mountpoint first.  Ick!  Nasty Nasty Ick!

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust Jan. 24, 2017, 11:46 p.m. UTC | #8
On Wed, 2017-01-25 at 12:28 +1300, Eric W. Biederman wrote:
> With respect to nfs and automounts.

> 

> Does NFS have different automount behavior based on the user

> performing the automount?

> 

> If NFS does not have different automount behavior depending on the

> user

> we just use the creds of the original mounter of NFS?

> 

> If NFS does have different automount behavior depending on the user

> (ouch!) we need to go through the call path and see where it makes

> sense to over ride things and where it does not.


The reason why the NFS client creates a mountpoint is because on
entering a directory, it detects that there is either a similar
mountpoint on the server, or there is a referral (which acts sort of
like a symlink, except it points to a path on one or more different NFS
servers).
Without that mountpoint, most things would work, but the user would end
up seeing nasty non-posix features like duplicate inode numbers.

We do not want to use any creds other than user creds here, because as
far as the security model is concerned, the process is just crossing
into an existing directory.
> 

> 

> 

> Seth the fundamental problem with your patch was that you were

> patching

> a location that is used for more just mounts.

> 

> I am strongly wishing that we could just change follow_automount

> from:

> 

> 

> 	old_cred = override_creds(&init_cred);

> 	mnt = path->dentry->d_op->d_automount(path);

> 	revert_creds(old_cred);

> 

> to:

> 

> 	old_cred = override_creds(path->mnt->mnt_sb->s_cred);

> 	mnt = path->dentry->d_op->d_automount(path);

> 	revert_creds(old_cred);

> 

> And all will be well with nfs.  That does remain possible.


No. That would break permissions checking. See above.

> 

> But looking at the code path you touched it seems to lookup the cred

> based purely on the local uid, gid, and groups.  Which suggests to

> me that even the original mounters creds may not be enough :(

> 

> At which point I am not certain of the solution.  But I fear that

> like

> autofs NFS actually cares which user is transition the magic

> mountpoint,

> and may return different data depending on who transitions the

> mountpoint first.  Ick!  Nasty Nasty Ick!

> 


The security model is the same as that of an ordinary directory. The
only difference is that we create a new superblock. Why is that "Ick"?

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com
Eric W. Biederman Jan. 24, 2017, 11:56 p.m. UTC | #9
Trond Myklebust <trondmy@primarydata.com> writes:

> On Wed, 2017-01-25 at 12:28 +1300, Eric W. Biederman wrote:
>> With respect to nfs and automounts.
>> 
>> Does NFS have different automount behavior based on the user
>> performing the automount?
>> 
>> If NFS does not have different automount behavior depending on the
>> user
>> we just use the creds of the original mounter of NFS?
>> 
>> If NFS does have different automount behavior depending on the user
>> (ouch!) we need to go through the call path and see where it makes
>> sense to over ride things and where it does not.
>
> The reason why the NFS client creates a mountpoint is because on
> entering a directory, it detects that there is either a similar
> mountpoint on the server, or there is a referral (which acts sort of
> like a symlink, except it points to a path on one or more different NFS
> servers).
> Without that mountpoint, most things would work, but the user would end
> up seeing nasty non-posix features like duplicate inode numbers.
>
> We do not want to use any creds other than user creds here, because as
> far as the security model is concerned, the process is just crossing
> into an existing directory.

But sget needs different creds.

Because the user who authorizes the mounting of the filesystem is
different than the user who is crossing into the new filesystem.

The local system now cares about that distinction even if the nfs server
does not.

>> Seth the fundamental problem with your patch was that you were
>> patching
>> a location that is used for more just mounts.
>> 
>> I am strongly wishing that we could just change follow_automount
>> from:
>> 
>> 
>> 	old_cred = override_creds(&init_cred);
>> 	mnt = path->dentry->d_op->d_automount(path);
>> 	revert_creds(old_cred);
>> 
>> to:
>> 
>> 	old_cred = override_creds(path->mnt->mnt_sb->s_cred);
>> 	mnt = path->dentry->d_op->d_automount(path);
>> 	revert_creds(old_cred);
>> 
>> And all will be well with nfs.  That does remain possible.
>
> No. That would break permissions checking. See above.

Then we need to look much harder at the permission checking
model of d_automount because we need to permission check against
both sets of creds.

>> But looking at the code path you touched it seems to lookup the cred
>> based purely on the local uid, gid, and groups.  Which suggests to
>> me that even the original mounters creds may not be enough :(
>> 
>> At which point I am not certain of the solution.  But I fear that
>> like
>> autofs NFS actually cares which user is transition the magic
>> mountpoint,
>> and may return different data depending on who transitions the
>> mountpoint first.  Ick!  Nasty Nasty Ick!
>> 
>
> The security model is the same as that of an ordinary directory. The
> only difference is that we create a new superblock. Why is that "Ick"?

The Ick is if the superblock that is created depends on the user
crossing the mountpoint.

AKA Do we get a different mountpoint if user A triggers the automount
vs user B triggering the automount?

If the problem is just root squash and not letting root trigger the
automount it is much less ick.  Weird but not ick.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust Jan. 25, 2017, 12:14 a.m. UTC | #10
QWRkaW5nIERhdmlkIEhvd2VsbHMgYW5kIFN0ZXZlIEZyZW5jaCBhcyBJIGJlbGlldmUgYm90aCBB
RlMgYW5kIENJRlMNCmhhdmUgdGhlIGV4YWN0IHNhbWUgcmVxdWlyZW1lbnRzIGFuZCBORlMgaGVy
ZS4NCg0KT24gV2VkLCAyMDE3LTAxLTI1IGF0IDEyOjU2ICsxMzAwLCBFcmljIFcuIEJpZWRlcm1h
biB3cm90ZToNCj4gVHJvbmQgTXlrbGVidXN0IDx0cm9uZG15QHByaW1hcnlkYXRhLmNvbT4gd3Jp
dGVzOg0KPiANCj4gPiBPbiBXZWQsIDIwMTctMDEtMjUgYXQgMTI6MjggKzEzMDAsIEVyaWMgVy4g
QmllZGVybWFuIHdyb3RlOg0KPiA+ID4gV2l0aCByZXNwZWN0IHRvIG5mcyBhbmQgYXV0b21vdW50
cy4NCj4gPiA+IA0KPiA+ID4gRG9lcyBORlMgaGF2ZSBkaWZmZXJlbnQgYXV0b21vdW50IGJlaGF2
aW9yIGJhc2VkIG9uIHRoZSB1c2VyDQo+ID4gPiBwZXJmb3JtaW5nIHRoZSBhdXRvbW91bnQ/DQo+
ID4gPiANCj4gPiA+IElmIE5GUyBkb2VzIG5vdCBoYXZlIGRpZmZlcmVudCBhdXRvbW91bnQgYmVo
YXZpb3IgZGVwZW5kaW5nIG9uDQo+ID4gPiB0aGUNCj4gPiA+IHVzZXINCj4gPiA+IHdlIGp1c3Qg
dXNlIHRoZSBjcmVkcyBvZiB0aGUgb3JpZ2luYWwgbW91bnRlciBvZiBORlM/DQo+ID4gPiANCj4g
PiA+IElmIE5GUyBkb2VzIGhhdmUgZGlmZmVyZW50IGF1dG9tb3VudCBiZWhhdmlvciBkZXBlbmRp
bmcgb24gdGhlDQo+ID4gPiB1c2VyDQo+ID4gPiAob3VjaCEpIHdlIG5lZWQgdG8gZ28gdGhyb3Vn
aCB0aGUgY2FsbCBwYXRoIGFuZCBzZWUgd2hlcmUgaXQNCj4gPiA+IG1ha2VzDQo+ID4gPiBzZW5z
ZSB0byBvdmVyIHJpZGUgdGhpbmdzIGFuZCB3aGVyZSBpdCBkb2VzIG5vdC4NCj4gPiANCj4gPiBU
aGUgcmVhc29uIHdoeSB0aGUgTkZTIGNsaWVudCBjcmVhdGVzIGEgbW91bnRwb2ludCBpcyBiZWNh
dXNlIG9uDQo+ID4gZW50ZXJpbmcgYSBkaXJlY3RvcnksIGl0IGRldGVjdHMgdGhhdCB0aGVyZSBp
cyBlaXRoZXIgYSBzaW1pbGFyDQo+ID4gbW91bnRwb2ludCBvbiB0aGUgc2VydmVyLCBvciB0aGVy
ZSBpcyBhIHJlZmVycmFsICh3aGljaCBhY3RzIHNvcnQNCj4gPiBvZg0KPiA+IGxpa2UgYSBzeW1s
aW5rLCBleGNlcHQgaXQgcG9pbnRzIHRvIGEgcGF0aCBvbiBvbmUgb3IgbW9yZSBkaWZmZXJlbnQN
Cj4gPiBORlMNCj4gPiBzZXJ2ZXJzKS4NCj4gPiBXaXRob3V0IHRoYXQgbW91bnRwb2ludCwgbW9z
dCB0aGluZ3Mgd291bGQgd29yaywgYnV0IHRoZSB1c2VyIHdvdWxkDQo+ID4gZW5kDQo+ID4gdXAg
c2VlaW5nIG5hc3R5IG5vbi1wb3NpeCBmZWF0dXJlcyBsaWtlIGR1cGxpY2F0ZSBpbm9kZSBudW1i
ZXJzLg0KPiA+IA0KPiA+IFdlIGRvIG5vdCB3YW50IHRvIHVzZSBhbnkgY3JlZHMgb3RoZXIgdGhh
biB1c2VyIGNyZWRzIGhlcmUsIGJlY2F1c2UNCj4gPiBhcw0KPiA+IGZhciBhcyB0aGUgc2VjdXJp
dHkgbW9kZWwgaXMgY29uY2VybmVkLCB0aGUgcHJvY2VzcyBpcyBqdXN0DQo+ID4gY3Jvc3NpbmcN
Cj4gPiBpbnRvIGFuIGV4aXN0aW5nIGRpcmVjdG9yeS4NCj4gDQo+IEJ1dCBzZ2V0IG5lZWRzIGRp
ZmZlcmVudCBjcmVkcy4NCj4gDQo+IEJlY2F1c2UgdGhlIHVzZXIgd2hvIGF1dGhvcml6ZXMgdGhl
IG1vdW50aW5nIG9mIHRoZSBmaWxlc3lzdGVtIGlzDQo+IGRpZmZlcmVudCB0aGFuIHRoZSB1c2Vy
IHdobyBpcyBjcm9zc2luZyBpbnRvIHRoZSBuZXcgZmlsZXN5c3RlbS4NCj4gDQo+IFRoZSBsb2Nh
bCBzeXN0ZW0gbm93IGNhcmVzIGFib3V0IHRoYXQgZGlzdGluY3Rpb24gZXZlbiBpZiB0aGUgbmZz
DQo+IHNlcnZlcg0KPiBkb2VzIG5vdC4NCg0KV2h5PyBUaGUgZmlsZXN5c3RlbSBpcyBhbHJlYWR5
IG1vdW50ZWQuIFdlJ3JlIGNyZWF0aW5nIGEgbmV3DQptb3VudHBvaW50LCBidXQgd2UgY291bGQg
ZXF1YWxseSB3ZWxsIGp1c3Qgc2F5ICdzb2QgdGhhdCcgYW5kIGNyZWF0ZSBhbg0Kb3JkaW5hcnkg
ZGlyZWN0b3J5LiBUaGUgcGVuYWx0eSB3b3VsZCBiZSBhZm9yZW1lbnRpb25lZCBub24tcG9zaXgN
CndlaXJkbmVzcy4NCg0KPiANCj4gPiA+IFNldGggdGhlIGZ1bmRhbWVudGFsIHByb2JsZW0gd2l0
aCB5b3VyIHBhdGNoIHdhcyB0aGF0IHlvdSB3ZXJlDQo+ID4gPiBwYXRjaGluZw0KPiA+ID4gYSBs
b2NhdGlvbiB0aGF0IGlzIHVzZWQgZm9yIG1vcmUganVzdCBtb3VudHMuDQo+ID4gPiANCj4gPiA+
IEkgYW0gc3Ryb25nbHkgd2lzaGluZyB0aGF0IHdlIGNvdWxkIGp1c3QgY2hhbmdlIGZvbGxvd19h
dXRvbW91bnQNCj4gPiA+IGZyb206DQo+ID4gPiANCj4gPiA+IA0KPiA+ID4gCW9sZF9jcmVkID0g
b3ZlcnJpZGVfY3JlZHMoJmluaXRfY3JlZCk7DQo+ID4gPiAJbW50ID0gcGF0aC0+ZGVudHJ5LT5k
X29wLT5kX2F1dG9tb3VudChwYXRoKTsNCj4gPiA+IAlyZXZlcnRfY3JlZHMob2xkX2NyZWQpOw0K
PiA+ID4gDQo+ID4gPiB0bzoNCj4gPiA+IA0KPiA+ID4gCW9sZF9jcmVkID0gb3ZlcnJpZGVfY3Jl
ZHMocGF0aC0+bW50LT5tbnRfc2ItPnNfY3JlZCk7DQo+ID4gPiAJbW50ID0gcGF0aC0+ZGVudHJ5
LT5kX29wLT5kX2F1dG9tb3VudChwYXRoKTsNCj4gPiA+IAlyZXZlcnRfY3JlZHMob2xkX2NyZWQp
Ow0KPiA+ID4gDQo+ID4gPiBBbmQgYWxsIHdpbGwgYmUgd2VsbCB3aXRoIG5mcy7CoMKgVGhhdCBk
b2VzIHJlbWFpbiBwb3NzaWJsZS4NCj4gPiANCj4gPiBOby4gVGhhdCB3b3VsZCBicmVhayBwZXJt
aXNzaW9ucyBjaGVja2luZy4gU2VlIGFib3ZlLg0KPiANCj4gVGhlbiB3ZSBuZWVkIHRvIGxvb2sg
bXVjaCBoYXJkZXIgYXQgdGhlIHBlcm1pc3Npb24gY2hlY2tpbmcNCj4gbW9kZWwgb2YgZF9hdXRv
bW91bnQgYmVjYXVzZSB3ZSBuZWVkIHRvIHBlcm1pc3Npb24gY2hlY2sgYWdhaW5zdA0KPiBib3Ro
IHNldHMgb2YgY3JlZHMuDQo+IA0KPiA+ID4gQnV0IGxvb2tpbmcgYXQgdGhlIGNvZGUgcGF0aCB5
b3UgdG91Y2hlZCBpdCBzZWVtcyB0byBsb29rdXAgdGhlDQo+ID4gPiBjcmVkDQo+ID4gPiBiYXNl
ZCBwdXJlbHkgb24gdGhlIGxvY2FsIHVpZCwgZ2lkLCBhbmQgZ3JvdXBzLsKgwqBXaGljaCBzdWdn
ZXN0cw0KPiA+ID4gdG8NCj4gPiA+IG1lIHRoYXQgZXZlbiB0aGUgb3JpZ2luYWwgbW91bnRlcnMg
Y3JlZHMgbWF5IG5vdCBiZSBlbm91Z2ggOigNCj4gPiA+IA0KPiA+ID4gQXQgd2hpY2ggcG9pbnQg
SSBhbSBub3QgY2VydGFpbiBvZiB0aGUgc29sdXRpb24uwqDCoEJ1dCBJIGZlYXIgdGhhdA0KPiA+
ID4gbGlrZQ0KPiA+ID4gYXV0b2ZzIE5GUyBhY3R1YWxseSBjYXJlcyB3aGljaCB1c2VyIGlzIHRy
YW5zaXRpb24gdGhlIG1hZ2ljDQo+ID4gPiBtb3VudHBvaW50LA0KPiA+ID4gYW5kIG1heSByZXR1
cm4gZGlmZmVyZW50IGRhdGEgZGVwZW5kaW5nIG9uIHdobyB0cmFuc2l0aW9ucyB0aGUNCj4gPiA+
IG1vdW50cG9pbnQgZmlyc3QuwqDCoEljayHCoMKgTmFzdHkgTmFzdHkgSWNrIQ0KPiA+ID4gDQo+
ID4gDQo+ID4gVGhlIHNlY3VyaXR5IG1vZGVsIGlzIHRoZSBzYW1lIGFzIHRoYXQgb2YgYW4gb3Jk
aW5hcnkgZGlyZWN0b3J5Lg0KPiA+IFRoZQ0KPiA+IG9ubHkgZGlmZmVyZW5jZSBpcyB0aGF0IHdl
IGNyZWF0ZSBhIG5ldyBzdXBlcmJsb2NrLiBXaHkgaXMgdGhhdA0KPiA+ICJJY2siPw0KPiANCj4g
VGhlIEljayBpcyBpZiB0aGUgc3VwZXJibG9jayB0aGF0IGlzIGNyZWF0ZWQgZGVwZW5kcyBvbiB0
aGUgdXNlcg0KPiBjcm9zc2luZyB0aGUgbW91bnRwb2ludC4NCj4gDQo+IEFLQSBEbyB3ZSBnZXQg
YSBkaWZmZXJlbnQgbW91bnRwb2ludCBpZiB1c2VyIEEgdHJpZ2dlcnMgdGhlIGF1dG9tb3VudA0K
PiB2cyB1c2VyIEIgdHJpZ2dlcmluZyB0aGUgYXV0b21vdW50Pw0KDQpZb3UgbWF5IGdldCBhbiBF
QUNDRVMuIE90aGVyd2lzZSB0aGUgbW91bnRwb2ludHMgc2hvdWxkIGJlIHRoZSBzYW1lLg0KDQo+
IElmIHRoZSBwcm9ibGVtIGlzIGp1c3Qgcm9vdCBzcXVhc2ggYW5kIG5vdCBsZXR0aW5nIHJvb3Qg
dHJpZ2dlciB0aGUNCj4gYXV0b21vdW50IGl0IGlzIG11Y2ggbGVzcyBpY2suwqDCoFdlaXJkIGJ1
dCBub3QgaWNrLg0KDQpOb3QganVzdCByb290IHNxdWFzaCwgYnV0IGdlbmVyYWwgcGVybWlzc2lv
bnMgY2hlY2tpbmcuIFdlIGRvbid0IHdhbnQNCnRvIGdpdmUgdXNlciBCIHJvb3QgcHJpdmlsZWdl
cyBhbGxvd2luZyBjcm9zc2luZyBpbnRvIGEgZGlyZWN0b3J5IHRvDQp3aGljaCBzL2hlIHdvdWxk
IG9yZGluYXJpbHkgaGF2ZSBubyByaWdodHMgZWl0aGVyLg0KDQotLSANClRyb25kIE15a2xlYnVz
dA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5RGF0YQ0KdHJvbmQubXlrbGVi
dXN0QHByaW1hcnlkYXRhLmNvbQ0K

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 2bff63a73cf8..e6197b2bda86 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -622,7 +622,7 @@  rpcauth_lookupcred(struct rpc_auth *auth, int flags)
 {
 	struct auth_cred acred;
 	struct rpc_cred *ret;
-	const struct cred *cred = current_cred();
+	const struct cred *cred = current_real_cred();
 
 	dprintk("RPC:       looking up %s cred\n",
 		auth->au_ops->au_name);