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.

15 comments

Comments are closed.

  • gumpyx gus
    gumpyx gus

    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

        If I recall correctly, some ex US Treasurer merged Burroughs and Univac and stripped $400 million in cash out of them.  I don’t know what’s worse, giving yourself an unearned paycheck of that size, or merging such two incompatible companies.  Univac had like an 18-bit product line of computers for Navy ships and a 36-bit series for scientific and general business use, with 6 and 8-bit character sets.   Burroughs had a stack-based 48-bit machine with EBCDIC internally and ASCII external character sets.  Not exactly a marriage made in computer heaven.  

  • gumpyx gus
    gumpyx gus

    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
      Em X

      The way to get that has been to use a managed language. There are lots of choices of languages that enforce buffer sizes for you — though like Raymond says, only you know whether you want the program to truncate, raise an exception, or terminate on this kind of error.

  • Avatar
    David Walker

    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
      Mike Shawaluk

      I observed the same behavior when I was viewing the blog while not logged in. However, once I logged in with my Microsoft Live account, it flipped to “oldest post first” mode. It also gives the option of logging in using a Google or Facebook ID.

      • Avatar
        Ralf Berkvens

        The devs are working on it 🙂 Got an e-mail from one of them after sending some feedback saying that they are working on oldest-post-first, and that the “load more” (comments) button has been fixed! (And it seemingly has.)

  • Avatar
    steve heller

    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

    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.