Re: [PATCH] vfprintf: validate nargs and maybe allocate from heap

[Kees Cook <kees at outflux dot net>]

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