511689 - (CVE-2009-3373) GIF Color Map Parsing Buffer Overflow Vulnerability -- iDefense [V-jb9pzo6q1s]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Brandon Sterne (:bsterne)]

This vulnerability was reported by iDefense to the [e-mail] alias.  I have replied requesting a testcase.  Credit should go to regenrecht.

-----

iDefense VCP Submission V-jb9pzo6q1s
08/20/2009
Mozilla Firefox GIF Color Map Parsing Buffer Overflow Vulnerability

Description: 
Remote exploitation of a buffer overflow in the Mozilla Foundation's libpr0n image processing library allows attackers to execute arbitrary code.

The libpr0n GIF parser was designed using a state machine which is represented as a series of switch/case statements. One particularly interesting state, 'gif_image_header', is responsible for interpreting a single image/frame description record. One of the fields, 'depth', specifies the number of bits per pixel in the image. A single GIF file may contain many images, each with a different color map associated.

Consider the source code from "mozilla\modules\libpr0n\decoders\gif\nsGIFDecoder2.cpp":

        mGIFStruct.local_colormap_size = 1 << depth;
        PRUint32 paletteSize;
        if (mGIFStruct.images_decoded) {
          // Copy directly into the palette of current frame,
          // by pointing mColormap to that palette.
          mImageFrame->GetPaletteData(&mColormap, &paletteSize);
        } else {
          // First frame has local colormap, allocate space for it
          // as the image frame doesn't have its own palette
          paletteSize = sizeof(PRUint32) << realDepth;
          if (!mGIFStruct.local_colormap) {
            mGIFStruct.local_colormap = (PRUint32*)PR_MALLOC(paletteSize);
            if (!mGIFStruct.local_colormap) {
              mGIFStruct.state = gif_oom;
              break;
            }
          }
          mColormap = mGIFStruct.local_colormap;
        }
        const PRUint32 size = 3 << depth;
        if (paletteSize > size) {
          // Clear the notfilled part of the colormap
          memset(((PRUint8*)mColormap) + size, 0, paletteSize - size);
        }
        if (len < size) {
          // Use 'hold' pattern to get the image colormap
          GETN(size, gif_image_colormap);
          break;
        }
        // Copy everything, go to colormap state to do CMS correction
        memcpy(mColormap, buf, size);

The problem here is that it is possible to enter the 'gif_image_header' state more than once while still having 'mGIFStruct.images_decoded' be equal to zero. Normally 'images_decoded' is incremented after every whole image is decoded. However, by providing invalid LZW data, one can skip this step and enter the 'gif_image_header' state again.
 
On the second iteration, the branch with 'PR_MALLOC()' will not be taken since the local color map was already allocated in the previous iteration. As a result, the memory allocated for the color map remains the same size regardless of the 'depth' field of a second image. When the second image is processed, data is copied into the buffer without respect for the buffer's size. This results in an exploitable heap overflow condition.

Analysis: 
Exploitation of this vulnerability results in the execution of arbitrary code with the privileges of the user running the vulnerable application. To exploit this vulnerability, a targeted user must load a malicious Web page created by an attacker. An attacker typically accomplishes this via social engineering or injecting content into compromised, trusted sites.

iDefense confirmed the existence of this vulnerability using Mozilla Firefox versions 3.0.13 and 3.5.2 on 32-bit Windows XP SP3. Other versions, and potentially other applications using libpr0n, are suspected to be vulnerable.

Credit: 
regenrecht

Comment 2

[Brandon Sterne (:bsterne)]

Here is also some new analysis from the reporter:

-------
Exploit
-------

First step is to prepare malformed GIF. It will consist of 2 images. First one has to have associated a small color map and some dummy LZW data - just enough to force decompressor to ignore image data following the header, but let him
keep on parsing further. Second image will contain a bigger color map, BUT!, without the last byte. Chopping that last byte is quite crucial. Otherwise execution would go to function ConvertColormap(), where our freshly overflown buffer full of RGBRGBRGB... (red, green and blue 8-bit components) would turn into RGBARGBARGBA... (RGB + alpha channel) with alpha set to 0xFF. And changing any important structure or function pointer to 0xFFbbggrr would not be useful as it points outside the user space.

When the browser asks for our malicious web page, we send her few lines of JavaScript that will spray the heap a bit. The idea is to store data that could be interpreted as a pointer to itself and at the same time as a legal stream of nop-equivalent CPU instructions. At the end of which shellcode will be appended.

And then we ask browser to decode a GIF. GIF's second local color map overflows allocated buffer and overwrites important stuff with, let's say, 0x08080808 (at that address we should already have few megabytes of 0x08 and shellcode at the end). So, 0x08080808 points at 0x08080808 and at the same time means something like "OR [EAX],CL", which can be treated in out context as a nop sled. CPU then slides down the hill and gets to shellcode.

Windows' calc.exe should be started. Usually it works. For a clean run Firefox (without some other web pages messing with memory and defragmenting the heap) it seems to be quite reliable proof of concept.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Avoid the exploit

So it looks like this problem was added with bug 143046.

The code here seems pretty tricky to understand and lacks tests as shown by bug 408310 and bug 403578. It seems like we could definitely use some more thorough tests. Do we have a gif fuzzer?

This patch avoids the heap overflow by always allocating MAX_COLORS. It also sets mColormapSize unconditionally of if (!mGIFStruct.images_decoded). (I'm not certain of the implications of this, but it seems more correct?)

I don't think this is necessarily the best approach but it seems like a pretty low risk way to fix this for right now.

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 396965 [details] [diff] [review]
Avoid the exploit

This doesn't feel like the right way to solve this bug. This is a problem in the GIF state machine, so we should solve it there.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #4)
> (From update of attachment 396965 [details] [diff] [review])
> This doesn't feel like the right way to solve this bug. This is a problem in
> the GIF state machine, so we should solve it there.

I'm not convinced that there is a problem in the state machine. Older code took into account resizing the color map when the palette depth increased.

Comment 6

[Joe Drew (not getting mail)]

Attachment: always increment images_decoded

Whenever we have errors decoding frames, images_decoded gets out of sync with the number of frames actually added to the imgContainer, which could actually cause issues further down the line. This patch simply always increments images_decoded, which solves this problem (and others like it).

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 397107 [details] [diff] [review]
always increment images_decoded

This could use more explanation in the code as it's riskier then the previous patch.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #7)
> (From update of attachment 397107 [details] [diff] [review])
> This could use more explanation in the code as it's riskier then the previous
> patch.

r- my comment for not being able to spell.

Comment 9

[Joe Drew (not getting mail)]

Attachment: Add explanatory comments

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 397185 [details] [diff] [review]
Add explanatory comments

here you go.

Comment 11

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/1fa7bd42a36c

Comment 12

[Samuel Sidler (old account; do not CC)]

Joe: Can you get this landed on 1.9.2 (get approval or get it blocking) and get a patch ready for 1.9.1 and 1.9.0 if the current one doesn't apply (if it does, please just request approval)?

Comment 13

[Joe Drew (not getting mail)]

This patch depends on bug 472590. It's possible to fix it without that bug's patch, but it'll require some hackery. I'm moderately OK with that, but I'd rather we get the benefit of that bugfix.

Comment 14

[Joe Drew (not getting mail)]

(Same comment goes for 1.9.0.)

Comment 15

[Joe Drew (not getting mail)]

Attachment: 1.9.1/1.9.0 branch patch

This is a patch that applies to both 1.9.0 and 1.9.1; it depends on the patch from bug 472590.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 402014 [details] [diff] [review]
1.9.1/1.9.0 branch patch

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz

Comment 17

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/58003e3413ca

Comment 18

[Joe Drew (not getting mail)]

Checking in nsGIFDecoder2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v  <--  nsGIFDecoder2.cpp
new revision: 1.106; previous revision: 1.105
done

Comment 19

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3bc177bd871f

Comment 20

[Martin Stránský [:stransky] (ni? me)]

Does not seem to affect 1.8.0, images_decoded is outside the EndImageFrame() method here and it's always incremented.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Verified fix for 1.9.1. Testcase crashes 1.9.0.14 but doesn't crash build 3 of 1.9.0.15. Tested on OS X 10.6.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

That was 1.9.0 above actually but I have verified for 1.9.1.4 as well (and compared against 1.9.1.3).