Re: [PATCH] vfprint: validate nargs and argument-based offsets

[Kees Cook <kees at outflux dot net>]

On Wed, Feb 01, 2012 at 01:26:40PM -0800, Roland McGrath wrote:
> > The nargs value can overflow when doing allocations, and argument-based
> > offsets are not bounds-checked, allowing arbitrary memory writes via
> > format strings, bypassing _FORTIFY_SOURCE protections:
> > http://www.phrack.org/issues.html?issue=67&id=9
> > 
> > This checks for nargs overflow and validates argument-based array offsets.
> 
> Seems like a good candidate for adding a test case that demonstrates the
> problems.

Excellent point. I've updated the patch now...

> > +    /* Check for potential integer overflow. */
> 
> Two spaces after a period (here and below).

Fixed.

-Kees


2012-02-02  Kees Cook  <keescook@chromium.org>

	* stdio-common/vfprintf.c (vfprintf): Checks for nargs overflow and
	validates argument-based array offsets.
	* stdio-common/tst-vfprintf-nargs.c: New file.
	* stdio-common/Makefile (tests): Add nargs overflow test.


diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 006f546..7d34b7a 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -60,7 +60,8 @@ tests := tstscanf test_rdwr test-popen tstgetln test-fseek \
 	 tst-popen tst-unlockedio tst-fmemopen2 tst-put-error tst-fgets \
 	 tst-fwrite bug16 bug17 tst-swscanf tst-sprintf2 bug18 bug18a \
 	 bug19 bug19a tst-popen2 scanf13 scanf14 scanf15 bug20 bug21 bug22 \
-	 scanf16 scanf17 tst-setvbuf1 tst-grouping bug23 bug24
+	 scanf16 scanf17 tst-setvbuf1 tst-grouping bug23 bug24 \
+	 tst-vfprintf-nargs
 
 test-srcs = tst-unbputc tst-printf
 
diff --git a/stdio-common/tst-vfprintf-nargs.c b/stdio-common/tst-vfprintf-nargs.c
new file mode 100644
index 0000000..7c48e20
--- /dev/null
+++ b/stdio-common/tst-vfprintf-nargs.c
@@ -0,0 +1,91 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <unistd.h>
+#include <inttypes.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+int
+format_failed(const char *fmt, const char *expected, int wanted)
+{
+  char output[80];
+  pid_t pid;
+  int status;
+
+  printf("%s : ", fmt);
+  fflush(NULL);
+
+  /* Since the stack could be extremely wrecked by this test, use
+     an external supervisor process to catch the signals, since a
+     signal handler may not be able to recover.
+   */
+  pid = fork();
+  if (pid < 0)
+    {
+      perror("fork");
+      return 1;
+    }
+  if (pid)
+    {
+      waitpid(pid, &status, 0);
+      if (WIFEXITED(status))
+        {
+          puts(WEXITSTATUS(status) == wanted ? "ok" : "FAILED");
+          return WEXITSTATUS(status);
+        }
+      else if (WIFSIGNALED(status))
+        {
+          if (WTERMSIG(status) == -wanted)
+            {
+              printf("ok (ignored expected signal %d)\n", -wanted);
+              return 0;
+            }
+          fprintf(stderr, "signal %d\n", WTERMSIG(status));
+          return 1;
+        }
+      else
+        {
+          fprintf(stderr, "Unexpected failure\n");
+          return 2;
+        }
+    }
+
+  memset(output, 0, sizeof(output));
+  /* Having sprintf itself detect a failure is good.  */
+  if (sprintf(output, fmt, 1, 2, 3, "test") < 0)
+      exit(0);
+  if (strcmp(output, expected))
+    {
+      fprintf(stderr, "(output '%s' != expected '%s') : ", output, expected);
+      exit(1);
+    }
+  exit(0);
+}
+
+int
+main(int argc, char *argv[])
+{
+  int rc = 0, wanted;
+  char buf[64];
+
+  /* Positional arguments are constructed via read_int(), so nargs
+     can only overflow on 32bit systems. On 64bit systems, it will
+     attempt to allocate a giant amount of stack memory and crash,
+     which is the expected situation.  */
+  if (sizeof(long) == 4)
+    wanted = 0;
+  else
+    wanted = -11;
+  sprintf(buf, "%%1$d %%%" PRIdPTR "$d", UINT32_MAX / sizeof(int));
+  if (format_failed(buf, "1 %$d", wanted)) rc = 1;
+
+  /* Regular positionals work.  */
+  if (format_failed("%1$d", "1", 0)) rc = 1;
+
+  /* Regular width positionals work.  */
+  if (format_failed("%1$*2$d", " 1", 0)) rc = 1;
+
+  return rc;
+}
diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index 952886b..3c1172c 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -1700,6 +1700,13 @@ do_positional:
     /* Determine the number of arguments the format string consumes.  */
     nargs = MAX (nargs, max_ref_arg);
 
+    /* Check for potential integer overflow. */
+    if (nargs > SIZE_MAX / (2 * sizeof (int) + sizeof (union printf_arg)))
+      {
+         done = -1;
+         goto all_done;
+      }
+
     /* Allocate memory for the argument descriptions.  */
     args_type = alloca (nargs * sizeof (int));
     memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
@@ -1715,13 +1722,17 @@ do_positional:
     for (cnt = 0; cnt < nspecs; ++cnt)
       {
 	/* If the width is determined by an argument this is an int.  */
-	if (specs[cnt].width_arg != -1)
+	if (specs[cnt].width_arg > -1 && specs[cnt].width_arg < nargs)
 	  args_type[specs[cnt].width_arg] = PA_INT;
 
 	/* If the precision is determined by an argument this is an int.  */
-	if (specs[cnt].prec_arg != -1)
+	if (specs[cnt].prec_arg > -1 && specs[cnt].prec_arg < nargs)
 	  args_type[specs[cnt].prec_arg] = PA_INT;
 
+	/* Sanity-check the data_arg location. */
+	if (specs[cnt].ndata_args && specs[cnt].data_arg >= nargs)
+	  continue;
+
 	switch (specs[cnt].ndata_args)
 	  {
 	  case 0:		/* No arguments.  */
-- 
1.7.5.4


-- 
Kees Cook                                            @outflux.net