Closed Bug 285595 Opened 19 years ago Closed 19 years ago

A buffer overflow vulnerability in GIF processing that can lead to remote compromise.


(Core :: Graphics: ImageLib, defect)

1.7 Branch
Not set





(Reporter: nmehta, Assigned: dveditz)


(6 keywords, Whiteboard: [sg:fix] CAN-2005-0399)


(3 files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1)
Build Identifier: 

The mozilla applications contain several image decoders for displaying images 
of various formats. Amoung the most common image formats is the Graphic 
Information File (GIF) format. The code for this parser is located in:


A vulnerability exists in the parsing of a Netscape-specific extension block 
that specifies a 32-bit size integer indicating how much image data the 
application should buffer, which is shown (taken from the gif_write() 

    /* Parse netscape-specific application extensions */
    case gif_consume_netscape_extension:

      /* Wait for specified # of bytes to enter buffer */
      else if (netscape_extension == 2) {
        gs->requested_buffer_fullness = GETINT32(q + 1);
        GETN(gs->requested_buffer_fullness, gif_wait_for_buffer_full);
      } else

Whilst the retrieval of this integer is fine, a signed comparison occurs when 
performing some buffer management using this 32-bit integer which has been 
read directly from the file. The following code contains the vulnerability:

    case gif_delay:
    case gif_gather:
      PRInt32 gather_remaining;
      PRInt32 request_size = gs->gather_request_size;

        gather_remaining = request_size - gs->gathered;

        /* Do we already have enough data in the accumulation
           buffer to satisfy the request ?  (This can happen
           after we transition from the gif_delay state.) */
           after we transition from the gif_delay state.) */
        if (gather_remaining <= 0) {
          gs->gathered -= request_size;
          q = gs->gather_head;
          gs->gather_head += request_size;
          gs->state = gs->post_gather_state;

        /* Shift remaining data to the head of the buffer */
        if (gs->gathered && (gs->gather_head != gs->hold)) {
          memmove(gs->hold, gs->gather_head, gs->gathered);
          gs->gather_head = gs->hold;

        /* If we add the data just handed to us by the netlib
           to what we've already gathered, is there enough to satisfy
           the current request ? */
        if ((ep - p) >= gather_remaining) {
          if (gs->gathered) { /* finish a prior gather */
            char *hold = (char*)gs->hold;
            BlockAllocCat(hold, gs->gathered, (char*)p, gather_remaining);
            gs->hold = (PRUint8*)hold;
            q = gs->gather_head = gs->hold;
            gs->gathered = 0;
          } else
            q = p;

          p += gather_remaining;
          gs->state = gs->post_gather_state;
        } else {
          char *hold = (char*)gs->hold;
          BlockAllocCat(hold, gs->gathered, (char*)p, ep - p);
          gs->hold = (PRUint8*)hold;
          gs->gather_head = gs->hold;
          gs->gathered += ep-p;
          return PR_SUCCESS;

By supplying negative request_size values at various points during parsing the 
gs->gathered variable can be made to be a negative number, which is not 
correctly accounted for in the above code. Since negative numbers aren't 
correctly handled the logic for buffer management becomes broken and allows 
users to do one of two things:

        1) Perform a negative memcpy on the heap, or
        2) Perform a small memcpy in an out of bounds region on the heap 
(right before the beginning of the gs->hold buffer, thus corrupting its' chunk 

Specifically, to trigger this second scenario, if we have an already allocated 
gs->hold buffer, and specify a negative integer as a request_size that is the 
same distance that gs->gather_head is away from gs->hold. The following code 
will move gs->gather_head back to the beginning of the buffer (to equal gs-

        if (gather_remaining <= 0) {
          gs->gathered -= request_size;
          q = gs->gather_head;
          gs->gather_head += request_size;
          gs->state = gs->post_gather_state;

After this, gs->gathered is set to the negative request size in the 
gif_wait_for_buffer_full state:

    case gif_wait_for_buffer_full:
      gs->gathered = gs->requested_buffer_fullness;
      GETN(1, gif_netscape_extension_block);

Hence, it is a small negative number. The next time that gif_delay/gif_gather 
state is entered, gather_remaining will be a small positive number, due to 
this line:

	gather_remaining = request_size - gs->gathered;

Request size is 1 the next time this state is entered, so:  (1 - (negative 
number)) will be a positive number. Since we made gs->gather_head equal to gs-
>hold in the previous step the following code branch will NOT be taken:

        /* Shift remaining data to the head of the buffer */
        if (gs->gathered && (gs->gather_head != gs->hold)) {
          memmove(gs->hold, gs->gather_head, gs->gathered);
          gs->gather_head = gs->hold;

and hence, a BlockAllocCat() will occur with the destination length (gs-
>gathered) negative. Looking at the internals of this function, it is easy to 
see that this will add the destination length from the newly allocated buffer 
(which is in effect a subtraction since this length is negative) and copy 
source data to that location.


Reproducible: Always

Steps to Reproduce:
We can provide a sample .GIF file if necessary. Please contact me if you need 
Actual Results:  
This vulnerability can lead to full compromise of a mozilla-based browser.

ISS X-Force plans to do a co-ordinated release and make public a security 
advisory about this issue when a Mozilla fix is ready to go. Please follow up 
with me personally about the timing of this notification. If you need any 
further details we'd also be glad to help.
dveditz, should this be confirmed?
Neel, if you could attach a sample .gif file to this bug so that we can test it,
that'd be great.

when we load that test file, would the current behavior be that firefox crashes?
confirming with the test case (you need to unzip it first): when trying to view
this file, it hangs firefox 1.0.1 and also hangs tbird (2005030906-1.0.1). upon
restarting, talkback starts up.

should this be moved over to the general security component since this affects
both tbird and firefox?
Ever confirmed: true
Keywords: crash, hang
Pav points out that bug 274391 removes the offending code, and has r+/sr+.

Dan, did Coverity's tools flag any of this code?

seems like there are two options to fix.  
1) don't overflow
2) remove support for the netscape specific extension.

should we do (1) for the immiediate security releases and then do (2) on the
trunk for release in 1.1 to get it some beta/test cycles?
-> Core
Component: Security → ImageLib
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0.2+
OS: All → Windows XP
Product: Firefox → Core
Hardware: All → PC
Whiteboard: [sg:fix]
Version: unspecified → 1.7 Branch
OS: Windows XP → All
Hardware: PC → All
add bclary to help assess the use of the Netscape-specific extension
Given the severity of this issue, it would be appreciated if once there is a
proper fix, we give the vendors a few days to get things under control.  The
recipients of vendor-sec would probably appreciate a heads up on this issue (and
it could be good to get an additional set of eyes on the code).
IMO, The best thing to do is to remove the Netscape Extension 2, as bug 274391
did.  My testing indicates that even a GIF with properly, in bounds, Netscape
Extension 2 does not work properly.   Furthermore, no other browsers support
this extension, nor do any popular GIF generating applications allow creating
this extension.  Removal will take removing 8 lines of code, and changing 2 lines.
According to bug 274391 we don't even use this extension, removing it.
Double-checked all the other ints read from the stream and they were all
unsigned and look to be handled properly.
Attachment #177085 - Flags: approval-aviary1.0.2?
Attachment #177085 - Flags: superreview?(tor)
Attachment #177085 - Flags: review?(pavlov)
Attachment #177085 - Flags: approval1.7.6?
Flags: blocking1.7.6?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.2?
Attachment #177085 - Flags: review?(pavlov) → review?(paper)
Attachment #177085 - Flags: review?(paper) → review+
Attachment #177085 - Flags: superreview?(tor) → superreview+
Comment on attachment 177085 [details] [diff] [review]
remove unused and buggy Netscape extension

a=caillon for the branches
Attachment #177085 - Flags: approval1.7.6?
Attachment #177085 - Flags: approval1.7.6+
Attachment #177085 - Flags: approval-aviary1.0.2?
Attachment #177085 - Flags: approval-aviary1.0.2+
Ready to land. The release is waiting on two more security fixes, will hold off
on landing until they're ready so as not to tip any bonsai-watching hackers
(unless drivers tell me otherwise).
Whiteboard: [sg:fix] → [sg:fix] ready to land
Chris tells me otherwise. Checked in to 1.7 and aviary-1.0.1 branches. Trunk
removed this code with bug 274391.
Closed: 19 years ago
Flags: blocking-aviary1.0.3+
Flags: blocking-aviary1.0.2?
Flags: blocking-aviary1.0.2+
Resolution: --- → FIXED
verified fixed with the following builds:

- mozilla 1.7.6 20050314 builds on Win XPsp2 and Mac OS X 10.3.8

- thunderbird 1.0.2 20050314 builds on Win XPsp2 and Mac OS X 10.3.8

- firefox 1.0.2 20050314 build on Mac OS X 10.3.8 (other platform bits not
presently available)

- firefox trunk 20050314 builds on Win XPsp2 and Mac OS X 10.3.8
Please use CAN-2005-0399 ( for those unfamiliar) for this issue.
Whiteboard: [sg:fix] ready to land → [sg:fix] CAN-2005-0399
Advisory published:
Group: security
Temporarily re-hiding bug at the request of the reporter to scrub his contact info.
Group: security
Whiteboard: [sg:fix] CAN-2005-0399 → [sg:fix] CAN-2005-0399 [temporary closure, see comment 19]
this has been ported to the AVIARY_1_0_20040515_BRANCH. This does not mean that
it was actually fixed in the 1.0 release of course.
Keywords: fixed-aviary1.0
Contact info scrubbed (thanks myk!), reopening bug.
Group: security
Whiteboard: [sg:fix] CAN-2005-0399 [temporary closure, see comment 19] → [sg:fix] CAN-2005-0399
Attachment #187759 - Flags: superreview?(dveditz)
Comment on attachment 187759 [details] [diff] [review]
Patch for 1.4 branch

Attachment #187759 - Flags: superreview?(dveditz) → superreview+
Flags: testcase+
Flags: in-testsuite+ → in-testsuite?
crash test landed
Flags: in-testsuite? → in-testsuite+
While updating my working directory with the newly landed crash test, I got a notification from Symantec Antivirus about modules/libpr0n/test/crashtests/285595-1.gif being affected with Bloodhound.Exploit.31 <>.  Reading the Symantec page I guess this is expected, but I thought I'd post a note here FWIW.
Ehsan, thanks for the tip. 

dveditz: the image is just a crash and not an exploit?
bc: no, this one was definitely an exploitable heap buffer overrun

The Symantec page linked above mentions the CVE number associated with this bug so I guess their scanner is specifically looking for this flaw, and therefore it's not surprising they flag a testcase for it.
The test image contains no malicious code, it is "just a crash" if that's what you meant.

Symantec is just being cautious which is why it comes up as their generic "bloodhound" type rather than a specific piece of malware. They found a GIF using a feature that 1) has no legitimate use and 2) was known to be exploitable in the past. Even without finding specific known malware that's suspicious, though in this instance (a testcase for the very thing they're looking for) it's expected.
C:\home\\mozilla-central>hg qpop -a && hg pull -u -r default && hg qpush -a
Patch queue now empty
pulling from
searching for changes
adding changesets
adding manifests
adding file changes
added 160 changesets with 664 changes to 544 files
abort: The system cannot find the file specified: C:\home\\mozilla-central\modules/libpr0n/test/crashtests/285595-1.gif

We need to do something about this. You've essentially broken my ability to do any work with mozilla. If this were CVS, I could do a partial checkout, but it's hg, so I can't.
(In reply to comment #29)
> We need to do something about this. You've essentially broken my ability to do
> any work with mozilla. If this were CVS, I could do a partial checkout, but
> it's hg, so I can't.

In order to solve this, I added the path to my source tree as an exception in Symantec so that it won't pick up the file in its auto-protect mode.  You may be able to do something similar with your antivirus.
backed out testcase. do not put it back without verifying you are not going to break people who work from windows based computers.
Flags: in-testsuite+ → in-testsuite-
Could a data: URL be used here perhaps?
ehsan: I don't control my antivirus, my employer does.

bill: maybe. But you need to check, because ideally antivirus software would detect that too :).

I think a ROT encoded string might work.
You need to log in before you can comment on or make changes to this bug.