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

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Jul 21, 2016 at 04:37:40PM -0400, Oleg Drokin wrote:
> 
> 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@linuxhacker.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?

No, you're right, for some reason I thought that the check for a
positive inode didn't happen till later.  But actually the logic is
basically:

	lock inode
	lookup_one_len
	return nfserr_exist if looked up dentry is positive.
	check for create permission
	vfs_create

So, yes, the initial MAY_EXEC test's needed to prevent that information
leak.

That said... I wonder why it's done that way?  Seems to me we could just
tremove that nfserr_exist check and the vfs would handle it for us....
I'll try that.

--b.

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