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

VERIFIED FIXED

Status

()

Core
ImageLib
--
critical
VERIFIED FIXED
13 years ago
8 years ago

People

(Reporter: Neel Mehta, Assigned: dveditz)

Tracking

(6 keywords)

1.7 Branch
crash, fixed-aviary1.0, fixed-aviary1.0.2, fixed1.7.6, hang, verified1.7.6
Points:
---
Bug Flags:
blocking1.7.6 +
blocking-aviary1.0.2 +
blocking-aviary1.5 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments)

577 bytes, application/octet-stream
Details
4.13 KB, patch
Stuart Parmenter
: review+
Christopher Aillon (sabbatical, not receiving bugmail)
: approval-aviary1.0.2+
Christopher Aillon (sabbatical, not receiving bugmail)
: approval1.7.6+
Details | Diff | Splinter Review
4.67 KB, patch
dveditz
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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.
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?
(Reporter)

Comment 3

13 years ago
Created attachment 177046 [details]
proof of concept GIF file - crashes FireFox
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, hang
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
OS: Windows XP → All
Hardware: PC → All
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

13 years ago
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?
(Assignee)

Comment 8

13 years ago
-> 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
(Assignee)

Updated

13 years ago
OS: Windows XP → All
Hardware: PC → All

Comment 9

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

Comment 10

13 years ago
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

13 years ago
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.
(Assignee)

Comment 12

13 years ago
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.
Attachment #177085 - Flags: approval-aviary1.0.2?
(Assignee)

Updated

13 years ago
Attachment #177085 - Flags: superreview?(tor)
Attachment #177085 - Flags: review?(pavlov)
Attachment #177085 - Flags: approval1.7.6?
Flags: blocking1.7.6?
(Assignee)

Updated

13 years ago
Flags: blocking1.7.6?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.2?
(Assignee)

Updated

13 years ago
Attachment #177085 - Flags: review?(pavlov) → review?(paper)

Updated

13 years ago
Attachment #177085 - Flags: review?(paper) → review+

Updated

13 years ago
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+
(Assignee)

Comment 14

13 years ago
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
(Assignee)

Comment 15

13 years ago
Chris tells me otherwise. Checked in to 1.7 and aviary-1.0.1 branches. Trunk
removed this code with bug 274391.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Flags: blocking-aviary1.0.3+
Flags: blocking-aviary1.0.2?
Flags: blocking-aviary1.0.2+
Keywords: fixed-aviary1.0.2, fixed1.7.6
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
Status: RESOLVED → VERIFIED

Comment 17

13 years ago
Please use CAN-2005-0399 (cve.mitre.org for those unfamiliar) for this issue.
Keywords: verified1.7.6
(Assignee)

Updated

13 years ago
Whiteboard: [sg:fix] ready to land → [sg:fix] CAN-2005-0399
(Assignee)

Comment 18

13 years ago
Advisory published: http://www.mozilla.org/security/announce/mfsa2005-30.html
Group: security
(Assignee)

Comment 19

13 years ago
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]

Comment 20

13 years ago
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
(Assignee)

Comment 21

13 years ago
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

Comment 22

12 years ago
Created attachment 187759 [details] [diff] [review]
Patch for 1.4 branch
Attachment #187759 - Flags: superreview?(dveditz)
(Assignee)

Comment 23

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

r/sr=dveditz
Attachment #187759 - Flags: superreview?(dveditz) → superreview+

Updated

12 years ago
Flags: testcase+

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?

Comment 24

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/b1207d1e0ea4
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 <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

8 years ago
Ehsan, thanks for the tip. 

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

Comment 27

8 years ago
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.
(Assignee)

Comment 28

8 years ago
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

8 years ago
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.
(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

8 years ago
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?

Comment 33

8 years ago
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.