[PATCH] fs: don't let getdents return bogus names

[PATCH] fs: don't let getdents return bogus names

[Jann Horn]

When you e.g. run `find` on a directory for which getdents returns
"filenames" that contain slashes, `find` passes those "filenames" back to
the kernel, which then interprets them as paths. That could conceivably
cause userspace to do something bad when accessing something like an
untrusted USB stick, but I'm not aware of any specific example.

Instead of returning bogus filenames to userspace, return -EUCLEAN.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
I didn't get any replies to my mail "readdir() and directory names
containing slashes" from a few days ago, so here's a suggested fix.

As often, I'm not entirely sure whether this should be marked for stable
backport.
Does anyone want to bikeshed the choice of error number? ext4 uses
-EUCLEAN (aliased as -EFSCORRUPTED) for signalling filesystem corruption;
I think vfat just skips errors; fuse uses -EIO for this specific problem.

Behavior of "find" when encountering such filesystem corruption:

$ sudo mount img mnt
$ strace -v -e trace=getdents ls mnt
getdents(3, [{d_ino=1, d_off=1, d_reclen=24, d_name=".", d_type=DT_DIR}, {d_ino=1, d_off=2, d_reclen=24, d_name="..", d_type=DT_DIR}, {d_ino=67, d_off=16384, d_reclen=32, d_name="../../xx", d_type=DT_DIR}], 32768) = 80
getdents(3, [], 32768)                  = 0
+++ exited with 0 +++
$ find ../xx -ls
   406454      4 drwxr-xr-x   3 user     user         4096 Jul 16 20:48 ../xx
   406455      4 drwxr-xr-x   2 user     user         4096 Jul 16 20:48 ../xx/blah
   406457      0 -rw-r--r--   1 user     user            0 Jul 16 20:48 ../xx/blah/bar
$ find mnt -ls
        1     16 drwxr-xr-x   3 root     root        16384 Jan  1  1970 mnt
   406454      4 drwxr-xr-x   3 user     user         4096 Jul 16 20:48 mnt/../../xx
   406455      4 drwxr-xr-x   2 user     user         4096 Jul 16 20:48 mnt/../../xx/blah
   406457      0 -rw-r--r--   1 user     user            0 Jul 16 20:48 mnt/../../xx/blah/bar

 arch/alpha/kernel/osf_sys.c |  3 +++
 fs/readdir.c                | 33 +++++++++++++++++++++++++++++++++
 include/linux/fs.h          |  3 +++
 3 files changed, 39 insertions(+)

diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 6e921754c8fc..f16857d9782d 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -40,6 +40,7 @@
 #include <linux/vfs.h>
 #include <linux/rcupdate.h>
 #include <linux/slab.h>
+#include <linux/fs.h>
 
 #include <asm/fpu.h>
 #include <asm/io.h>
@@ -117,6 +118,8 @@ osf_filldir(struct dir_context *ctx, const char *name, int namlen,
 	unsigned int reclen = ALIGN(NAME_OFFSET + namlen + 1, sizeof(u32));
 	unsigned int d_ino;
 
+	if (bogus_dirent_name(&buf->error, name, namlen, __func__))
+		return -EUCLEAN;
 	buf->error = -EINVAL;	/* only used if we fail */
 	if (reclen > buf->count)
 		return -EINVAL;
diff --git a/fs/readdir.c b/fs/readdir.c
index d97f548e6323..a061a52b06d1 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -88,6 +88,29 @@ struct readdir_callback {
 	int result;
 };
 
+/*
+ * Most filesystems don't filter out bogus directory entry names, and userspace
+ * can get very confused by such names. Behave as if a low-level IO error had
+ * happened while reading directory entries.
+ */
+bool bogus_dirent_name(int *errp, const char *name, int namlen,
+		       const char *caller)
+{
+	if (namlen == 0) {
+		pr_err_once("%s: filesystem returned bogus empty name\n",
+			    caller);
+		*errp = -EUCLEAN;
+		return true;
+	}
+	if (memchr(name, '/', namlen)) {
+		pr_err_once("%s: filesystem returned bogus name '%*pEhp' (contains slash)\n",
+			    caller, namlen, name);
+		*errp = -EUCLEAN;
+		return true;
+	}
+	return false;
+}
+
 static int fillonedir(struct dir_context *ctx, const char *name, int namlen,
 		      loff_t offset, u64 ino, unsigned int d_type)
 {
@@ -98,6 +121,8 @@ static int fillonedir(struct dir_context *ctx, const char *name, int namlen,
 
 	if (buf->result)
 		return -EINVAL;
+	if (bogus_dirent_name(&buf->result, name, namlen, __func__))
+		return -EUCLEAN;
 	d_ino = ino;
 	if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) {
 		buf->result = -EOVERFLOW;
@@ -173,6 +198,8 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
 	int reclen = ALIGN(offsetof(struct linux_dirent, d_name) + namlen + 2,
 		sizeof(long));
 
+	if (bogus_dirent_name(&buf->error, name, namlen, __func__))
+		return -EUCLEAN;
 	buf->error = -EINVAL;	/* only used if we fail.. */
 	if (reclen > buf->count)
 		return -EINVAL;
@@ -259,6 +286,8 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
 	int reclen = ALIGN(offsetof(struct linux_dirent64, d_name) + namlen + 1,
 		sizeof(u64));
 
+	if (bogus_dirent_name(&buf->error, name, namlen, __func__))
+		return -EUCLEAN;
 	buf->error = -EINVAL;	/* only used if we fail.. */
 	if (reclen > buf->count)
 		return -EINVAL;
@@ -358,6 +387,8 @@ static int compat_fillonedir(struct dir_context *ctx, const char *name,
 
 	if (buf->result)
 		return -EINVAL;
+	if (bogus_dirent_name(&buf->result, name, namlen, __func__))
+		return -EUCLEAN;
 	d_ino = ino;
 	if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) {
 		buf->result = -EOVERFLOW;
@@ -427,6 +458,8 @@ static int compat_filldir(struct dir_context *ctx, const char *name, int namlen,
 	int reclen = ALIGN(offsetof(struct compat_linux_dirent, d_name) +
 		namlen + 2, sizeof(compat_long_t));
 
+	if (bogus_dirent_name(&buf->error, name, namlen, __func__))
+		return -EUCLEAN;
 	buf->error = -EINVAL;	/* only used if we fail.. */
 	if (reclen > buf->count)
 		return -EINVAL;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d78d146a98da..5d12a40bcc0c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1680,6 +1680,9 @@ struct dir_context {
 	loff_t pos;
 };
 
+bool bogus_dirent_name(int *errp, const char *name, int namlen,
+		       const char *caller);
+
 struct block_device_operations;
 
 /* These macros are for out of kernel modules to test that
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH] fs: don't let getdents return bogus names

[Al Viro]

On Mon, Jul 16, 2018 at 09:48:43PM +0200, Jann Horn wrote:
> When you e.g. run `find` on a directory for which getdents returns
> "filenames" that contain slashes, `find` passes those "filenames" back to
> the kernel, which then interprets them as paths. That could conceivably
> cause userspace to do something bad when accessing something like an
> untrusted USB stick, but I'm not aware of any specific example.
> 
> Instead of returning bogus filenames to userspace, return -EUCLEAN.

Because there's such a lot of userland code that expect and handles that
error value...

I'm not sure if this mitigation is actually better than "just return it
as-is", TBH.

Re: [PATCH] fs: don't let getdents return bogus names

[Jann Horn]

On Mon, Jul 16, 2018 at 9:57 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Jul 16, 2018 at 09:48:43PM +0200, Jann Horn wrote:
> > When you e.g. run `find` on a directory for which getdents returns
> > "filenames" that contain slashes, `find` passes those "filenames" back to
> > the kernel, which then interprets them as paths. That could conceivably
> > cause userspace to do something bad when accessing something like an
> > untrusted USB stick, but I'm not aware of any specific example.
> >
> > Instead of returning bogus filenames to userspace, return -EUCLEAN.
>
> Because there's such a lot of userland code that expect and handles that
> error value...

Empirically, for example, "find" seems to handle it (by printing an
error message to its caller).

> I'm not sure if this mitigation is actually better than "just return it
> as-is", TBH.

Turning it into something that behaves approximately like an I/O error
seems way better to me than letting userspace run around wildly and
potentially (depending on the filename and what userspace is trying to
do) mess with unrelated files belonging to entirely different mounts.
For accidental corruption, that's probably not an issue; but when
dealing with something like a malicious vfat filesystem on a USB
stick, I imagine it could be problematic.

Re: [PATCH] fs: don't let getdents return bogus names

[Dave Chinner]

On Mon, Jul 16, 2018 at 08:56:57PM +0100, Al Viro wrote:
> On Mon, Jul 16, 2018 at 09:48:43PM +0200, Jann Horn wrote:
> > When you e.g. run `find` on a directory for which getdents returns
> > "filenames" that contain slashes, `find` passes those "filenames" back to
> > the kernel, which then interprets them as paths. That could conceivably
> > cause userspace to do something bad when accessing something like an
> > untrusted USB stick, but I'm not aware of any specific example.
> > 
> > Instead of returning bogus filenames to userspace, return -EUCLEAN.
> 
> Because there's such a lot of userland code that expect and handles that
> error value...

We've been using EUCLEAN ("structure needs cleaning") for indicating
filesystem corruption errors for many years now. e.g.

fs/ext2/ext2.h:#define  EFSCORRUPTED                    EUCLEAN /* Filesystem is corrupted */
fs/ext4/ext4.h:#define EFSCORRUPTED     EUCLEAN         /* Filesystem is corrupted */
fs/hpfs/hpfs_fn.h:#define EFSERROR  EUCLEAN
fs/xfs/xfs_linux.h:#define EFSCORRUPTED EUCLEAN         /* Filesystem is corrupted */
include/linux/jbd2.h:#define EFSCORRUPTED       EUCLEAN         /* Filesystem is corrupted */

There's hundreds of places in the filesystem code that this
specific error is returned to userspace - there's more than 500
individual places this error can be returned from just XFS....

To me it seems like the right error to return if a dirent is
corrupted, because that's exactly what XFS will return if any of the
directory structure around the dirent name itself is corrupt...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] fs: don't let getdents return bogus names

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On Mon 2018-07-16 20:56:57, Al Viro wrote:
> On Mon, Jul 16, 2018 at 09:48:43PM +0200, Jann Horn wrote:
> > When you e.g. run `find` on a directory for which getdents returns
> > "filenames" that contain slashes, `find` passes those "filenames" back to
> > the kernel, which then interprets them as paths. That could conceivably
> > cause userspace to do something bad when accessing something like an
> > untrusted USB stick, but I'm not aware of any specific example.
> > 
> > Instead of returning bogus filenames to userspace, return -EUCLEAN.
> 
> Because there's such a lot of userland code that expect and handles that
> error value...
> 
> I'm not sure if this mitigation is actually better than "just return it
> as-is", TBH.

Well, userspace should handle errors.. it may not understand what this
particular error means, but that's still better than risking issues
with / in path....

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] fs: don't let getdents return bogus names

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]

Hi Jann,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc5 next-20180716]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jann-Horn/fs-don-t-let-getdents-return-bogus-names/20180717-040055
config: openrisc-or1ksim_defconfig (attached as .config)
compiler: or1k-linux-gcc (GCC) 6.0.0 20160327 (experimental)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=openrisc 

All errors (new ones prefixed by >>):

   fs/readdir.o: In function `filldir':
>> readdir.c:(.text+0x234): undefined reference to `bogus_dirent_name'
   readdir.c:(.text+0x234): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `bogus_dirent_name'
   fs/readdir.o: In function `filldir64':
   readdir.c:(.text+0x3fc): undefined reference to `bogus_dirent_name'
   readdir.c:(.text+0x3fc): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `bogus_dirent_name'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7654 bytes --]

Re: [PATCH] fs: don't let getdents return bogus names

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

Hi Jann,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc5 next-20180716]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jann-Horn/fs-don-t-let-getdents-return-bogus-names/20180717-040055
config: microblaze-mmu_defconfig (attached as .config)
compiler: microblaze-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=microblaze 

All errors (new ones prefixed by >>):

   fs/readdir.o: In function `filldir':
>> (.text+0x224): undefined reference to `bogus_dirent_name'
   fs/readdir.o: In function `filldir64':
   (.text+0x3f8): undefined reference to `bogus_dirent_name'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12611 bytes --]