255067 - BMP integer overflow exploits

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Veditz [:dveditz]]

------------------------------------------------------------
Security Audit of Mozilla's .bmp image parsing (August 2004) 
by Gael Delalleau  <[e-mail]>
------------------------------------------------------------


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).

Comment 1

[Daniel Veditz [:dveditz]]

Presuming this will block everything

Comment 2

[chris hofmann]

caillion, tor, pav can you help on this one?

Comment 3

[Daniel Veditz [:dveditz]]

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.

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[Daniel Veditz [:dveditz]]

btw, nsICODecoder.cpp has the same problems

Comment 6

[chris hofmann]

could you lend some cycles to looking at other decoders

Comment 7

[chris hofmann]

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.

Comment 8

[Daniel Veditz [:dveditz]]

Attachment: sample bmp mentioned in original report (CRASH, exploitable)

Comment 9

[Daniel Veditz [:dveditz]]

Attachment: Fix the overflows

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #5)
> btw, nsICODecoder.cpp has the same problems

Your patch doesn't address this, does it?

Comment 11

[Daniel Veditz [:dveditz]]

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

[[e-mail]]

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?

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 156339 [details] [diff] [review]
Fix the overflows

unsetting 1.8a3 approval request. we've shipped already.

Comment 14

[Daniel Veditz [:dveditz]]

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)

Comment 15

[Stuart Parmenter]

I think we should support at least 64k values.  32k seems like it doesn't have
much room to grow..

Comment 16

[Daniel Veditz [:dveditz]]

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

[chris hofmann]

pav said 64 is good yesterday.

Comment 18

[Stuart Parmenter]

yeah, r='d with that

Comment 19

[Daniel Veditz [:dveditz]]

Attachment: incorporate review comments

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 157135 [details] [diff] [review]
incorporate review comments

adding r=pav from comment

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 157135 [details] [diff] [review]
incorporate review comments

adding r=pav from comment

Comment 22

[Scott Collins]

Comment on attachment 157135 [details] [diff] [review]
incorporate review comments

sr=scc ... pending changing 0xFFFF limit test to a const that ensures
appropriate signedness

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 157135 [details] [diff] [review]
incorporate review comments

Adding branch approvals from meeting earlier today

Comment 24

[Daniel Veditz [:dveditz]]

Fix checked in to trunk, aviary branch, 1.7 branch and 1.7.2 branch 

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 156339 [details] [diff] [review]
Fix the overflows

please don't request approval until you have a fully reviewed patch. thanks.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

(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

[Christian :Biesinger (don't email me, ping me on IRC)]

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

[Gaël Delalleau]

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

[Gaël Delalleau]

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

[chris hofmann]

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.

Comment 31

[Tracy Walker [:tracy]]

verified on windows 1.7.5 12/15