This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] vfprintf: validate nargs and maybe allocate from heap
- From: Andreas Jaeger <aj at suse dot com>
- To: Kees Cook <kees at outflux dot net>
- Cc: "Ryan S. Arnold" <ryan dot arnold at gmail dot com>,libc-alpha at sourceware dot org, Paul Eggert <eggert at cs dot ucla dot edu>,Roland McGrath <roland at hack dot frob dot com>,Andreas Schwab <schwab at linux-m68k dot org>
- Date: Fri, 02 Mar 2012 16:58:18 +0100
- Subject: Re: [PATCH] vfprintf: validate nargs and maybe allocate from heap
- References: <20120206062537.GM4979@outflux.net> <20120207000509.GP4989@outflux.net> <20120210192457.GF20420@outflux.net> <CAAKybw8AgkGsKAx=kvX4Tsi74f+HtuVnatTCB0VfsHi7vVFi1Q@mail.gmail.com> <20120214223048.GM20420@outflux.net> <CAAKybw_HS+cav+YcDw3ns7UXu6_xA7EHPrkiB87P+OGwEB0PVQ@mail.gmail.com> <20120214224543.GN20420@outflux.net> <20120216161613.GZ20420@outflux.net>
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