diff mbox series

nfsd4: readdirplus shouldn't return parent of export

Message ID 20210312182309.12421-2-tim.gardner@canonical.com
State New
Headers show
Series nfsd4: readdirplus shouldn't return parent of export | expand

Commit Message

Tim Gardner March 12, 2021, 6:23 p.m. UTC
From: "J. Bruce Fields" <bfields@redhat.com>

CVE-2021-3178

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: Youjipeng <wangzhibei1999@gmail.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
(cherry picked from commit 51b2ee7d006a736a9126e8111d1f24e4fd0afaa6)
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 fs/nfsd/nfs3xdr.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Stefan Bader March 15, 2021, 10:41 a.m. UTC | #1
On 12.03.21 19:23, Tim Gardner wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> CVE-2021-3178
> 
> 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: Youjipeng <wangzhibei1999@gmail.com>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> (cherry picked from commit 51b2ee7d006a736a9126e8111d1f24e4fd0afaa6)
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.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 aae514d40b64..1a9e177be158 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -849,9 +849,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
> +			 * the filesystem or export root:
> +			 */
>   			if (dchild == dparent)
>   				goto out;
> +			if (dparent == exp->ex_path.dentry)
> +				goto out;
>   		} else
>   			dchild = dget(dparent);
>   	} else
>
Kleber Sacilotto de Souza March 23, 2021, 10:06 a.m. UTC | #2
On 12.03.21 19:23, Tim Gardner wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> CVE-2021-3178
> 
> 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: Youjipeng <wangzhibei1999@gmail.com>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> (cherry picked from commit 51b2ee7d006a736a9126e8111d1f24e4fd0afaa6)
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.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 aae514d40b64..1a9e177be158 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -849,9 +849,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
> +			 * the filesystem or export root:
> +			 */
>   			if (dchild == dparent)
>   				goto out;
> +			if (dparent == exp->ex_path.dentry)
> +				goto out;
>   		} else
>   			dchild = dget(dparent);
>   	} else
>
diff mbox series

Patch

diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index aae514d40b64..1a9e177be158 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -849,9 +849,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
+			 * the filesystem or export root:
+			 */
 			if (dchild == dparent)
 				goto out;
+			if (dparent == exp->ex_path.dentry)
+				goto out;
 		} else
 			dchild = dget(dparent);
 	} else