Closed Bug 511689 (CVE-2009-3373) Opened 11 years ago Closed 11 years ago

GIF Color Map Parsing Buffer Overflow Vulnerability -- iDefense [V-jb9pzo6q1s]


(Core :: Graphics, defect)

Not set



Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .4+
status1.9.1 --- .4-fixed


(Reporter: bsterne, Assigned: joe)



(4 keywords, Whiteboard: [sg:critical])


(2 files, 2 obsolete files)

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


iDefense VCP Submission V-jb9pzo6q1s
Mozilla Firefox GIF Color Map Parsing Buffer Overflow Vulnerability

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

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.

Assignee: nobody → joe
blocking1.9.1: --- → ?
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
Flags: blocking1.9.0.15?
Flags: blocking1.9.0.14?
Keywords: crash
Summary: Investigate iDefense [V-jb9pzo6q1s] GIF Color Map Parsing Buffer Overflow Vulnerability → GIF Color Map Parsing Buffer Overflow Vulnerability -- iDefense [V-jb9pzo6q1s]
Whiteboard: [sg:critical]
Flags: blocking1.9.0.15?
Flags: blocking1.9.0.15+
Flags: blocking1.9.0.14?
blocking1.9.1: ? → .4+
Here is also some new analysis from the reporter:


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.
Attached patch Avoid the exploit (obsolete) — Splinter Review
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.
Attachment #396965 - Flags: review?(joe)
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.
Attachment #396965 - Flags: review?(joe) → review-
(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.
Attached patch always increment images_decoded (obsolete) — Splinter Review
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).
Attachment #396965 - Attachment is obsolete: true
Attachment #397107 - Flags: review?(jmuizelaar)
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.
Attachment #397107 - Flags: review?(jmuizelaar) → review-
(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.
Attachment #397107 - Attachment is obsolete: true
Attachment #397185 - Flags: review?(jmuizelaar)
Comment on attachment 397185 [details] [diff] [review]
Add explanatory comments

here you go.
Attachment #397185 - Flags: review?(jmuizelaar) → review+
Attachment #397185 - Flags: review+ → review?(alfredkayser)
Attachment #397185 - Flags: superreview?(vladimir)
Attachment #397185 - Flags: review?(alfredkayser) → review+
Attachment #397185 - Flags: superreview?(vladimir) → superreview+
Closed: 11 years ago
Resolution: --- → FIXED
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)?
Flags: blocking1.9.2?
Version: 1.9.1 Branch → Trunk
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.
(Same comment goes for 1.9.0.)
Depends on: 472590
This is a patch that applies to both 1.9.0 and 1.9.1; it depends on the patch from bug 472590.
Attachment #402014 - Flags: approval1.9.1.4?
Attachment #402014 - Flags: approval1.9.0.15?
Comment on attachment 402014 [details] [diff] [review]
1.9.1/1.9.0 branch patch

Approved for and, a=dveditz
Attachment #402014 - Flags: approval1.9.1.4?
Attachment #402014 - Flags: approval1.9.1.4+
Attachment #402014 - Flags: approval1.9.0.15?
Attachment #402014 - Flags: approval1.9.0.15+
Checking in nsGIFDecoder2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v  <--  nsGIFDecoder2.cpp
new revision: 1.106; previous revision: 1.105
Keywords: fixed1.9.0.15
Flags: blocking1.9.2? → blocking1.9.2+
Does not seem to affect 1.8.0, images_decoded is outside the EndImageFrame() method here and it's always incremented.
Verified fix for 1.9.1. Testcase crashes but doesn't crash build 3 of Tested on OS X 10.6.
That was 1.9.0 above actually but I have verified for as well (and compared against
Keywords: verified1.9.1
Alias: CVE-2009-3373
Flags: wanted1.8.1.x-
Flags: wanted1.8.1.x+
Severity: normal → critical
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.3a1
Group: core-security
You need to log in before you can comment on or make changes to this bug.