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

[Andreas Jaeger]

On 02/16/2012 05:16 PM, 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

So a security issue - can we get this fixed quickly, please? I'd like to 
ping for a review and commit!


Kees, thanks for the patch.

 > [...]
> 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
 > [...]
> @@ -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];

don't you have an off-by-one error here? You allocate nargs arguments 
and access [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);

Andreas
-- 
  Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
   SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
    GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
     GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126