Closed
Bug 255067
Opened 21 years ago
Closed 21 years ago
BMP integer overflow exploits
Categories
(Core :: Graphics: ImageLib, defect)
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)
1.11 KB,
image/bmp
|
Details | |
2.73 KB,
patch
|
dveditz
:
review+
scc
:
superreview+
dveditz
:
approval-aviary+
dveditz
:
approval1.7.5+
|
Details | Diff | Splinter Review |
------------------------------------------------------------
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).
Assignee | ||
Comment 1•21 years ago
|
||
Presuming this will block everything
Flags: blocking1.8a3+
Flags: blocking1.7.3+
Flags: blocking-aviary1.0PR+
Comment 2•21 years ago
|
||
caillion, tor, pav can you help on this one?
Assignee | ||
Comment 3•21 years ago
|
||
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 | ||
Comment 4•21 years ago
|
||
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
Assignee | ||
Comment 5•21 years ago
|
||
btw, nsICODecoder.cpp has the same problems
Comment 6•21 years ago
|
||
could you lend some cycles to looking at other decoders
Comment 7•21 years ago
|
||
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.
Assignee | ||
Comment 8•21 years ago
|
||
Assignee | ||
Comment 9•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
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?
Assignee | ||
Updated•21 years ago
|
Attachment #156339 -
Flags: review?(cbiesinger) → review?(pavlov)
Comment 10•21 years ago
|
||
(In reply to comment #5)
> btw, nsICODecoder.cpp has the same problems
Your patch doesn't address this, does it?
Assignee | ||
Comment 11•21 years ago
|
||
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 12•21 years ago
|
||
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?
Updated•21 years ago
|
Flags: blocking1.8a3+ → blocking1.8a3-
Comment 13•21 years ago
|
||
Comment on attachment 156339 [details] [diff] [review]
Fix the overflows
unsetting 1.8a3 approval request. we've shipped already.
Attachment #156339 -
Flags: approval1.8a3?
Assignee | ||
Comment 14•21 years ago
|
||
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)
Updated•21 years ago
|
Whiteboard: [sg:fix] → [sg:fix] have patch - need reviews pavlov tor
Comment 15•21 years ago
|
||
I think we should support at least 64k values. 32k seems like it doesn't have
much room to grow..
Assignee | ||
Comment 16•21 years ago
|
||
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?
Comment 17•21 years ago
|
||
pav said 64 is good yesterday.
Comment 18•21 years ago
|
||
yeah, r='d with that
Assignee | ||
Comment 19•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #156339 -
Attachment is obsolete: true
Assignee | ||
Comment 20•21 years ago
|
||
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?
Assignee | ||
Comment 21•21 years ago
|
||
Comment on attachment 157135 [details] [diff] [review]
incorporate review comments
adding r=pav from comment
Comment 22•21 years ago
|
||
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+
Assignee | ||
Comment 23•21 years ago
|
||
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+
Assignee | ||
Comment 24•21 years ago
|
||
Fix checked in to trunk, aviary branch, 1.7 branch and 1.7.2 branch
Status: NEW → RESOLVED
Closed: 21 years ago
Keywords: fixed-aviary1.0,
fixed1.7.3
Resolution: --- → FIXED
Whiteboard: [sg:fix] have patch - need reviews pavlov tor → [sg:fix] fixed1.7.2+
Comment 25•21 years ago
|
||
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?
Comment 26•21 years ago
|
||
(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
Comment 27•20 years ago
|
||
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.
Comment 28•20 years ago
|
||
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.
Comment 29•20 years ago
|
||
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.
Comment 30•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Group: security
Severity: critical → enhancement
Whiteboard: [sg:fix] fixed1.7.2+ → [sg:fix] fixed1.7.3
Assignee | ||
Updated•20 years ago
|
Severity: enhancement → critical
Updated•20 years ago
|
Attachment #156339 -
Flags: superreview?(tor)
Attachment #156339 -
Flags: review?(pavlov)
Assignee | ||
Updated•20 years ago
|
Keywords: fixed1.7.5 → fixed1.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.
Description
•