This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] vfprintf: validate nargs and positional offsets
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.
> Send from whichever address you want to receive replies on.
>
> > [BZ 13656]
>
> The format is "[BZ #nnn]".
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
> > * stdio-common/bug13656.c: New file.
>
> I'm not sure we have any precedent for naming files for bugzilla numbers.
> It might be a reasonable thing. But the numbered ones we have are just
> increasing numbers (so bug25 is next). Personally I have something at
> least vaguely descriptive in the name (e.g. bug-vfprintf-fortify-nargs),
> though I'll bow to consensus on that.
If a specific style is required, I can resend. I think it makes sense
to name it after the BZ number if it's a bug regression check. Seems
like it would make merges much less confusing. Feature tests, yeah,
"tst-description" seems good.
> > + 02111-1307 USA. */
>
> Blank line here.
Fixed.
> > + if (sprintf (output, fmt, 1, 2, 3, "test") > 0 &&
> > + strcmp (output, expected) != 0)
>
> Operator goes on the second line when a clause is line-wrapped like this.
Fixed.
> > + /* Check behavior of 32bit positional overflow. */
>
> Say "32-bit".
Gah, missed one. Thanks.
> > +/* Positional arguments are constructed via read_int(), so nargs
>
> I personally hate the convention of appending () to a function name when
> referring to it. Comments are written in English, with normal punctuation
> standards. But people often do it, so that's not a blocker, just a pet
> peeve.
Given the lack of any kind of markup to distinguish English from code, I
like having the "()" in plain text.
> > +# define EXPECTED_SIGNAL 11
>
> Use SIGSEGV, not the integer literal.
Ah, yes. Good call. Fixed.
> Third time's the charm.
>
> I'm being extremely pedantic just because you are a new contributor and I
> want to teach all the conventions for future reference. We are often
> looser about some of this stuff, especially in test cases.
Sure, understood. New version...
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/bug13656.c: New file.
* stdio-common/Makefile (tests): Add nargs overflow test.
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 006f546..5ece3f6 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -60,7 +60,7 @@ 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 bug13656
test-srcs = tst-unbputc tst-printf
diff --git a/stdio-common/bug13656.c b/stdio-common/bug13656.c
new file mode 100644
index 0000000..5c2ffd7
--- /dev/null
+++ b/stdio-common/bug13656.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..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