Closed Bug 255067 Opened 21 years ago Closed 21 years ago

BMP integer overflow exploits

Categories

(Core :: Graphics: ImageLib, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: dveditz, Assigned: dveditz)

References

Details

(Keywords: crash, fixed-aviary1.0, fixed1.7.3, Whiteboard: [sg:fix])

Attachments

(2 files, 1 obsolete file)

------------------------------------------------------------ Security Audit of Mozilla's .bmp image parsing (August 2004) by Gael Delalleau <gael.delalleau+moz@m4x.org> ------------------------------------------------------------ Overview -------- This report presents the results of a security audit of a part of the Mozilla 1.7.2 C++ source code tree. This security audit was done during my free time as an attempt to promote and participate to the Mozilla Security Bug Bounty Program (http://www.mozilla.org/security/bug-bounty.html). Several integer overflows were found in the image parsing code of Mozilla. Some of them leads to exploitable security bugs, ranging from Denial of Service (crash of the Mozilla client) to arbitrary code execution on the user's computer. Source code files audited ------------------------- mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.h + parts of various related files Details ------- There are mutiple integer overflows in the code used to parse and display .bmp images. Some of them leads to exploitable security bugs. These integer overflow can be found: - in the .bmp decoder of the libpr0n module - in the OS-specific code used to display images. For instance, the following file is used on Windows systems: mozilla/gfx/src/windows/nsImageWin.cpp I will now highlight all the integer overflow issues found during the audit, and try to show how these various issues finally allow for arbitrary code execution. (1) *** Denial Of Service while parsing a malformed .bmp image *** Vulnerable systems: Linux, Windows XP Other systems: untested * in nsBMPDecoder.cpp line 287: mRow = new PRUint8[(mBIH.width*mBIH.bpp)/8 + 4]; mBIH.width*mBIH.bpp can wrap around and thus the allocated buffer can be made very small. On Linux (Mozilla 1.7.2 developpement version) a negative value in the new[] operator will crash the client (uncatched exception?) Then mFrame->Init() is called, followed by NS_ENSURE_SUCESS, which ensures some sanity checks on the height, width and bpp of the image. However, these checks are not strict enough (they do not prevent mBIH.width*mBIH.bpp from wrapping around). ~ Fortunately on systems using gtk to display images (like Linux) an additional check is done in mImage->Init() which prevents Mozilla to display the image if the width or height is greater than 32767, thus preventing arbitrary code execution. ~ On Windows systems, there is no limit to the height and width of the image. Denial of service and arbitrary code execution are possible as shown below. ~ Other systems: untested. * in nsImageWin.cpp line 160: mRowBytes = CalcBytesSpan(mBHead->biWidth); with : 1498 nsImageWin :: CalcBytesSpan(PRUint32 aWidth) 1499 { 1500 PRInt32 spanBytes; 1503 spanBytes = (aWidth * mBHead->biBitCount) >> 5; (...) 1510 spanBytes <<= 2; 1513 return(spanBytes); 1514 } aWidth * mBHead->biBitCount is actually similar to mBIH.width*mBIH.bpp. It can wrap around, so mRowBytes can be very small. This value finds its way in nsBMPDecoder.cpp in the mBpr variable, which is used in line 374 to allocate a buffer (which can be very small due to the integer wrap around we just explained): mDecoded = (PRUint8*)malloc(mBpr); Thus at lines 420 to 428 we can end in a big loop (lpos==width iterations) which writes bytes to memory pointed by p, which is actually our small mDecoded buffer: 421 while (lpos > 0) { 422 SetPixel(d, p[2], p[1], p[0]); 423 p += 2; 424 --lpos; 425 if (mBIH.bpp == 32) 426 p++; // Padding byte 427 ++p; 428 } The SetPixel function writes the 3 bytes to d and increments d by 3. Thus, memory is being copied from the p buffer allocated in the heap, to the mDecoded buffer allocated later on the same heap. Both buffers can be made very small, so the loop will soon copy a part of the heap to another part of the heap. Consequences: ~ the client will crash when this big loop reaches the end of the allocated memory for the heap (write attempt to an unmapped memory area). ~ it might be possible to trigger arbitrary code execution if another thread uses the corrupted heap before the crash happens, or if we can allocate enough memory on the same heap to make it big enough to avoid the crash. ~ Consequences on systems other than Windows (32 bits): not evaluated. ---------- Side Note: there are other suspicious pieces of code in nsImageWin.cpp, without checks for integer overflows. For instance at line 161: mSizeImage = mRowBytes * mBHead->biHeight; (...) mImageBits = new unsigned char[mSizeImage]; ---------- (2) *** Arbitrary code execution while parsing a malformed .bmp image *** Vulnerable: Windows XP Not vulnerable (?): Linux Other systems: untested With the same bugs, there is a better way to corrupt the heap after the mDecoded pointer with arbitrary values, and without crashing the client. Indeed, we can use a RLE-encoded bitmap image (depth 8 bits, encoded with RLE8, width big enough to make mBpr wrap around, small height to pass the overflow check on width*height. See the sample image file attached to this report). Line 464, the buffer is allocated (but too small!): mDecoded = (PRUint8*)calloc(mBpr, 1); Then we can legitimately use the RLE decoder to write arbitrary data in the mDecoded buffer, with the length limit being mBIH.width which is a big value (much bigger than the size of the buffer). Heap corruption with arbitrary data on the Windows OS is a common situation, known to allow arbitrary code execution. See the two screenshots showing a debugger attached to the mozilla.exe process after the crash; they show that at least two exploitation techniques may be used to get control of EIP and hijack the execution flow of the program: (a) overwriting C++ pointers to the methods of objects stored on the heap. The screenshot shows that we can land in: CALL [EAX+10] where EAX=0x42424242 (our arbitrary data) (b) using standard heap overflow techniques. The screenshot shows that we can land in ntdll.dll in: MOV [ECX], EAX where EAX and ECX are arbitrary values supplied by the attacker in the malicious image file. --------- Side Note: in the RLE decoder of nsBMPDecoder.cpp at line 568, there is: mDecoding += byte * GFXBYTESPERPIXEL; This can be abused to put the mDecoding pointer out of the bounds of the mDecoded buffer. Even though this does not seems to be exploitable with the current surrounding code, it deserves to be fixed. --------- Exploit code ------------ I did not wrote the actual exploit code because: - it is outside the scope of a source code audit - I gave enough information to show such an exploit can be written. The issue is just the same kind as many other heap overflow vulnerabilities, including previous Mozilla vulnerabilities like the SOAPParameter overflow (236618). - in France, the legal status of publishing exploit code is unknown at the moment. Further audit work on the same topic ------------------------------------ - Need to check other image parsers (.jpeg, .gif, .ico, icon, .xpm...) in libpr0n for integer overflows. - Need to check deeper the OS-dependent image display code, for all operating systems (I audited only small parts of the gtk and Win Image code).
Presuming this will block everything
Flags: blocking1.8a3+
Flags: blocking1.7.3+
Flags: blocking-aviary1.0PR+
caillion, tor, pav can you help on this one?
I'm working on a patch for the specificly enumerated spots. What would really help is extra eyes looking for similar problems in the other decoders.
I'm working on a patch for the specificly enumerated spots. What would really help is extra eyes looking for similar problems in the other decoders.
Assignee: jdunn → dveditz
btw, nsICODecoder.cpp has the same problems
could you lend some cycles to looking at other decoders
cbiesinger, can you help look at the other decoders too? going to try and get dveditx and dbaron together tomorrow to come up with a plan.
Keywords: crash
Whiteboard: [sg:fix]
Attached patch Fix the overflows (obsolete) — Splinter Review
Attachment #156339 - Flags: superreview?(tor)
Attachment #156339 - Flags: review?(cbiesinger)
Attachment #156339 - Flags: approval1.8a3?
Attachment #156339 - Flags: approval1.7.3?
Attachment #156339 - Flags: approval-aviary?
Attachment #156339 - Flags: review?(cbiesinger) → review?(pavlov)
(In reply to comment #5) > btw, nsICODecoder.cpp has the same problems Your patch doesn't address this, does it?
The patch doesn't touch ICO. At a closer look ICO files are limited to single byte widths and heights so they don't really have the same overflow issue in what looked like the analogous spot. Not in the same place, anyway. I found bug 245631 that should be fixed just in case it's a similar overflow of some kind.
Comment on attachment 156339 [details] [diff] [review] Fix the overflows > // BMPs with negative width are invalid >- if (mBIH.width < 0) >+ // Reject extremely wide BMP's to prevent overflows >+ if (mBIH.width < 0 || mBIH.width > 0xFFFFFF) > return NS_ERROR_FAILURE; > > PRUint32 real_height = (mBIH.height > 0) ? mBIH.height : -mBIH.height; > rv = mImage->Init(mBIH.width, real_height, mObserver); > NS_ENSURE_SUCCESS(rv, rv); Maybe I misunderstood, but I thought that you were limiting the image to 32K in each direction, so that if you do need this test too then you might as well check against 32K here too?
Flags: blocking1.8a3+ → blocking1.8a3-
Comment on attachment 156339 [details] [diff] [review] Fix the overflows unsetting 1.8a3 approval request. we've shipped already.
Attachment #156339 - Flags: approval1.8a3?
Blocks: sbb?
I *do* want to check in each place. The BMP code has a problem and needs to protect itself, the upper level checks are belt-and-suspenders. I think 32K is too small already, this 30K x 15K image is legit: http://visibleearth.nasa.gov/data/ev58/ev5826_nightearth.gif (Warning, you will probably crash from lack of memory)
Whiteboard: [sg:fix] → [sg:fix] have patch - need reviews pavlov tor
I think we should support at least 64k values. 32k seems like it doesn't have much room to grow..
OK, if I promise to change all the checks to 0xFFFF (including the BMP-specific larger one) can I have r=? or do you want to see another patch?
pav said 64 is good yesterday.
yeah, r='d with that
Attachment #156339 - Attachment is obsolete: true
Comment on attachment 157135 [details] [diff] [review] incorporate review comments adding r=pav from comment
Attachment #157135 - Flags: superreview?
Attachment #157135 - Flags: review+
Attachment #157135 - Flags: approval1.7.3?
Attachment #157135 - Flags: approval-aviary?
Comment on attachment 157135 [details] [diff] [review] incorporate review comments adding r=pav from comment
Comment on attachment 157135 [details] [diff] [review] incorporate review comments sr=scc ... pending changing 0xFFFF limit test to a const that ensures appropriate signedness
Attachment #157135 - Flags: superreview+
Comment on attachment 157135 [details] [diff] [review] incorporate review comments Adding branch approvals from meeting earlier today
Attachment #157135 - Flags: superreview?
Attachment #157135 - Flags: approval1.7.3?
Attachment #157135 - Flags: approval1.7.3+
Attachment #157135 - Flags: approval-aviary?
Attachment #157135 - Flags: approval-aviary+
Fix checked in to trunk, aviary branch, 1.7 branch and 1.7.2 branch
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Whiteboard: [sg:fix] have patch - need reviews pavlov tor → [sg:fix] fixed1.7.2+
Comment on attachment 156339 [details] [diff] [review] Fix the overflows please don't request approval until you have a fully reviewed patch. thanks.
Attachment #156339 - Flags: approval1.7.3?
Attachment #156339 - Flags: approval-aviary?
(In reply to comment #7) > cbiesinger, can you help look at the other decoders too? going to try and get > dveditx and dbaron together tomorrow to come up with a plan. I was on vacation w/o net access until today... I'll try to look at the other decoders during the next few days
possible issues in other decoders- (note: I only looked at issues like the one here, i.e. where too small a buffer is allocated due to overflowing integers) gif: il_BACat adds two size_t and allocs that, and writes into the buffer. If that overflows, data would be written to unallocated memory, possibly overwriting other stuff; but I can't tell whether those two ints come from untrusted sources. gs->gathered, one of the args, seems to be limited to (0xFFFFFFL), so that should be safe. imgContainerGIF.cpp has this code: void imgContainerGIF::BlackenFrame(gfxIImageFrame *aFrame, PRInt32 aX, PRInt32 aY, PRInt32 aWidth, PRInt32 aHeight) [...] const PRUint32 bprToWrite = width * bpp; [...] PRUint8* tmpRow = NS_STATIC_CAST(PRUint8*, nsMemory::Alloc(bprToWrite)); If aWidth were large enough, this would overflow. But, only zeroes would be written to that memory. (why does this not use calloc?). Also, the arguments seem to come always from a gfxIImageFrame's rect, and the patch here limits those sizes. that should be enough defense, I think... jpeg: here we have: row_stride = mInfo.output_width * 3; ... mSamples = (*mInfo.mem->alloc_sarray)((j_common_ptr) &mInfo, JPOOL_IMAGE, row_stride, 1); #if defined(XP_WIN) || defined(XP_OS2) || defined(XP_BEOS) || defined(XP_MAC) || defined(XP_MACOSX) || defined(MOZ_WIDGET_PHOTON) // allocate buffer to do byte flipping / padding mRGBRow = (PRUint8*) PR_MALLOC(row_stride); if output_width is sufficiently high, that int would overflow, and not enouhg memory would be allocated. the previous patch's gfxImageFrame check prevents reaching this code in that case, though. (hm... these two allocations are never checked for success, so in out-of-memory situations, the jpeg decoder owuld likely cause a crash) later in this file: PRUint32 new_backtrack_buflen = src->pub.bytes_in_buffer + src->decoder->mBackBufferLen; if (src->decoder->mBackBufferSize < new_backtrack_buflen) { PRUint32 roundup_buflen = ((new_backtrack_buflen + 15) >> 4) << 4; src->decoder->mBackBuffer = (JOCTET*)PR_MALLOC(roundup_buflen); that addition looks safe, from the names of the variables, since bytes_in_buffer is unlikely to be very high... I haven't verified this though. PNG: decoder->interlacebuf = (PRUint8 *)nsMemory::Alloc(decoder->ibpr*height); looks like exactly the same issue. again, the gfxImageFrame patch in this bug prevents reaching this code if height is too high.
Hi, You added a sanity check in nsImageWin.cpp in ::Init. Maybe you want to do the same with the other OSes, and do some additional audit work on these source code files. For instance I just looked at nsImageMac.cpp, and I saw in nsImageMac::CreateGWorldInternal (called by nsImageMac::Init), we have the same problem : PRInt32 imageSize = rowBytes * aHeight; char* imageBits = (char*)calloc(imageSize, 1); where rowBytes can be ((aWidth * aDepth + 31) / 32) * 4 Even with the current 0xffff limit for the height and width, there is an integer overflow here, which I bet may be a good candidate for arbitrary heap overwrite (not verified, no time left today)... so maybe investigate that and other spots like this before marking the bug as fixed.
Another possibility is not doing any sanity check in the OS-dependent parts, and to do all the sanity checks in gfxImageFrame.cpp (at first sight they seem good enough to prevent this overflow). With the hope gfxImageFrame::Init is always called before the OS-specific parts, and that its failure is always fatal.
now that this bug as been marked fix and the first patch has landed might be good to spin off the addition research and work into new bug(s). that will make it easy to escalate and get on the radar of new releases if any addition al problems are found.
Group: security
Severity: critical → enhancement
Whiteboard: [sg:fix] fixed1.7.2+ → [sg:fix] fixed1.7.3
Severity: enhancement → critical
Blocks: sbb+
No longer blocks: sbb?
Attachment #156339 - Flags: superreview?(tor)
Attachment #156339 - Flags: review?(pavlov)
verified on windows 1.7.5 12/15
Status: RESOLVED → VERIFIED
Keywords: fixed1.7.5fixed1.7.3
Whiteboard: [sg:fix] fixed1.7.3 → [sg:fix]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: