| Message ID | 20210111210129.GA11652@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd4: readdirplus shouldn't return parent of export | expand |
> On Jan 11, 2021, at 4:01 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > If you export a subdirectory of a filesystem, a READDIRPLUS on the root > of that export will return the filehandle of the parent with the ".." > entry. > > The filehandle is optional, so let's just not return the filehandle for > ".." if we're at the root of an export. > > Note that once the client learns one filehandle outside of the export, > they can trivially access the rest of the export using further lookups. > > However, it is also not very difficult to guess filehandles outside of > the export. So exporting a subdirectory of a filesystem should > considered equivalent to providing access to the entire filesystem. To > avoid confusion, we recommend only exporting entire filesystems. > > Reported-by: 吴异 <wangzhibei1999@gmail.com> > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > fs/nfsd/nfs3xdr.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c > index 821db21ba072..34b880211e5e 100644 > --- a/fs/nfsd/nfs3xdr.c > +++ b/fs/nfsd/nfs3xdr.c > @@ -865,9 +865,14 @@ compose_entry_fh(struct nfsd3_readdirres *cd, struct svc_fh *fhp, > if (isdotent(name, namlen)) { > if (namlen == 2) { > dchild = dget_parent(dparent); > - /* filesystem root - cannot return filehandle for ".." */ > + /* > + * Don't return filehandle for ".." if we're at > OA+ * the filesystem or export root: > + */ > if (dchild == dparent) > goto out; > + if (dparent == exp->ex_path.dentry) > + goto out; > } else > dchild = dget(dparent); > } else > -- > 2.29.2 Thanks for the fix! I've replaced the Reported-by: tag and pushed this to my cel-next topic branch, and intend to submit it with the next 5.11 -rc pull request. See: https://git.linux-nfs.org/?p=cel/cel-2.6.git;a=shortlog;h=refs/heads/cel-next Is there additional context that should be added? A Link: tag that points to the discussion on security@ perhaps? Note there was some damage in the patch body: there's a spurious "OA" in the hunk that had to be removed before the patch would apply. -- Chuck Lever
On Tue, Jan 12, 2021 at 08:31:59AM -0500, Chuck Lever wrote: > > > > On Jan 11, 2021, at 4:01 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > If you export a subdirectory of a filesystem, a READDIRPLUS on the root > > of that export will return the filehandle of the parent with the ".." > > entry. > > > > The filehandle is optional, so let's just not return the filehandle for > > ".." if we're at the root of an export. > > > > Note that once the client learns one filehandle outside of the export, > > they can trivially access the rest of the export using further lookups. > > > > However, it is also not very difficult to guess filehandles outside of > > the export. So exporting a subdirectory of a filesystem should > > considered equivalent to providing access to the entire filesystem. To > > avoid confusion, we recommend only exporting entire filesystems. > > > > Reported-by: 吴异 <wangzhibei1999@gmail.com> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > fs/nfsd/nfs3xdr.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c > > index 821db21ba072..34b880211e5e 100644 > > --- a/fs/nfsd/nfs3xdr.c > > +++ b/fs/nfsd/nfs3xdr.c > > @@ -865,9 +865,14 @@ compose_entry_fh(struct nfsd3_readdirres *cd, struct svc_fh *fhp, > > if (isdotent(name, namlen)) { > > if (namlen == 2) { > > dchild = dget_parent(dparent); > > - /* filesystem root - cannot return filehandle for ".." */ > > + /* > > + * Don't return filehandle for ".." if we're at > > OA+ * the filesystem or export root: > > + */ > > if (dchild == dparent) > > goto out; > > + if (dparent == exp->ex_path.dentry) > > + goto out; > > } else > > dchild = dget(dparent); > > } else > > -- > > 2.29.2 > > Thanks for the fix! > > I've replaced the Reported-by: tag and pushed this to my > cel-next topic branch, and intend to submit it with the > next 5.11 -rc pull request. See: > > https://git.linux-nfs.org/?p=cel/cel-2.6.git;a=shortlog;h=refs/heads/cel-next > > Is there additional context that should be added? A Link: > tag that points to the discussion on security@ perhaps? I don't think so. I guess it should get a stable cc: too. > Note there was some damage in the patch body: there's a > spurious "OA" in the hunk that had to be removed before > the patch would apply. Whoops, apologies, I'm not sure how that happened.... --b.
diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c index 821db21ba072..34b880211e5e 100644 --- a/fs/nfsd/nfs3xdr.c +++ b/fs/nfsd/nfs3xdr.c @@ -865,9 +865,14 @@ compose_entry_fh(struct nfsd3_readdirres *cd, struct svc_fh *fhp, if (isdotent(name, namlen)) { if (namlen == 2) { dchild = dget_parent(dparent); - /* filesystem root - cannot return filehandle for ".." */ + /* + * Don't return filehandle for ".." if we're at OA+ * the filesystem or export root: + */ if (dchild == dparent) goto out; + if (dparent == exp->ex_path.dentry) + goto out; } else dchild = dget(dparent); } else