If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

EXC_BAD_ACCESS crash with gif image

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: dveditz, Assigned: Brian Crowder)

Tracking

({crash, verified1.8.0.12, verified1.8.1.4})

1.8 Branch
crash, verified1.8.0.12, verified1.8.1.4
Points:
---
Bug Flags:
blocking1.8.1.4 +
blocking1.8.0.12 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 258436 [details]
test image received with initial submission

security@mozilla.org 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
(Reporter)

Comment 1

11 years ago
I do not crash in FF2.0.0.2 on WinXP SP2 (release or debug).
I crash using Firefox 2.0.0.2 on Mac OS X release. It seems to be fairly random though.
Created attachment 258440 [details]
valgrind warnings (trunk, Linux/i686)
(Assignee)

Comment 4

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

Comment 5

11 years ago
cc:ing Stuart since the crash I got was in libpr0n
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
OS: Windows XP → All
Hardware: PC → All
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.
(Assignee)

Comment 7

11 years ago
Created attachment 258469 [details] [diff] [review]
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.
Assignee: nobody → crowder
Status: NEW → ASSIGNED
Attachment #258469 - Flags: superreview?
Attachment #258469 - Flags: review?(pavlov)
(Assignee)

Comment 8

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

Updated

11 years ago
Attachment #258469 - Flags: superreview? → superreview?(vladimir)
So remove s-s flag if this is a benign (crash-only, not read-to-jump-thru-vtbl) ABR impurity.

/be
(Assignee)

Updated

11 years ago
Group: security
(Assignee)

Comment 10

11 years ago
Don't need to block for it, either.  Sorry for bugspam.
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
(Reporter)

Comment 11

11 years ago
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.
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?

Comment 12

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

Comment 13

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

Updated

11 years ago
Attachment #258469 - Flags: superreview?(vladimir)
Attachment #258469 - Flags: superreview+
Attachment #258469 - Flags: review?(tor)
Attachment #258469 - Flags: review?(pavlov)

Comment 14

11 years ago
Since code is used as an index into a MAX_BITS sized array, shouldn't the test be "code >= MAX_BITS"?
(Assignee)

Comment 15

11 years ago
Created attachment 258587 [details] [diff] [review]
off-by-one fix, thanks to T Rowley

Woops.  Yeah, sure should be.  Thanks for the catch.
Attachment #258469 - Attachment is obsolete: true
Attachment #258587 - Flags: superreview?(pavlov)
Attachment #258587 - Flags: review?(tor)
Attachment #258469 - Flags: review?(tor)
(Assignee)

Comment 16

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

Comment 17

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

Comment 18

11 years ago
Created attachment 258703 [details] [diff] [review]
one more off-by-one

Instead of > MAX - 1, how about >= MAX to match the other code?
Attachment #258587 - Attachment is obsolete: true
Attachment #258703 - Flags: superreview?(pavlov)
Attachment #258703 - Flags: review?(tor)
Attachment #258587 - Flags: superreview?(pavlov)
Attachment #258587 - Flags: review?(tor)
(Assignee)

Comment 19

11 years ago
Created attachment 258726 [details] [diff] [review]
for real this time
Attachment #258726 - Flags: superreview?(pavlov)
Attachment #258726 - Flags: review?
(Assignee)

Comment 20

11 years ago
Created attachment 258727 [details] [diff] [review]
for real this time
Attachment #258703 - Attachment is obsolete: true
Attachment #258727 - Flags: superreview?(pavlov)
Attachment #258727 - Flags: review?
Attachment #258703 - Flags: superreview?(pavlov)
Attachment #258703 - Flags: review?(tor)
(Assignee)

Updated

11 years ago
Attachment #258726 - Attachment is obsolete: true
Attachment #258726 - Flags: superreview?(pavlov)
Attachment #258726 - Flags: review?

Updated

11 years ago
Attachment #258727 - Flags: superreview?(pavlov) → superreview+
(Reporter)

Updated

11 years ago
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
(Assignee)

Comment 21

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

Grr, fixing the review flag.
Attachment #258727 - Flags: review? → review?(tor)

Updated

11 years ago
Attachment #258727 - Flags: review?(tor) → review+
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 22

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

Trunk patch applies on both branches with hunk offsets.
Attachment #258727 - Flags: approval1.8.1.4?
Attachment #258727 - Flags: approval1.8.0.12?
Flags: in-testsuite?
(Reporter)

Comment 23

11 years ago
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
Attachment #258727 - Flags: approval1.8.1.4?
Attachment #258727 - Flags: approval1.8.1.4+
Attachment #258727 - Flags: approval1.8.0.12?
Attachment #258727 - Flags: approval1.8.0.12+
(Assignee)

Comment 24

11 years ago
Moz_1.8:   GIF2.cpp: 1.52.8.1
Moz_1.8.0: GIF2.cpp: 1.52.12.2
Duplicate of this bug: 377293
Whiteboard: [checkin needed - 1.8 branches]
(Assignee)

Comment 26

11 years ago
I did land these on the branches, just didn't put the "fixed" keywords in, apologies.
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: [checkin needed - 1.8 branches]
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
Keywords: fixed1.8.0.12, fixed1.8.1.4 → verified1.8.0.12, verified1.8.1.4
(Reporter)

Updated

11 years ago
Keywords: crash
You need to log in before you can comment on or make changes to this bug.