From libc-alpha-return-25252-listarch-libc-alpha=sources dot redhat dot com at sourceware dot org Thu Feb 16 16:21:17 2012 Return-Path: Delivered-To: listarch-libc-alpha at sources dot redhat dot com Received: (qmail 5187 invoked by alias); 16 Feb 2012 16:21:14 -0000 Delivered-To: moderator for libc-alpha at sourceware dot org Received: (qmail 2174 invoked by uid 22791); 16 Feb 2012 16:17:18 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,TW_TV,TW_VB,TW_VF,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Date: Thu, 16 Feb 2012 08:16:13 -0800 From: Kees Cook To: "Ryan S dot Arnold" Cc: libc-alpha at sourceware dot org, Paul Eggert , Roland McGrath , Andreas Schwab Subject: Re: [PATCH] vfprintf: validate nargs and maybe allocate from heap Message-ID: <20120216161613.GZ20420@outflux.net> References: <20120206062537.GM4979@outflux.net> <20120207000509 dot GP4989 at outflux dot net> <20120210192457 dot GF20420 at outflux dot net> <20120214223048 dot GM20420 at outflux dot net> <20120214224543 dot GN20420 at outflux dot net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120214224543 dot GN20420 at outflux dot net> X-MIMEDefang-Filter: outflux$Revision: 1.316 $ X-HELO: www.outflux.net Mailing-List: contact libc-alpha-help at sourceware dot org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner at sourceware dot org Delivered-To: mailing list libc-alpha at sourceware dot org 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. diff --git a/stdio-common/Makefile b/stdio-common/Makefile index a847b28..080badc 100644 --- a/stdio-common/Makefile +++ b/stdio-common/Makefile @@ -59,7 +59,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..13c66c0 --- /dev/null +++ b/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" diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c index 863cd5d..022e72b 100644 --- a/stdio-common/vfprintf.c +++ b/stdio-common/vfprintf.c @@ -235,6 +235,9 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap) 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. */ @@ -1647,9 +1650,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 @@ -1698,13 +1702,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 @@ -1973,8 +1997,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); -- 1.7.5.4 -- Kees Cook @outflux.net