This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] vfprintf stack overflow [BZ #16617]
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: Florian Weimer <fweimer at redhat dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>, Jeff Law <law at redhat dot com>
- Date: Sat, 6 Dec 2014 14:38:59 +0100
- Subject: Re: [PATCH] vfprintf stack overflow [BZ #16617]
- Authentication-results: sourceware.org; auth=none
- References: <5481E0BD dot 9000203 at redhat dot com>
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.