How can we use IsBadWritePtr to fix a buffer overflow, if IsBadWritePtr is itself bad?

Raymond Chen

Raymond

A customer asked for assistance in investigating an access violation caused by a buffer overflow. They figured that they could probe whether the buffer is large enough to receive the data by using Is­Bad­Write­Ptr, but then they saw that Is­Bad­Xxx­Ptr should really be called Crash­Program­Randomly. They were wondering what alternatives existed to Is­Bad­Xxx­Ptr.

The alternative to Is­Bad­Xxx­Ptr is not passing bad pointers in the first place.

If you are getting an access violation from a buffer overflow, the fix for the problem is not to try to detect the overflow as it happens. the fix is to stop the overflow before it happens.

The customer shared their code and the stack trace at which the access violation occurred:

msvcrt!memcpy+0xb4
contoso!CBuffer::CopyFromRange+0x92
contoso!CBuffer::ReadAt+0x861
contoso!CLockBytes::ReadAt+0xfd
contoso!CStream::Read+0xe3
contoso!CData::ParseFile+0x606

The buffer overflow occurred because the memcpy was writing past the end of the buffer passed to CStream::Read. The thing to do is not try to detect the maximum writable buffer size, but to stop passing invalid buffer sizes.

Because there’s probably writable memory after the buffer that is not part of the buffer. If the invalid buffer size were only slightly larger than the buffer (rather than ridiculously larger than the buffer), you wouldn’t have gotten an access violation, but you still had a buffer overflow.

The offending Read call came from here:

// Code in italics is wrong
    uint32_t numBlocks;
    uint32_t actualBytesRead;

    // First, read the number of blocks.
    HRESULT hr = stream.Read(&numBlocks, sizeof(uint32_t), &actualBytesRead);
    if (FAILED(hr) || actualBytesRead != sizeof(uint32_t)) {
        goto Reject;
    }

    // Next, read the size of each block.
    uint32_t blockSize;
    hr = stream.Read(&blockSize, sizeof(uint32_t), &actualBytesRead);
    if (FAILED(hr) || actualBytesRead != sizeof(uint32_t)) {
        goto Reject;
    }

    // Now read the blocks.
    DWORD i;
    for (i = 0; i < numBlocks; i++)
    {
        // Read each block.
        BLOCK block = { 0 };
        hr = stream.Read(&block, blockSize, &actualBytesRead);
        //               ^^^^^^^^^^^^^^^^^ invalid buffer here
        if (FAILED(hr) || actualBytesRead != sizeof(uint32_t)) {
            goto Reject;
        }

The stack trace implicates the highlighted line of code.

So how do we prevent the invalid buffer from being passed to the Read method?

From code inspection, we see that we read blockSize bytes into a BLOCK structure, but we didn’t take any steps to ensure that blockSize is no larger than at BLOCK. In other words, we have a buffer of size sizeof(BLOCK), and we ask to read blockSize bytes into it, so it is our responsibility to ensure that blockSize <= sizeof(BLOCK).

However, no such buffer size validation was present.

How to fix this depends on how you want to deal with unexpected block sizes.

If your intent is to allow large block sizes and just ignore the fields that are “from the future”, then you would read min(blockSize, sizeof(block)) bytes, and then Seek over the extra bytes (if any).

If your intent is to reject large block sizes, then go ahead and reject them.

Raymond Chen
Raymond Chen

Follow Raymond   

15 Comments
gumpyx gus
gumpyx gus 2019-03-15 07:32:53

Way back before Disco, a company named Burroughs came out with a mainframe computer with hardware-enforced addres limits on every array and structure.  Then in 1982, Intel took a cue from Rick Springfield's tune "Don't talk to Strangers", and they implemented a somewhat-limited version of Burroughs segments.   Microsoft thankfully embraced that memory model for a while, in the vaguely-supported 1989-era DPMI.  Some of us rewrote malloc() to use those hardware segments, and many buffer overflows were automatically and preemptively caught.

Howsomeever the hardware wasn't widely used, so Intel dropped those segments in the 386. I think.   Sigh.  How many billions of buffer overflows and exploits have lived long fruitful lives due to the lack of hardware-protected segments?    Dog only knows.    Intel, please bring back segments, at least as an option for picky systems and programmers!  

gumpyx gus
gumpyx gus 2019-03-15 08:22:42
I should have clarified-- we'd like more than just a few generic segments for stack, code, and data-- separate protections for each data structure would be nice.  Probably would require some hot segment-lookaside registers, so maybe as an option?
Avatar
David Walker 2019-03-15 09:01:23
Is the whole world going to "most recent item on top", like this blog software does?  I don't like that choice... Even my bank does that online, and it just looks backwards.  Oh well.
Avatar
Михаил Макаров 2019-03-15 11:34:51
Raymond, thanks for enabling the full blog entry text in RSS feed!
Avatar
Alexis Ryan 2019-03-17 20:58:49
this seems to be an example of exactly what not to do. read the data size  and then read that much data into a fixed sized buffer in the stack. just perfect for exploitation!
Avatar
Dan B 2019-03-17 21:57:17
Dear Raymond, please tell Adobe to supply more of those apps that use Is­Bad­Write­Ptr to address their buffer overflows.Signed, (RedTeam) Hackers.
Avatar
steve heller 2019-03-20 13:02:56
I studied the Algol-derived systems programming language for one of those Burroughs computers, I think a B7700, for a brief time in the 1970's. I wish we still had those hardware-enforced limits. Maybe FPGAs can help? And no, I'm not interested in managed languages. I want performance, and the only way to get that is by programming as close to the metal as possible.  
Avatar
Dan Weiss 2019-03-23 16:11:00
I have used VirtualQuery as an alternative to IsBadXxxPtr, just because it won't trip guard pages.  For C++ objects, you can also validate the vtable member to see if you were passed a bad pointer or not. Obviously, you want to avoid bad pointers entirely, but sometimes people have time pressures, or are unwilling to try to fix other people's code in an unfamiliar codebase.