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

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Neel Mehta]

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

dveditz, should this be confirmed?

Comment 2

[sairuh (rarely reading bugmail)]

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]

Attachment: proof of concept GIF file - crashes FireFox

Comment 4

[sairuh (rarely reading bugmail)]

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 5

[sairuh (rarely reading bugmail)]

just tested this on linux fc3.

tb for tbird:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=4255606%0D%0A

tb for ffox:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=4255567%0D%0A

Comment 6

[Brendan Eich [:brendan]]

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]

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]]

-> Core

Comment 9

[chris hofmann]

add bclary to help assess the use of the Netscape-specific extension

Comment 10

[Josh Bressers]

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

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]]

Attachment: 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)]

Comment on attachment 177085 [details] [diff] [review]
remove unused and buggy Netscape extension

a=caillon for the branches

Comment 14

[Daniel Veditz [:dveditz]]

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]]

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

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]

Please use CAN-2005-0399 (cve.mitre.org for those unfamiliar) for this issue.

Comment 18

[Daniel Veditz [:dveditz]]

Advisory published: http://www.mozilla.org/security/announce/mfsa2005-30.html

Comment 19

[Daniel Veditz [:dveditz]]

Temporarily re-hiding bug at the request of the reporter to scrub his contact info.

Comment 20

[Scott MacGregor]

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]]

Contact info scrubbed (thanks myk!), reopening bug.

Comment 22

[Leon Sha]

Attachment: Patch for 1.4 branch

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 187759 [details] [diff] [review]
Patch for 1.4 branch

r/sr=dveditz

Comment 24

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/b1207d1e0ea4

Comment 25

[(no longer active)]

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] (inactive)]

Ehsan, thanks for the tip. 

dveditz: the image is just a crash and not an exploit?

Comment 27

[Daniel Veditz [:dveditz]]

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]]

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]

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

[(no longer active)]

(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]

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]]

Could a data: URL be used here perhaps?

Comment 33

[timeless]

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.