[v2] vfprintf stack overflow [BZ #16617]

Message ID 5485CACB.5070604@redhat.com
State Committed
Headers

Commit Message

Florian Weimer Dec. 8, 2014, 3:59 p.m. UTC
  On 12/08/2014 04:41 PM, Joseph Myers wrote:
> On Mon, 8 Dec 2014, Florian Weimer wrote:
>
>> +		specs = malloc (nspecs_size);
>> +		if (specs == NULL)
>> +		  {
>> +		    __set_errno (ENOMEM);
>> +		    done = -1;
>> +		    goto all_done;
>> +		  }
>
> It looks to me like this will leak the previous copy of specs on
> allocation failure, if the previous value was also malloced (so you need
> "specs = old;" or similar here to get the return path to free it if
> appropriate).

Ugh, thanks, I've made that change.  Anything else?
  

Comments

Florian Weimer Dec. 12, 2014, 10:07 a.m. UTC | #1
On 12/08/2014 04:59 PM, Florian Weimer wrote:
> On 12/08/2014 04:41 PM, Joseph Myers wrote:
>> On Mon, 8 Dec 2014, Florian Weimer wrote:
>>
>>> +        specs = malloc (nspecs_size);
>>> +        if (specs == NULL)
>>> +          {
>>> +            __set_errno (ENOMEM);
>>> +            done = -1;
>>> +            goto all_done;
>>> +          }
>>
>> It looks to me like this will leak the previous copy of specs on
>> allocation failure, if the previous value was also malloced (so you need
>> "specs = old;" or similar here to get the return path to free it if
>> appropriate).
>
> Ugh, thanks, I've made that change.  Anything else?

FYI, I plan to commit this in Monday so that this doesn't fall through 
the cracks again.
  
Allan McRae Dec. 15, 2014, 10:25 a.m. UTC | #2
On 12/12/14 20:07, Florian Weimer wrote:
> On 12/08/2014 04:59 PM, Florian Weimer wrote:
>> On 12/08/2014 04:41 PM, Joseph Myers wrote:
>>> On Mon, 8 Dec 2014, Florian Weimer wrote:
>>>
>>>> +        specs = malloc (nspecs_size);
>>>> +        if (specs == NULL)
>>>> +          {
>>>> +            __set_errno (ENOMEM);
>>>> +            done = -1;
>>>> +            goto all_done;
>>>> +          }
>>>
>>> It looks to me like this will leak the previous copy of specs on
>>> allocation failure, if the previous value was also malloced (so you need
>>> "specs = old;" or similar here to get the return path to free it if
>>> appropriate).
>>
>> Ugh, thanks, I've made that change.  Anything else?
> 
> FYI, I plan to commit this in Monday so that this doesn't fall through
> the cracks again.

Are you going to backport this to release branches?
  
Florian Weimer Dec. 15, 2014, 10:37 a.m. UTC | #3
On 12/15/2014 11:25 AM, Allan McRae wrote:
> On 12/12/14 20:07, Florian Weimer wrote:
>> On 12/08/2014 04:59 PM, Florian Weimer wrote:
>>> On 12/08/2014 04:41 PM, Joseph Myers wrote:
>>>> On Mon, 8 Dec 2014, Florian Weimer wrote:
>>>>
>>>>> +        specs = malloc (nspecs_size);
>>>>> +        if (specs == NULL)
>>>>> +          {
>>>>> +            __set_errno (ENOMEM);
>>>>> +            done = -1;
>>>>> +            goto all_done;
>>>>> +          }
>>>>
>>>> It looks to me like this will leak the previous copy of specs on
>>>> allocation failure, if the previous value was also malloced (so you need
>>>> "specs = old;" or similar here to get the return path to free it if
>>>> appropriate).
>>>
>>> Ugh, thanks, I've made that change.  Anything else?
>>
>> FYI, I plan to commit this in Monday so that this doesn't fall through
>> the cracks again.
>
> Are you going to backport this to release branches?

What's the procedure there?

I'm sure it's documented somewhere in the wiki, but could you at 
pointers in the appropriate places, such as:

   https://sourceware.org/glibc/wiki/Committer%20checklist
   https://sourceware.org/glibc/wiki/Consensus
  
Allan McRae Dec. 15, 2014, 10:55 a.m. UTC | #4
On 15/12/14 20:37, Florian Weimer wrote:
> On 12/15/2014 11:25 AM, Allan McRae wrote:
>> On 12/12/14 20:07, Florian Weimer wrote:
>>> On 12/08/2014 04:59 PM, Florian Weimer wrote:
>>>> On 12/08/2014 04:41 PM, Joseph Myers wrote:
>>>>> On Mon, 8 Dec 2014, Florian Weimer wrote:
>>>>>
>>>>>> +        specs = malloc (nspecs_size);
>>>>>> +        if (specs == NULL)
>>>>>> +          {
>>>>>> +            __set_errno (ENOMEM);
>>>>>> +            done = -1;
>>>>>> +            goto all_done;
>>>>>> +          }
>>>>>
>>>>> It looks to me like this will leak the previous copy of specs on
>>>>> allocation failure, if the previous value was also malloced (so you
>>>>> need
>>>>> "specs = old;" or similar here to get the return path to free it if
>>>>> appropriate).
>>>>
>>>> Ugh, thanks, I've made that change.  Anything else?
>>>
>>> FYI, I plan to commit this in Monday so that this doesn't fall through
>>> the cracks again.
>>
>> Are you going to backport this to release branches?
> 
> What's the procedure there?
> 
> I'm sure it's documented somewhere in the wiki, but could you at
> pointers in the appropriate places, such as:
> 
>   https://sourceware.org/glibc/wiki/Committer%20checklist
>   https://sourceware.org/glibc/wiki/Consensus
> 

https://sourceware.org/glibc/wiki/Release/#General_policy

In short, "git cherry-pick -x <commitid>".  You will need to fix the
conflict in NEWS for this patch.

Allan
  
Florian Weimer Dec. 15, 2014, 11:09 a.m. UTC | #5
On 12/15/2014 11:55 AM, Allan McRae wrote:

> https://sourceware.org/glibc/wiki/Release/#General_policy
>
> In short, "git cherry-pick -x <commitid>".  You will need to fix the
> conflict in NEWS for this patch.

I'm currently not in a position to send a (politically acceptable) pull 
request, so I'd prefer if someone else took care of it.
  
Allan McRae Dec. 16, 2014, 4:39 a.m. UTC | #6
On 15/12/14 21:09, Florian Weimer wrote:
> On 12/15/2014 11:55 AM, Allan McRae wrote:
> 
>> https://sourceware.org/glibc/wiki/Release/#General_policy
>>
>> In short, "git cherry-pick -x <commitid>".  You will need to fix the
>> conflict in NEWS for this patch.
> 
> I'm currently not in a position to send a (politically acceptable) pull
> request, so I'd prefer if someone else took care of it.
> 

That is a bit outdated too...   All release branches are now community
maintained with any patch that is committed to master (while being
conservative!) able to be pulled.  I pulled this to the 2.20 release
branch as it was the one I am interested in.

I also corrected a testcase being dropped in stdio-common/Makefile,
which appeared to be from a bad merge.

Allan
  

Patch

From 527e88d9699cf2e8bad704517745502cf34a7e9a Mon Sep 17 00:00:00 2001
From: Jeff Law <law@redhat.com>
Date: Fri, 5 Dec 2014 15:49:34 +0100
Subject: [PATCH] CVE-2012-3406: Stack overflow in vfprintf [BZ #16617]

A larger number of format specifiers coudld cause a stack overflow,
potentially allowing to bypass _FORTIFY_SOURCE format string
protection.

2014-12-05  Jeff Law  <law@redhat.com>

	[BZ #16617]
	* stdio-common/vfprintf.c (vfprintf): Allocate large specs array
	on the heap.  (CVE-2012-3406)
	* stdio-common/bug23-2.c, stdio-common/bug23-3.c: New file.
	* stdio-common/bug23-4.c: New file.  Test case by Joseph Myers.
	* stdio-common/Makefile (tests): Add bug23-2, bug23-3, bug23-4.

diff --git a/NEWS b/NEWS
index 1b08d40..821892f 100644
--- a/NEWS
+++ b/NEWS
@@ -10,11 +10,11 @@  Version 2.21
 * The following bugs are resolved with this release:
 
   6652, 12926, 13862, 14132, 14138, 14171, 14498, 15215, 15884, 16469,
-  16619, 16740, 16857, 17192, 17266, 17344, 17363, 17370, 17371, 17411,
-  17460, 17475, 17485, 17501, 17506, 17508, 17522, 17555, 17570, 17571,
-  17572, 17573, 17574, 17581, 17582, 17583, 17584, 17585, 17589, 17594,
-  17601, 17608, 17616, 17625, 17633, 17647, 17653, 17664, 17665, 17668,
-  17682.
+  16617, 16619, 16740, 16857, 17192, 17266, 17344, 17363, 17370, 17371,
+  17411, 17460, 17475, 17485, 17501, 17506, 17508, 17522, 17555, 17570,
+  17571, 17572, 17573, 17574, 17581, 17582, 17583, 17584, 17585, 17589,
+  17594, 17601, 17608, 17616, 17625, 17633, 17647, 17653, 17664, 17665,
+  17668, 17682.
 
 * CVE-2104-7817 The wordexp function could ignore the WRDE_NOCMD flag
   under certain input conditions resulting in the execution of a shell for
@@ -22,6 +22,9 @@  Version 2.21
   implementation now checks WRDE_NOCMD immediately before executing the
   shell and returns the error WRDE_CMDSUB as expected.
 
+* CVE-2012-3406 printf-style functions could run into a stack overflow when
+  processing format strings with a large number of format specifiers.
+
 * The minimum GCC version that can be used to build this version of the GNU
   C Library is GCC 4.6.  Older GCC versions, and non-GNU compilers, can
   still be used to compile programs using the GNU C Library.
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 5f8e534..24e8496 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -57,7 +57,7 @@  tests := tstscanf test_rdwr test-popen tstgetln test-fseek \
 	 bug19 bug19a tst-popen2 scanf13 scanf14 scanf15 bug20 bug21 bug22 \
 	 scanf16 scanf17 tst-setvbuf1 tst-grouping bug23 bug24 \
 	 bug-vfprintf-nargs tst-long-dbl-fphex tst-fphex-wide tst-sprintf3 \
-	 bug25 tst-printf-round bug26
+	 bug25 tst-printf-round bug23-2 bug23-3 bug23-4
 
 test-srcs = tst-unbputc tst-printf
 
diff --git a/stdio-common/bug23-2.c b/stdio-common/bug23-2.c
new file mode 100644
index 0000000..9e0cfe6
--- /dev/null
+++ b/stdio-common/bug23-2.c
@@ -0,0 +1,70 @@ 
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+
+static const char expected[] = "\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55\
+\n\
+a\n\
+abbcd55%%%%%%%%%%%%%%%%%%%%%%%%%%\n";
+
+static int
+do_test (void)
+{
+  char *buf = malloc (strlen (expected) + 1);
+  snprintf (buf, strlen (expected) + 1,
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+	    "%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%\n",
+	    "a", "b", "c", "d", 5);
+  return strcmp (buf, expected) != 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/stdio-common/bug23-3.c b/stdio-common/bug23-3.c
new file mode 100644
index 0000000..57c8cef
--- /dev/null
+++ b/stdio-common/bug23-3.c
@@ -0,0 +1,50 @@ 
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+
+int
+do_test (void)
+{
+  size_t instances = 16384;
+#define X0 "\n%1$s\n" "%1$s" "%2$s" "%2$s" "%3$s" "%4$s" "%5$d" "%5$d"
+  const char *item = "\na\nabbcd55";
+#define X3 X0 X0 X0 X0 X0 X0 X0 X0
+#define X6 X3 X3 X3 X3 X3 X3 X3 X3
+#define X9 X6 X6 X6 X6 X6 X6 X6 X6
+#define X12 X9 X9 X9 X9 X9 X9 X9 X9
+#define X14 X12 X12 X12 X12
+#define TRAILER "%%%%%%%%%%%%%%%%%%%%%%%%%%"
+#define TRAILER2 TRAILER TRAILER
+  size_t length = instances * strlen (item) + strlen (TRAILER) + 1;
+
+  char *buf = malloc (length + 1);
+  snprintf (buf, length + 1,
+	    X14 TRAILER2 "\n",
+	    "a", "b", "c", "d", 5);
+
+  const char *p = buf;
+  size_t i;
+  for (i = 0; i < instances; ++i)
+    {
+      const char *expected;
+      for (expected = item; *expected; ++expected)
+	{
+	  if (*p != *expected)
+	    {
+	      printf ("mismatch at offset %zu (%zu): expected %d, got %d\n",
+		      (size_t) (p - buf), i, *expected & 0xFF, *p & 0xFF);
+	      return 1;
+	    }
+	  ++p;
+	}
+    }
+  if (strcmp (p, TRAILER "\n") != 0)
+    {
+      printf ("mismatch at trailer: [%s]\n", p);
+      return 1;
+    }
+  free (buf);
+  return 0;
+}
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/stdio-common/bug23-4.c b/stdio-common/bug23-4.c
new file mode 100644
index 0000000..a478564
--- /dev/null
+++ b/stdio-common/bug23-4.c
@@ -0,0 +1,31 @@ 
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/resource.h>
+
+#define LIMIT 1000000
+
+int
+main (void)
+{
+  struct rlimit lim;
+  getrlimit (RLIMIT_STACK, &lim);
+  lim.rlim_cur = 1048576;
+  setrlimit (RLIMIT_STACK, &lim);
+  char *fmtstr = malloc (4 * LIMIT + 1);
+  if (fmtstr == NULL)
+    abort ();
+  char *output = malloc (LIMIT + 1);
+  if (output == NULL)
+    abort ();
+  for (size_t i = 0; i < LIMIT; i++)
+    memcpy (fmtstr + 4 * i, "%1$d", 4);
+  fmtstr[4 * LIMIT] = '\0';
+  int ret = snprintf (output, LIMIT + 1, fmtstr, 0);
+  if (ret != LIMIT)
+    abort ();
+  for (size_t i = 0; i < LIMIT; i++)
+    if (output[i] != '0')
+      abort ();
+  return 0;
+}
diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index c4ff833..429a3d1 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -263,6 +263,12 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap)
   /* For the argument descriptions, which may be allocated on the heap.  */
   void *args_malloced = NULL;
 
+  /* For positional argument handling.  */
+  struct printf_spec *specs;
+
+  /* Track if we malloced the SPECS array and thus must free it.  */
+  bool specs_malloced = false;
+
   /* This table maps a character into a number representing a
      class.  In each step there is a destination label for each
      class.  */
@@ -1679,8 +1685,8 @@  do_positional:
     size_t nspecs = 0;
     /* A more or less arbitrary start value.  */
     size_t nspecs_size = 32 * sizeof (struct printf_spec);
-    struct printf_spec *specs = alloca (nspecs_size);
 
+    specs = alloca (nspecs_size);
     /* The number of arguments the format string requests.  This will
        determine the size of the array needed to store the argument
        attributes.  */
@@ -1721,11 +1727,39 @@  do_positional:
 	if (nspecs * sizeof (*specs) >= nspecs_size)
 	  {
 	    /* Extend the array of format specifiers.  */
+	    if (nspecs_size * 2 < nspecs_size)
+	      {
+		__set_errno (ENOMEM);
+		done = -1;
+		goto all_done;
+	      }
 	    struct printf_spec *old = specs;
-	    specs = extend_alloca (specs, nspecs_size, 2 * nspecs_size);
+	    if (__libc_use_alloca (2 * nspecs_size))
+	      specs = extend_alloca (specs, nspecs_size, 2 * nspecs_size);
+	    else
+	      {
+		nspecs_size *= 2;
+		specs = malloc (nspecs_size);
+		if (specs == NULL)
+		  {
+		    __set_errno (ENOMEM);
+		    specs = old;
+		    done = -1;
+		    goto all_done;
+		  }
+	      }
 
 	    /* Copy the old array's elements to the new space.  */
 	    memmove (specs, old, nspecs * sizeof (*specs));
+
+	    /* If we had previously malloc'd space for SPECS, then
+	       release it after the copy is complete.  */
+	    if (specs_malloced)
+	      free (old);
+
+	    /* Now set SPECS_MALLOCED if needed.  */
+	    if (!__libc_use_alloca (nspecs_size))
+	      specs_malloced = true;
 	  }
 
 	/* Parse the format specifier.  */
@@ -2046,6 +2080,8 @@  do_positional:
   }
 
 all_done:
+  if (specs_malloced)
+    free (specs);
   if (__glibc_unlikely (args_malloced != NULL))
     free (args_malloced);
   if (__glibc_unlikely (workstart != NULL))
-- 
1.9.3