How can we use IsBadWritePtr to fix a buffer overflow, if IsBadWritePtr is itself bad?
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 IsBadWritePtr, but then they saw that IsBadXxxPtr should really be called CrashProgramRandomly. They were wondering what alternatives existed to IsBadXxxPtr.
The alternative to IsBadXxxPtr 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.
Light
Dark
15 comments
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!
Burroughs seem to have had invented and implemented all sorts of interesting and useful stuf… and then they went bankrupt and everyone forgot about them. Rather sad.
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.
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?
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.
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.
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.
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.)
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
I don’t really mind people top-posting or bottom-posting. The bottomline is: just don’t mix it.
That’s why I hate Yahoo’s webmail a lot.
Raymond, thanks for enabling the full blog entry text in RSS feed!
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!
Dear Raymond, please tell Adobe to supply more of those apps that use IsBadWritePtr to address their buffer overflows.Signed, (RedTeam) Hackers.
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.
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.