This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] vfprintf stack overflow [BZ #16617]


On Fri, Dec 05, 2014 at 05:43:41PM +0100, Florian Weimer wrote:
> This fell through the cracks.  I took Jeff Law's patch (which we
> carry as a local patch in Fedora and downstream), compressed the
> bug23-3.c test case, and added Joseph's test case from the bug as
> bug23-4.c.
> 
> Tested on x86_64-redhat-linux-gnu, with no regressions.
> 
> -- 
> Florian Weimer / Red Hat Product Security

> >From 3fcae31c646f9419b5201c9189f961487f2b6743 Mon Sep 17 00:00:00 2001
> From: Jeff Law <law@redhat.com>
> Date: Fri, 5 Dec 2014 15:49:34 +0100
> Subject: [PATCH] CVE-2012-3406: Stack overflow in vfprintf [BZ #16617]
> 
> A larger number of format specifiers coudld cause a stack overflow,
> potentially allowing to bypass _FORTIFY_SOURCE format string
> protection.
>

snip
 
> @@ -1722,10 +1728,25 @@ do_positional:
>  	  {
>  	    /* Extend the array of format specifiers.  */
>  	    struct printf_spec *old = specs;
> -	    specs = extend_alloca (specs, nspecs_size, 2 * nspecs_size);
> +	    if (__libc_use_alloca (2 * nspecs_size))
> +	      specs = extend_alloca (specs, nspecs_size, 2 * nspecs_size);
> +	    else
> +	      {
> +		nspecs_size *= 2;
> +		specs = malloc (nspecs_size);

No check to return with errno=ENOMEM?

> +	      }
>  
>  	    /* Copy the old array's elements to the new space.  */
>  	    memmove (specs, old, nspecs * sizeof (*specs));
> +
> +	    /* If we had previously malloc'd space for SPECS, then
> +	       release it after the copy is complete.  */
> +	    if (specs_malloced)
> +	      free (old);
> +
> +	    /* Now set SPECS_MALLOCED if needed.  */
> +	    if (!__libc_use_alloca (nspecs_size))
> +	      specs_malloced = true;
>  	  }
>  
>  	/* Parse the format specifier.  */
> @@ -2046,6 +2067,8 @@ do_positional:
>    }
>  
>  all_done:
> +  if (specs_malloced)
> +    free (specs);
>    if (__glibc_unlikely (args_malloced != NULL))
>      free (args_malloced);
>    if (__glibc_unlikely (workstart != NULL))
> -- 
> 1.9.3
> 

Otherwise I am ok with that change, I would drop extend_alloca as code
is simpler (and bump initial size to 126). 

Performance-wise a memmove dominates if realloc that does not move it could be faster.

Only problem with that is printf in signal handlers but I do not belive that somebody
needs that much specifiers.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]