Re: [PATCH] vfprintf: validate nargs and positional offsets

[Paul Eggert <eggert at cs dot ucla dot edu>]

On 02/02/2012 03:15 PM, Kees Cook wrote:
 
> +    /* Check for potential integer overflow.  */
> +    if (nargs > SIZE_MAX / (2 * sizeof (int) + sizeof (union printf_arg)))
> +      {
> +         done = -1;
> +         goto all_done;
> +      }

I don't see how this is reliable in general.  First, there's no
integer calculation later such that integer overflow will occur
if nargs == X + 1 but will not occur if nargs == X,
where X is SIZE_MAX / (2 * sizeof (int) + sizeof (union printf_arg))).
So why bother to check whether nargs > X?

Second, the check is immediately followed by several calls to alloca,
and on many hosts alloca has untoward behavior well before integer
overflow occurs; all one needs to do is to alloca enough memory that the
stack pointer grows past the allocated stack (jumping past any guard page
at the end of the stack).  The above code doesn't prevent this stack-overflow
problem, so how is it attacking the bug in question?

>      /* Allocate memory for the argument descriptions.  */
>      args_type = alloca (nargs * sizeof (int));
>      memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
> @@ -1715,13 +1722,17 @@ do_positional:
>      for (cnt = 0; cnt < nspecs; ++cnt)
>        {
>  	/* If the width is determined by an argument this is an int.  */
> -	if (specs[cnt].width_arg != -1)
> +	if (specs[cnt].width_arg > -1 && specs[cnt].width_arg < nargs)
>  	  args_type[specs[cnt].width_arg] = PA_INT;
>  
>  	/* If the precision is determined by an argument this is an int.  */
> -	if (specs[cnt].prec_arg != -1)
> +	if (specs[cnt].prec_arg > -1 && specs[cnt].prec_arg < nargs)
>  	  args_type[specs[cnt].prec_arg] = PA_INT;

I don't see why the above two changes are needed.  Doesn't
__parse_one_specwc guarantee that specs[cnt].width_arg < max_ref_arg
(and therefore specs[cnt].width_arg < nargs), and similarly for
specs[cnt].prec_arg?

> +	/* Sanity-check the data_arg location.  */
> +	if (specs[cnt].ndata_args && specs[cnt].data_arg >= nargs)
> +	  continue;

Likewise; we already know that specs[cnt].ndata_args must be less than
max_ref_arg (and therefore less than nargs), no?