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: <libc-alpha-return-25252-listarch-libc-alpha=sources dot redhat dot com at sourceware dot org>

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 <kees at outflux dot net>

To: "Ryan S dot  Arnold" <ryan dot arnold at gmail dot com>

Cc: libc-alpha at sourceware dot org, Paul Eggert <eggert at cs dot ucla dot edu>,

        Roland McGrath <roland at hack dot frob dot com>,

        Andreas Schwab <schwab at linux-m68k dot org>

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>

 <CAAKybw8AgkGsKAx=kvX4Tsi74f+HtuVnatTCB0VfsHi7vVFi1Q at mail dot gmail dot com>

 <20120214223048 dot GM20420 at outflux dot net>

 <CAAKybw_HS+cav+YcDw3ns7UXu6_xA7EHPrkiB87P+OGwEB0PVQ at mail dot gmail dot com>

 <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: <libc-alpha.sourceware.org>

List-Subscribe: <mailto:libc-alpha-subscribe at sourceware dot org>

List-Archive: <http://sourceware.org/ml/libc-alpha/>

List-Post: <mailto:libc-alpha at sourceware dot org>

List-Help: <mailto:libc-alpha-help at sourceware dot org>, <http://sourceware dot org/ml/#faqs>

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  <keescook@chromium.org>

 	[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 -rup a/stdio-common/Makefile b/stdio-common/Makefile

--- a/stdio-common/Makefile	2010-05-04 05:27:23.000000000 -0600

+++ b/stdio-common/Makefile	2012-02-20 21:57:52.983040992 -0700

@@ -60,7 +60,7 @@ 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

+	 scanf16 scanf17 tst-setvbuf1 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 <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;

+

+  /* 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