Re: incorrect integer conversions in OpenSSL can result in memory corruption.

[Jeffrey Walton <noloader () gmail com>]

On Sat, Apr 21, 2012 at 5:50 PM, Zach C. <fxchip () gmail com> wrote:
> Well, not cleanly... I would think though that a signed integer cast to a size_t would have unpredictable results 
> (but mostly just a larger value than intended...). At least when size_t and int are both 32bit. Or am I wrong?
It depends on the context and the result. If the attacker forces -2 as
a size which is subsequently interpreted as a size_t (0xffff fffe),
then your server might brick. If the context allows something more
interesting, such as "add offset to base and then copy the string"
then you might get a remote code execution (remember, data execution
is the norm under Linux, not data execution prevention).

The one thing that was not clear to me (I would need to see the whole
function): did CRYPTO_realloc_clean check to see if length was <0? If
so, the cast was probably OK.

Parameter validation (a CompSci 101 skill) is often overlooked, and I
suspect the string pointer could be NULL and the length could be
negative and CRYPTO_realloc_clean would truck on as if everything was
OK. It's right up there with free'ing a NULL pointer (its legal C/C++,
but makes no sense and demonstrates a level of sloppiness which
usually heightens my awareness when reviewing code).

-Wall -Wextra -Wconversion -Wstrict-overflow -Wformat=2
-Wformat-security is your friend when using GCC. But don't try it with
OpenSSL - it can't clean compile. Apparently, a clean compile with
reasonable warnings is not a security gate for the project.

Jeff

> On Apr 21, 2012, at 2:33 PM, Jeffrey Walton <noloader () gmail com> wrote:
>
> > On Thu, Apr 19, 2012 at 10:32 AM, Benjamin Kreuter
> > <ben.kreuter () gmail com> wrote:
> > > -----BEGIN PGP SIGNED MESSAGE-----
> > > Hash: SHA512
> > >
> > > On Thu, 19 Apr 2012 12:35:22 +0200
> > > Tavis Ormandy <taviso () cmpxchg8b com> wrote:
> > >
> > > > All versions of OpenSSL on all platforms up to and including version
> > > > 1.0.1 are affected.
> > >
> > > [snip]
> > >
> > > > BUF_MEM_grow_clean accepts a size_t, but the subroutine it uses to
> > > > handle the allocation only accepts a 32bit signed integer.
> > >
> > > Correct me if I am wrong, but shouldn't this only be a problem on
> > > systems where a size_t is wider than an int i.e. not on 32 bit systems?
> > I don't believe so (that is, it can be a problem on 32 bit systems),
> > but I'd need to see more context. For example, if the attacker
> > controls the size and forces the size to negative (due to use of an
> > int), then it will never convert to a size_t.
> >
> > void *CRYPTO_realloc_clean(void *str, int old_len, int num, const char
> > *file, int line)
> > {
> >   /* ... */
> >   ret=malloc_ex_func(num,file,line);
> >   if(ret)
> >       {
> >       memcpy(ret,str,old_len);
> >       OPENSSL_cleanse(str,old_len);
> >       free_func(str);
> >       }
> >   /* ... */
> >   return ret;
> > }
> >
> > _______________________________________________
> > Full-Disclosure - We believe in it.
> > Charter: http://lists.grok.org.uk/full-disclosure-charter.html
> > Hosted and sponsored by Secunia - http://secunia.com/

_______________________________________________
Full-Disclosure - We believe in it.
Charter: http://lists.grok.org.uk/full-disclosure-charter.html
Hosted and sponsored by Secunia - http://secunia.com/