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: Kees Cook <kees at outflux dot net>
- To: Andreas Jaeger <aj at suse dot com>
- 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, 2 Mar 2012 10:56:04 -0800
- Subject: Re: [PATCH] vfprintf: validate nargs and maybe allocate from heap
- References: <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><4F50EE1A.90902@suse.com><20120302164858.GB3990@outflux.net><4F510F3D.6040908@suse.com>
On Fri, Mar 02, 2012 at 07:19:41PM +0100, Andreas Jaeger wrote:
> On 03/02/2012 05:48 PM, Kees Cook wrote:
> >On Fri, Mar 02, 2012 at 04:58:18PM +0100, Andreas Jaeger wrote:
> >>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!
> >
> >Ryan has been trying to make some time for a final testing round, so
> >I'm confident a commit will be coming soon.
>
> Ryan, do you see any problems or want specific tests? I just tested
> on x86 and x86-64 and think the patch is fine to commit. I can do
> the commit, just tell me what's holding you up...
BTW, unrelated to this patch, but can someone commit this change too?
http://cygwin.com/ml/libc-alpha/2012-02/msg00269.html
Or, optionally, what do I need to do for commit access, so I can commit
these sorts of things myself? :)
> >>Kees, thanks for the patch.
> >
> >Sure thing!
> >
> >>>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:
> >>>+ bytes_per_arg = sizeof (*args_value) + sizeof (*args_size)
> >>>+ + sizeof (*args_type);
> >...
> >>>+ 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);
> >...
> >>>+ }
> >>>+
> >>>+ 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]
> >
> >This is a bit of type trickery. The allocation covers all three arrays,
> >args_value, args_size, and args_type. This code is setting up the other
> >two pointers to aim just after where the previous array ends. So, yes,
> >it is "past the end", but intentionally so.
>
> That makes sense - could you add a comment to explain this?
Good idea! I've sent an updated patch that adds/updates some comments
in this area.
-Kees
--
Kees Cook @outflux.net