Patchwork Fix path length overflow in realpath (BZ#22786)

login
register
mail settings
Submitter Paul Pluzhnikov
Date April 17, 2018, 11:50 p.m.
Message ID <CALoOobNywsKOoamnNH7=eF28=yoQMsFPy+5O6cgD4oqpVDhARA@mail.gmail.com>
Download mbox | patch
Permalink /patch/26771/
State New
Headers show

Comments

Paul Pluzhnikov - April 17, 2018, 11:50 p.m.
On Tue, Apr 17, 2018 at 2:01 PM Joseph Myers <joseph@codesourcery.com>
wrote:

> On Mon, 9 Apr 2018, Paul Pluzhnikov wrote:

> > +# suppress warnings about allocation size.
> > +CFLAGS-test-bz22786.c += $(+gcc-nowarn)

> Warnings should be disabled as locally as possible in the sources

Revised patch attached. Thanks,

2018-04-17  Paul Pluzhnikov  <ppluzhnikov@google.com>

          [BZ #22786]
          * stdlib/canonicalize.c (__realpath): Fix overflow in path length
          computation.
          * stdlib/Makefile (test-bz22786): New test.
          * stdlib/test-bz22786.c: New test.
Paul Pluzhnikov - April 30, 2018, 9:41 p.m.
Ping?

On Tue, Apr 17, 2018 at 4:50 PM Paul Pluzhnikov <ppluzhnikov@google.com>
wrote:

> Revised patch attached. Thanks,

> 2018-04-17  Paul Pluzhnikov  <ppluzhnikov@google.com>
Paul Pluzhnikov - May 8, 2018, 2:33 p.m.
On Mon, Apr 30, 2018 at 2:41 PM Paul Pluzhnikov <ppluzhnikov@google.com>
wrote:

> Ping?

Ping x2?

> On Tue, Apr 17, 2018 at 4:50 PM Paul Pluzhnikov <ppluzhnikov@google.com>
> wrote:

> > Revised patch attached. Thanks,

> > 2018-04-17  Paul Pluzhnikov  <ppluzhnikov@google.com>
Andreas Schwab - May 8, 2018, 2:59 p.m.
On Apr 17 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
> new file mode 100644
> index 0000000000..1b6331ac5c
> --- /dev/null
> +++ b/stdlib/test-bz22786.c
> @@ -0,0 +1,90 @@
> +/* Bug 22786: test for stack overflow in realpath.

This is actually a buffer overflow.  Ok with that change.

Andreas.
Paul Pluzhnikov - May 8, 2018, 3:10 p.m.
On Tue, May 8, 2018 at 7:59 AM Andreas Schwab <schwab@suse.de> wrote:

> On Apr 17 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> > diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
> > new file mode 100644
> > index 0000000000..1b6331ac5c
> > --- /dev/null
> > +++ b/stdlib/test-bz22786.c
> > @@ -0,0 +1,90 @@
> > +/* Bug 22786: test for stack overflow in realpath.

> This is actually a buffer overflow.  Ok with that change.

I am not sure what you mean by that.

The (stack) allocated buffer is large enough, so technically there is no
buffer overflow here (at least not in the sense that "allocated buffer was
too small"). But the stack is not large enough to hold the buffer of that
size.
Andreas Schwab - May 8, 2018, 3:29 p.m.
On Mai 08 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> On Tue, May 8, 2018 at 7:59 AM Andreas Schwab <schwab@suse.de> wrote:
>
>> On Apr 17 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
>> > diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
>> > new file mode 100644
>> > index 0000000000..1b6331ac5c
>> > --- /dev/null
>> > +++ b/stdlib/test-bz22786.c
>> > @@ -0,0 +1,90 @@
>> > +/* Bug 22786: test for stack overflow in realpath.
>
>> This is actually a buffer overflow.  Ok with that change.
>
> I am not sure what you mean by that.
>
> The (stack) allocated buffer is large enough

Is it?  The condition is about the limit of the buffer being written,
and about overflow missing the limit.

Andreas.
Paul Pluzhnikov - May 8, 2018, 3:45 p.m.
On Tue, May 8, 2018 at 8:29 AM Andreas Schwab <schwab@suse.de> wrote:

> > The (stack) allocated buffer is large enough

> Is it?  The condition is about the limit of the buffer being written,
> and about overflow missing the limit.

Oops, I mixed up this patch with the other overflow (BZ 20419).
You are absolutely right. Will retest and commit shortly.

Thanks,

Patch

diff --git a/stdlib/Makefile b/stdlib/Makefile
index af1643c0c4..1ddb1f9d18 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -84,7 +84,7 @@  tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-cxa_atexit tst-on_exit test-atexit-race 		    \
 		   test-at_quick_exit-race test-cxa_atexit-race             \
 		   test-on_exit-race test-dlclose-exit-race 		    \
-		   tst-makecontext-align
+		   tst-makecontext-align test-bz22786
 
 tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
 		   tst-tls-atexit tst-tls-atexit-nodelete
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 4135f3f33c..390fb437a8 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -181,7 +181,7 @@  __realpath (const char *name, char *resolved)
 		extra_buf = __alloca (path_max);
 
 	      len = strlen (end);
-	      if ((long int) (n + len) >= path_max)
+	      if (path_max - n <= len)
 		{
 		  __set_errno (ENAMETOOLONG);
 		  goto error;
diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
new file mode 100644
index 0000000000..1b6331ac5c
--- /dev/null
+++ b/stdlib/test-bz22786.c
@@ -0,0 +1,90 @@ 
+/* Bug 22786: test for stack overflow in realpath.
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This file must be run from within a directory called "stdlib".  */
+
+#include <errno.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <support/test-driver.h>
+#include <libc-diag.h>
+
+static int
+do_test (void)
+{
+  const char dir[] = "bz22786";
+  const char lnk[] = "bz22786/symlink";
+
+  rmdir (dir);
+  if (mkdir (dir, 0755) != 0 && errno != EEXIST)
+    {
+      printf ("mkdir %s: %m\n", dir);
+      return EXIT_FAILURE;
+    }
+  if (symlink (".", lnk) != 0 && errno != EEXIST)
+    {
+      printf ("symlink (%s, %s): %m\n", dir, lnk);
+      return EXIT_FAILURE;
+    }
+
+  const size_t path_len = (size_t) INT_MAX + 1;
+
+  DIAG_PUSH_NEEDS_COMMENT;
+#if __GNUC_PREREQ (7, 0)
+  /* GCC 7 warns about too-large allocations; here we need such
+     allocation to succeed for the test to work.  */
+  DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
+#endif
+  char *path = malloc (path_len);
+  DIAG_POP_NEEDS_COMMENT;
+
+  if (path == NULL)
+    {
+      printf ("malloc (%zu): %m\n", path_len);
+      return EXIT_UNSUPPORTED;
+    }
+
+  /* Construct very long path = "bz22786/symlink/aaaa....."  */
+  char *p = mempcpy (path, lnk, sizeof (lnk) - 1);
+  *(p++) = '/';
+  memset (p, 'a', path_len - (path - p) - 2);
+  p[path_len - (path - p) - 1] = '\0';
+
+  /* This call crashes before the fix for bz22786 on 32-bit platforms.  */
+  p = realpath (path, NULL);
+
+  if (p != NULL || errno != ENAMETOOLONG)
+    {
+      printf ("realpath: %s (%m)", p);
+      return EXIT_FAILURE;
+    }
+
+  /* Cleanup.  */
+  unlink (lnk);
+  rmdir (dir);
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test
+#include <support/test-driver.c>