Date: Thu, 16 Feb 2012 08:16:13 -0800 From: Kees Cook To: "Ryan S. Arnold" Cc: libc-alpha@sourceware.org, Paul Eggert , Roland McGrath , Andreas Schwab Subject: Re: [PATCH] vfprintf: validate nargs and maybe allocate from heap The nargs value can overflow when doing allocations, allowing arbitrary memory writes via format strings, bypassing _FORTIFY_SOURCE: http://www.phrack.org/issues.html?issue=67&id=9 This checks for nargs overflow and possibly allocates from heap instead of stack, and adds a regression test for the situation. I have FSF assignment via Google. (Sent from @outflux since that's how I'm subscribed here, but CL shows @chromium.org as part of my Google work.) This version disables the useless test on non-32-bit platforms. 2012-02-16 Kees Cook [BZ #13656] * stdio-common/vfprintf.c (vfprintf): Check for nargs overflow and possibly allocate from heap instead of stack. * stdio-common/bug-vfprintf-nargs.c: New file. * stdio-common/Makefile (tests): Add nargs overflow test. Index: glibc-2.15/stdio-common/Makefile =================================================================== --- glibc-2.15.orig/stdio-common/Makefile +++ glibc-2.15/stdio-common/Makefile @@ -60,7 +60,8 @@ tests := tstscanf test_rdwr test-popen t 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 Index: glibc-2.15/stdio-common/bug-vfprintf-nargs.c =================================================================== --- /dev/null +++ glibc-2.15/stdio-common/bug-vfprintf-nargs.c @@ -0,0 +1,78 @@ +/* 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 , 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 +#include +#include +#include +#include +#include +#include + +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; + + /* 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 memory and possibly crash, which is the + expected situation. Since the 64-bit behavior is arch-specific, only + test this on 32-bit systems. */ + if (sizeof (long int) == 4) + { + sprintf (buf, "%%1$d %%%" PRIdPTR "$d", UINT32_MAX / sizeof (int)); + if (format_failed (buf, "1 %$d") != 0) + rc = 1; + } + + return rc; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" Index: glibc-2.15/stdio-common/vfprintf.c =================================================================== --- glibc-2.15.orig/stdio-common/vfprintf.c +++ glibc-2.15/stdio-common/vfprintf.c @@ -236,6 +236,9 @@ vfprintf (FILE *s, const CHAR_T *format, 0 if unknown. */ int readonly_format = 0; + /* For the argument descriptions, which may be allocated on the heap. */ + void *args_malloced = NULL; + /* This table maps a character into a number representing a class. In each step there is a destination label for each class. */ @@ -1648,9 +1651,10 @@ do_positional: determine the size of the array needed to store the argument attributes. */ size_t nargs = 0; - int *args_type; - union printf_arg *args_value = NULL; + size_t bytes_per_arg; + union printf_arg *args_value; int *args_size; + int *args_type; /* Positional parameters refer to arguments directly. This could also determine the maximum number of arguments. Track the @@ -1699,13 +1703,33 @@ do_positional: /* Determine the number of arguments the format string consumes. */ nargs = MAX (nargs, max_ref_arg); + bytes_per_arg = sizeof (*args_value) + sizeof (*args_size) + + sizeof (*args_type); + + /* Check for potential integer overflow. */ + if (nargs > SIZE_MAX / bytes_per_arg) + { + done = -1; + goto all_done; + } /* Allocate memory for the argument descriptions. */ - args_type = alloca (nargs * sizeof (int)); + if (__libc_use_alloca (nargs * bytes_per_arg)) + args_value = alloca (nargs * bytes_per_arg); + else + { + args_value = args_malloced = malloc (nargs * bytes_per_arg); + if (args_value == NULL) + { + done = -1; + goto all_done; + } + } + + args_size = &args_value[nargs].pa_int; + args_type = &args_size[nargs]; memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0', - nargs * sizeof (int)); - args_value = alloca (nargs * sizeof (union printf_arg)); - args_size = alloca (nargs * sizeof (int)); + nargs * sizeof (*args_type)); /* XXX Could do sanity check here: If any element in ARGS_TYPE is still zero after this loop, format is invalid. For now we @@ -1974,8 +1998,8 @@ do_positional: } all_done: - if (__builtin_expect (workstart != NULL, 0)) - free (workstart); + free (args_malloced); + free (workstart); /* Unlock the stream. */ _IO_funlockfile (s); _IO_cleanup_region_end (0);