This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] vfprintf: validate nargs and positional offsets


On Thu, Feb 02, 2012 at 03:03:41PM -0800, Roland McGrath wrote:
> > On Thu, Feb 02, 2012 at 02:15:12PM -0800, Roland McGrath wrote:
> > > Posting here is not limited to subscribers.  
> > 
> > Based on my earlier attempts to send email here, non-subbed senders are
> > silently dropped.
> 
> I'm moderately sure you just misinterpreted the moderation delay.
> I recall seeing your first posting twice from two addresses, though
> the archive only has one copy (maybe some de-dup logic??).

Ah-ha, it wasn't clear there was a moderator for the list.

> > Ah, yes. I see this now in the existing ChangeLog, but the "#" should
> > probably be added to the template in
> > http://sourceware.org/glibc/wiki/Contribution%20checklist
> 
> It's a wiki, dude.  Fix it.

Already done. :)

> > +    /* Check for potential integer overflow. */
> [...]
> > +	/* Sanity-check the data_arg location. */
> 
> Fourth time might be the charm??

Again! :)


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

	[BZ #13656]
	* stdio-common/vfprintf.c (vfprintf): Check for nargs overflow and
	validate argument-based array offsets.
	* stdio-common/bug-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..593f5d4 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 \
+	 bug-vfprintf-nargs
 
 test-srcs = tst-unbputc tst-printf
 
diff --git a/stdio-common/bug-vfprintf-nargs.c b/stdio-common/bug-vfprintf-nargs.c
new file mode 100644
index 0000000..ad82713
--- /dev/null
+++ b/stdio-common/bug-vfprintf-nargs.c
@@ -0,0 +1,81 @@
+/* Test for vfprintf nargs allocation overflow (BZ #13656).
+   Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Kees Cook <keescook@chromium.org>, 2012.
+
+   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, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <unistd.h>
+#include <inttypes.h>
+#include <string.h>
+#include <signal.h>
+
+static int
+format_failed (const char *fmt, const char *expected)
+{
+  char output[80];
+
+  printf ("%s : ", fmt);
+
+  memset (output, 0, sizeof output);
+  /* Having sprintf itself detect a failure is good.  */
+  if (sprintf (output, fmt, 1, 2, 3, "test") > 0
+      && strcmp (output, expected) != 0)
+    {
+      printf ("FAIL (output '%s' != expected '%s')\n", output, expected);
+      return 1;
+    }
+  puts ("ok");
+  return 0;
+}
+
+static int
+do_test (void)
+{
+  int rc = 0;
+  char buf[64];
+
+  /* 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;
+
+  /* Check behavior of 32-bit positional overflow.  */
+  sprintf (buf, "%%1$d %%%" PRIdPTR "$d", UINT32_MAX / sizeof (int));
+  if (format_failed (buf, "1 %$d") != 0)
+    rc = 1;
+
+  return rc;
+}
+
+/* Positional arguments are constructed via read_int, so nargs can only
+   overflow on 32-bit systems.  On 64-bit systems, it will attempt to
+   allocate a giant amount of stack memory and crash, which is the
+   expected situation.  */
+#if __WORDSIZE == 32
+# define EXPECTED_STATUS 0
+#else
+# define EXPECTED_SIGNAL SIGSEGV
+#endif
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index 952886b..0220e48 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]