Re: [PATCH] vfprintf: validate nargs and maybe allocate from heap

[Kees Cook <kees at outflux dot net>]

Hi,

Just checking in on this. Is anyone willing to ACK this patch?

Thanks!

-Kees

On Tue, Feb 07, 2012 at 01:42:09PM -0800, Kees Cook wrote:
> 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.)
> 
> Now with more nits fixed! ;)
> 
> 2012-02-07  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 --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 006f546..593f5d4 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -60,7 +60,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..ad82713
> --- /dev/null
> +++ b/stdio-common/bug-vfprintf-nargs.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..5e7ea8f 100644
> --- a/stdio-common/vfprintf.c
> +++ b/stdio-common/vfprintf.c
> @@ -236,6 +236,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.  */
> @@ -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);
> -- 
> 1.7.5.4
> 
> 
> -- 
> Kees Cook                                            @outflux.net

-- 
Kees Cook                                            @outflux.net