Re: [PATCH] Unbound alloca in crypt routines

[Jeff Law <law at redhat dot com>]

On 03/29/2012 11:11 AM, Marek Polacek wrote:
> On Thu, Mar 29, 2012 at 10:55:43AM -0600, Jeff Law wrote:
> > 	* crypt/md5-crypt (__md5_crypt_r): Avoid unbounded alloca uses
> 			^^
> Missing ".c".
Fixed.

> > 	due to long  keys.
> 		^^
> Redundant space.
Fixed.

> > @@ -120,7 +121,22 @@ __md5_crypt_r (key, salt, buffer, buflen
> >
> >     if ((key - (char *) 0) % __alignof__ (md5_uint32) != 0)
> >       {
> > -      char *tmp = (char *) alloca (key_len + __alignof__ (md5_uint32));
> > +      char *tmp;
> > +
> > +      /* An attacker could use a very long key to clobber another
> > + 	 thread's stack or heap areas.  Punt to malloc if the key is
> > +	 long.  Alloca should abolished.  */
>
> Missing word in last sentence?
Comment WRT abolishing alloca removed in all 3 places.

> > +      if (__libc_use_alloca (key_len + __alignof__ (md5_uint32)))
> > +	{
> > +	  tmp = (char *) alloca (key_len + __alignof__ (md5_uint32));
> > +	}
>
> Those { } aren't really necessary here (and in further code).
Not strictly necessary.  However, I've found that keeping the if and 
else clauses at the same indention level makes the code easier to read. 
 I'll defer to the maintainers on this.

jeff