linux-kernel - Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM

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:	Thu, 21 Jul 2016 16:37:40 -0400
From:	Oleg Drokin <green@...uxhacker.ru>
To:	"J. Bruce Fields" <bfields@...ldses.org>
Cc:	Jeff Layton <jlayton@...chiereds.net>, linux-nfs@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nfsd: Make creates return EEXIST correctly instead of EPERM


On Jul 21, 2016, at 4:34 PM, J. Bruce Fields wrote:

> On Fri, Jul 08, 2016 at 05:53:19PM -0400, Oleg Drokin wrote:
>> 
>> On Jul 8, 2016, at 4:54 PM, J. Bruce Fields wrote:
>> 
>>> On Thu, Jul 07, 2016 at 09:47:46PM -0400, Oleg Drokin wrote:
>>>> It looks like we are bit overzealous about failing mkdir/create/mknod
>>>> with permission denied if the parent dir is not writeable.
>>>> Need to make sure the name does not exist first, because we need to
>>>> return EEXIST in that case.
>>>> 
>>>> Signed-off-by: Oleg Drokin <green@...uxhacker.ru>
>>>> ---
>>>> A very similar problem exists with symlinks, but the patch is more
>>>> involved, so assuming this one is ok, I'll send a symlink one separately.
>>>> fs/nfsd/nfs4proc.c |  6 +++++-
>>>> fs/nfsd/vfs.c      | 11 ++++++++++-
>>>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>> index de1ff1d..0067520 100644
>>>> --- a/fs/nfsd/nfs4proc.c
>>>> +++ b/fs/nfsd/nfs4proc.c
>>>> @@ -605,8 +605,12 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>> 
>>>> 	fh_init(&resfh, NFS4_FHSIZE);
>>>> 
>>>> +	/*
>>>> +	 * We just check thta parent is accessible here, nfsd_* do their
>>>> +	 * own access permission checks
>>>> +	 */
>>>> 	status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR,
>>>> -			   NFSD_MAY_CREATE);
>>>> +			   NFSD_MAY_EXEC);
>>>> 	if (status)
>>>> 		return status;
>>>> 
>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>> index 6fbd81e..6a45ec6 100644
>>>> --- a/fs/nfsd/vfs.c
>>>> +++ b/fs/nfsd/vfs.c
>>>> @@ -1161,7 +1161,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>> 	if (isdotent(fname, flen))
>>>> 		goto out;
>>>> 
>>>> -	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
>>>> +	/*
>>>> +	 * Even though it is a create, first we see if we are even allowed
>>>> +	 * to peek inside the parent
>>>> +	 */
>>>> +	err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC);
>>> 
>>> Looks like in the v3 case we haven't actually locked the directory yet
>>> at this point so this check is a little race-prone.
>> 
>> In reality this check is not really needed, I suspect.
>> When we call vfs_create/mknod/mkdir later on, it has it's own permission check
>> anyway so if there was a race and somebody changed dir access in the middle,
>> there's going to be another check anyway and it would be caught.
>> Unless there's some weird server-side permission wiggling as well that makes it
>> ineffective, but I imagine that one cannot really change in a racy way?
> 
> Yeah, I think I'll just change those NFSD_MAY_EXEC's to NFSD_MAY_NOP's.
> We still need the fh_verify there since it's also what does the
> filehandle->dentry translation, but we don't need permission checking
> here yet.

This will likely need an extra test to ensure that when you
do mkdir where you do not have exec permissions, you would get EACCES instead
of EEXIST, otherwise that would be information leakage, no?
Or do you think the second time we do nfsd_permission, that would be covered?

> Applying with that one change.  (And I'll followup with some additional
> minor cleanup of the create code.)
> 
> --b.
> 
>> 
>>> I wonder why the code's structured that way--it's confusing.
>> 
>> Probably years of accumulated "damage" ;)
>> 
>>> --b.
>>> 
>>>> 	if (err)
>>>> 		goto out;
>>>> 
>>>> @@ -1211,6 +1215,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>> 		goto out; 
>>>> 	}
>>>> 
>>>> +	/* Now let's see if we actually have permissions to create */
>>>> +	err = nfsd_permission(rqstp, fhp->fh_export, dentry, NFSD_MAY_CREATE);
>>>> +	if (err)
>>>> +		goto out;
>>>> +
>>>> 	if (!(iap->ia_valid & ATTR_MODE))
>>>> 		iap->ia_mode = 0;
>>>> 	iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type;
>>>> -- 
>>>> 2.7.4