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

From: J. Bruce Fields
Date: Thu Jul 21 2016 - 21:57:30 EST


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