373794 - EXC_BAD_ACCESS crash with gif image

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: test image received with initial submission

[e-mail] received the following submission:
- - - - - - - - - - - - - - - - - - -
Mozilla Security Team,

The attached fuzzed GIF image consistently crashes Firefox 2.0.0.2 and today's
CVS checkout from 2.0.0.3pre on OS X/PPC, and occasionally crashes Firefox
2.0.0.2 on Windows XP and Windows 2000.  According to gdb, the crash occurs at
the following location:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x1aeaaa11
do_lzw (gs=0x1aea2004, q=0x2365938 "") at GIF2.cpp:347
347             *stackp++ = suffix[code];
(gdb) print code
$1 = 26215

I don't have the time at the moment to determine if this is exploitable
for anything besides a denial-of-service condition

Let me know if you have any questions.

thanks,
Clinton Ruoho

Comment 1

[Daniel Veditz [:dveditz]]

I do not crash in FF2.0.0.2 on WinXP SP2 (release or debug).

Comment 2

[Samuel Sidler (old account; do not CC)]

I crash using Firefox 2.0.0.2 on Mac OS X release. It seems to be fairly random though.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: valgrind warnings (trunk, Linux/i686)

Comment 4

[Brian Crowder]

I got a pretty good crash from this on my first attempt, and I have written a small hack/patch that might fix it.  I will post after I have tested some (needs a rebuild).  This is definitely (at least in the situation I got this crash) rightly filed as security sensitive.

Comment 5

[Brian Crowder]

cc:ing Stuart since the crash I got was in libpr0n

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looked to me like it's just a crash ("denial of service"), since it's a series of out-of-bounds reads, not writes.

Comment 7

[Brian Crowder]

Attachment: fix potential bounds error

dbaron's right, this probably isn't a very serious bug, security-wise (if at all).  Sorry for bad initial guess.  Looks like we read past the end of the "prefix" array due to a missing bounds check.

Comment 8

[Brian Crowder]

Comment on attachment 258469 [details] [diff] [review]
fix potential bounds error

This is a trunk patch, btw.  I will port to branches if it is accepted.

Comment 9

[Brendan Eich [:brendan]]

So remove s-s flag if this is a benign (crash-only, not read-to-jump-thru-vtbl) ABR impurity.

/be

Comment 10

[Brian Crowder]

Don't need to block for it, either.  Sorry for bugspam.

Comment 11

[Daniel Veditz [:dveditz]]

If the fix is this simple and safe it'd be good to land on the branches -- image fuzzing is in vogue and this will prevent future false alarms and scrambles to recreate this analysis for each new image.

Comment 12

[Alfred Kayser]

To be really sure, why not replace == with >= in:
if (stackp == stack + MAX_BITS || code > MAX_BITS)

Comment 13

[Brian Crowder]

Doesn't seem necessary since stackp only increments one-at-a-time, it doesn't jump randomly as |code| may.  I was aiming for as light a touch on the logic here as possible.  If we're worried about someone altering this loop in the future to change the way stackp strides, then that would probably be a good change, sure.

Comment 14

[tor]

Since code is used as an index into a MAX_BITS sized array, shouldn't the test be "code >= MAX_BITS"?

Comment 15

[Brian Crowder]

Attachment: off-by-one fix, thanks to T Rowley

Woops.  Yeah, sure should be.  Thanks for the catch.

Comment 16

[Brian Crowder]

Perhaps it is also possible for code to be negative, actually.  Stuart?

Comment 17

[tor]

Reading through the lzw logic, I think the logic prevents it from being negative except for a special case of -1, which is handled.

While looking through the code and the GIF spec, I think there's another minor problem in GIF2.cpp.  In the LZW appendix, it states "The output codes are of variable length, starting at <code size>+1 bits per code, up to 12 bits per code. This defines a maximum code value of 4095 (0xFFF)."  GIF2.cpp contains this snippet:

      /* Initialize LZW parser/decoder */
      gs->datasize = *q;
      if (gs->datasize > MAX_LZW_BITS) {
        gs->state = gif_error;
        break;
      }
      ...
      gs->codesize = gs->datasize + 1;

That should probably test against MAX_LZW_BITS-1;

Comment 18

[Brian Crowder]

Attachment: one more off-by-one

Instead of > MAX - 1, how about >= MAX to match the other code?

Comment 19

[Brian Crowder]

Attachment: for real this time

Comment 20

[Brian Crowder]

Attachment: for real this time

Comment 21

[Brian Crowder]

Comment on attachment 258727 [details] [diff] [review]
for real this time

Grr, fixing the review flag.

Comment 22

[Brian Crowder]

Comment on attachment 258727 [details] [diff] [review]
for real this time

Trunk patch applies on both branches with hunk offsets.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 258727 [details] [diff] [review]
for real this time

approved for 1.8.0.12/1.8.1.4, a=dveditz for release-drivers

Comment 24

[Brian Crowder]

Moz_1.8:   GIF2.cpp: 1.52.8.1
Moz_1.8.0: GIF2.cpp: 1.52.12.2

Comment 26

[Brian Crowder]

I did land these on the branches, just didn't put the "fixed" keywords in, apologies.

Comment 27

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.4 using the test image from comment #0 on vista, xp x64 Sp2 and linux fedora Fc 6 
Builds:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4pre) Gecko/2007042803 BonEcho/2.0.0.4pre

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.4pre) Gecko/2007042805 BonEcho/2.0.0.4pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.4pre) Gecko/2007042805 BonEcho/2.0.0.4pre

and verified fixed 1.8.0.12 using the test image on 
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.12pre) Gecko/20070428 Firefox/1.5.0.12pre

- no crash on test image -> adding verified keyword