Checking Allocations & Potential for Int Mayhem

Must be synchronicity. I started out the day with a really interesting mail from Chris Wysopal talking about how allocations can go wrong, fun with signed int math, and the new[] operator. Once I got done responding to Chris, I then notice Robert Hensing's blog pointing me to Thomas Ptacek's comments about Mark Dowd's new exploit, found at https://www.matasano.com/log/1032/this-new-vulnerability-dowds-inhuman-flash-exploit/.

Some people were even saying that it makes no sense to check returns from malloc. I'm not sure what psychotropic substances were involved in that conclusion, but I don't want any. Let's look at all the ways this stuff can go wrong.

First, let's assume a non-throwing new (or new[]), or a malloc call. If you don't check the return and it fails, then we have roughly the following 3 categories, all of which are bad:

1. No one adds anything user-controlled to the pointer, and it just gets used directly (or null + small, fixed offset). Your program crashes. If some other vulnerability exists, maybe you execute arbitrary code on the way out. I've written about this sort of thing before here and here. This is bad, customer is not happy, and if it is EVER used on a server, it is very bad.
2. Someone adds a user-controlled value to the offset (and you didn't validate that, also bad). We now have an arbitrary memory write, and while Mr. Dowd is LOTS better at shell code than I want to be, making an exploit from one of these has the hard part mostly done.
3. Next, and this is an interesting twist that's obvious once you think about it, is when someone then does this – ptr->array[offset], where offset is user controlled. Having ptr be null could actually even be an advantage, since you have an absolute starting point!

Now let's consider that we take the code and port it to 64-bit. On 32-bit, we can assume a 2GB alloc is going to fail. It has to. It's normal for an allocator to have code along these lines:

if( (ptrdiff_t)size < 0 ) fail_alloc;

Now go and change things to 64-bit, and a 2GB alloc might actually succeed, and there's actually no knowing what the upper bound might really be. You can now no longer say that 2GB ought to be enough for anybody, because it might not. Thus, in the non-throwing case, not checking returns is asking to get hacked, especially on 64-bit.

Next, let's consider the throwing case. This is something I was talking about with Richard van Eeden that's an example of bad code. He pointed this out:

Foo* pFoo;

try{ pFoo = new Foo;}

catch(…){delete pFoo;}

Obviously, there would be more to it than this, but what happens if new – or just as importantly the Foo constructor – throws, then we delete pFoo, which is uninitialized, and we should ALWAYS consider uninitialized variables to be attacker controlled. The problem here is that the programmer did not recognize the try-catch as a branch, and didn't ensure that pFoo was initialized everywhere. Since this is moderately subtle, it's worth checking for in an audit.

Now on to an interesting wrinkle – what about that throwing constructor? It's my preference not to do this if I can avoid it, but YMMV. So here's something interesting – now say someone wrote this:

class Foo

{

public:

Foo()

{

p1 = new Thing1;

p2 = new Thing2;

}

~Foo()

{

delete[] p1;

delete[] p2;

}

Thing1* p1;

Thing2* p2;

};

If the first allocation succeeds, and the second fails (or could be more complex constructors), then because the constructor never completed, the destructor never runs. This is thus dangerous code (especially if the first thing had global side-effects external to the app). There are two correct ways to fix it. The first is to properly use resource holders, like auto_ptr, and those will properly clean up even if the actual containing class destructor never runs. This is nicer, and you should _always_ use these when working with exceptions. The second way to make it work is to not do anything that can fail in the constructor, mark it throw(), and provide an init() method. Now the destructor has to run, things get cleaned up, even if init only makes it ½ way.

This leads me to my second main point, which is that if you're using throwing allocators, then you MUST write exception-safe code. If you do not, then something is going to get in a bad state when the stack unwinds, and it's quite often exploitable.

I hope I've established pretty firmly that not checking allocation returns is a very bad thing, and that not using proper exception safe code when throwing exceptions (assuming that you're not using the exception as a proxy for CatchFireAndBurn() ) is also a prescription for disaster.

Lastly, something else that's an interesting side-effect of the fact that the VS 2005 (and later) new[] implementation detects int overflows, we can actually use it in some respects to check math. So let's say I pass an int into new[]. First problem is that if the int is negative, the allocation is going to fail as being too large, even if sizeof(element) == 1. If it succeeds, we know the value is >= 0. Next, count * sizeof(element) has to not overflow. For this to be true, count <= INT_MAX/sizeof(element), also has to be true, and we may then be able to leverage that to avoid having to do redundant checks on count further along in the function IFF we checked our returns.