Last Comment Bug 285595 - A buffer overflow vulnerability in GIF processing that can lead to remote compromise.
: A buffer overflow vulnerability in GIF processing that can lead to remote com...
Status: VERIFIED FIXED
[sg:fix] CAN-2005-0399
: crash, fixed-aviary1.0, fixed-aviary1.0.2, fixed1.7.6, hang, verified1.7.6
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: 1.7 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-03-10 07:56 PST by Neel Mehta
Modified: 2009-08-26 23:01 PDT (History)
25 users (show)
dveditz: blocking1.7.6+
dveditz: blocking‑aviary1.0.2+
dveditz: blocking‑aviary1.5+
timeless: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proof of concept GIF file - crashes FireFox (577 bytes, application/octet-stream)
2005-03-10 12:15 PST, Neel Mehta
no flags Details
remove unused and buggy Netscape extension (4.13 KB, patch)
2005-03-10 18:36 PST, Daniel Veditz [:dveditz]
pavlov: review+
tor: superreview+
caillon: approval‑aviary1.0.2+
caillon: approval1.7.6+
Details | Diff | Splinter Review
Patch for 1.4 branch (4.67 KB, patch)
2005-06-29 23:30 PDT, Leon Sha
dveditz: superreview+
Details | Diff | Splinter Review

Description Neel Mehta 2005-03-10 07:56:37 PST
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:

mozilla/modules/libpr0n/decoders/gif/GIF2.cpp

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


    /* 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;
          break;
        }

        /* 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 
header).

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

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

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);
      break;

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 
this.
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.
Comment 1 sairuh (rarely reading bugmail) 2005-03-10 11:54:55 PST
dveditz, should this be confirmed?
Comment 2 sairuh (rarely reading bugmail) 2005-03-10 11:56:32 PST
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?
Comment 3 Neel Mehta 2005-03-10 12:15:14 PST
Created attachment 177046 [details]
proof of concept GIF file - crashes FireFox
Comment 4 sairuh (rarely reading bugmail) 2005-03-10 12:52:50 PST
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?
Comment 6 Brendan Eich [:brendan] 2005-03-10 13:56:35 PST
Pav points out that bug 274391 removes the offending code, and has r+/sr+.

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

/be
Comment 7 chris hofmann 2005-03-10 14:05:40 PST
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?
Comment 8 Daniel Veditz [:dveditz] 2005-03-10 14:32:52 PST
-> Core
Comment 9 chris hofmann 2005-03-10 16:21:18 PST
add bclary to help assess the use of the Netscape-specific extension
Comment 10 Josh Bressers 2005-03-10 16:56:17 PST
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).
Comment 11 ArronM (:paper) 2005-03-10 18:27:22 PST
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.
Comment 12 Daniel Veditz [:dveditz] 2005-03-10 18:36:50 PST
Created attachment 177085 [details] [diff] [review]
remove unused and buggy Netscape extension

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.
Comment 13 Christopher Aillon (sabbatical, not receiving bugmail) 2005-03-11 14:59:37 PST
Comment on attachment 177085 [details] [diff] [review]
remove unused and buggy Netscape extension

a=caillon for the branches
Comment 14 Daniel Veditz [:dveditz] 2005-03-11 17:39:17 PST
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).
Comment 15 Daniel Veditz [:dveditz] 2005-03-11 18:32:13 PST
Chris tells me otherwise. Checked in to 1.7 and aviary-1.0.1 branches. Trunk
removed this code with bug 274391.
Comment 16 sairuh (rarely reading bugmail) 2005-03-14 11:06:45 PST
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
Comment 17 Josh Bressers 2005-03-15 16:19:54 PST
Please use CAN-2005-0399 (cve.mitre.org for those unfamiliar) for this issue.
Comment 18 Daniel Veditz [:dveditz] 2005-03-23 13:40:20 PST
Advisory published: http://www.mozilla.org/security/announce/mfsa2005-30.html
Comment 19 Daniel Veditz [:dveditz] 2005-03-24 12:34:46 PST
Temporarily re-hiding bug at the request of the reporter to scrub his contact info.
Comment 20 Scott MacGregor 2005-03-25 10:29:39 PST
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.
Comment 21 Daniel Veditz [:dveditz] 2005-03-28 12:35:13 PST
Contact info scrubbed (thanks myk!), reopening bug.
Comment 22 Leon Sha 2005-06-29 23:30:21 PDT
Created attachment 187759 [details] [diff] [review]
Patch for 1.4 branch
Comment 23 Daniel Veditz [:dveditz] 2005-08-11 22:27:27 PDT
Comment on attachment 187759 [details] [diff] [review]
Patch for 1.4 branch

r/sr=dveditz
Comment 24 Bob Clary [:bc:] 2009-04-24 11:29:02 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/b1207d1e0ea4
Comment 25 :Ehsan Akhgari 2009-04-24 11:40:18 PDT
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 <http://securityresponse.symantec.com/security_response/writeup.jsp?docid=2005-032911-3744-99>.  Reading the Symantec page I guess this is expected, but I thought I'd post a note here FWIW.
Comment 26 Bob Clary [:bc:] 2009-04-24 11:47:12 PDT
Ehsan, thanks for the tip. 

dveditz: the image is just a crash and not an exploit?
Comment 27 Daniel Veditz [:dveditz] 2009-04-24 13:04:57 PDT
bc: no, this one was definitely an exploitable heap buffer overrun
http://www.mozilla.org/security/announce/2005/mfsa2005-30.html

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.
Comment 28 Daniel Veditz [:dveditz] 2009-04-24 13:17:49 PDT
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.
Comment 29 timeless 2009-04-25 16:23:49 PDT
C:\home\mozilla.org\mozilla-central>hg qpop -a && hg pull -u -r default && hg qpush -a
Patch queue now empty
pulling from https://hg.mozilla.org/mozilla-central
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.org\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.
Comment 30 :Ehsan Akhgari 2009-04-25 17:27:57 PDT
(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.
Comment 31 timeless 2009-04-25 18:00:27 PDT
backed out testcase. do not put it back without verifying you are not going to break people who work from windows based computers.
Comment 32 Bill Gianopoulos [:WG9s] 2009-04-28 05:20:50 PDT
Could a data: URL be used here perhaps?
Comment 33 timeless 2009-05-03 08:20:11 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.