From 0e190d479d2e7dd365577ff0d47169f851d34c24 Mon Sep 17 00:00:00 2001 From: Jeff Law Date: Mon, 20 Feb 2012 21:21:30 -0700 Subject: [PATCH] - Avoid "nargs" integer overflow which could be used to bypass FORTIFY_SOURCE (#794797) --- glibc-rh794797.patch | 240 +++++++++++++++++++++++++++++++++++++++++++ glibc.spec | 8 +- 2 files changed, 247 insertions(+), 1 deletion(-) create mode 100644 glibc-rh794797.patch diff --git a/glibc-rh794797.patch b/glibc-rh794797.patch new file mode 100644 index 0000000..271538d --- /dev/null +++ b/glibc-rh794797.patch @@ -0,0 +1,240 @@ +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 + diff --git a/glibc.spec b/glibc.spec index 1dad96f..00dea67 100644 --- a/glibc.spec +++ b/glibc.spec @@ -28,7 +28,7 @@ Summary: The GNU libc libraries Name: glibc Version: %{glibcversion} -Release: 22%{?dist} +Release: 23%{?dist} # GPLv2+ is used in a bunch of programs, LGPLv2+ is used for libraries. # Things that are linked directly into dynamically linked programs # and shared libraries (e.g. crt files, lib*_nonshared.a) have an additional @@ -98,6 +98,8 @@ Patch31 : %{name}-rh697149.patch Patch32 : %{name}-rh739743.patch # Discussion started upstream, patch needs to be submitted Patch33 : %{name}-rh789238.patch +# Patch posted upstream, discussion ongoing, Paul E. seems to think it's OK +Patch34 : %{name}-rh794797.patch Buildroot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) @@ -350,6 +352,7 @@ rm -rf %{glibcportsdir} %patch31 -p1 %patch32 -p1 %patch33 -p1 +%patch34 -p1 # A lot of programs still misuse memcpy when they have to use # memmove. The memcpy implementation below is not tolerant at @@ -1202,6 +1205,9 @@ rm -f *.filelist* %endif %changelog +* Mon Feb 20 2012 Jeff Law - 2.15-23 + - Avoid "nargs" integer overflow which could be used to bypass FORTIFY_SOURCE (#794797) + * Mon Feb 20 2012 Jeff Law - 2.15-22 - Fix main arena locking in malloc/calloc retry path (#789238)