Closed
Bug 373794
Opened 17 years ago
Closed 17 years ago
EXC_BAD_ACCESS crash with gif image
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dveditz, Assigned: crowderbt)
References
Details
(Keywords: crash, verified1.8.0.12, verified1.8.1.4)
Attachments
(3 files, 4 obsolete files)
105.90 KB,
image/gif
|
Details | |
22.70 KB,
text/plain; charset=UTF-8
|
Details | |
1.30 KB,
patch
|
tor
:
review+
pavlov
:
superreview+
dveditz
:
approval1.8.1.4+
dveditz
:
approval1.8.0.12+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
I do not crash in FF2.0.0.2 on WinXP SP2 (release or debug).
Comment 2•17 years ago
|
||
I crash using Firefox 2.0.0.2 on Mac OS X release. It seems to be fairly random though.
Assignee | ||
Comment 4•17 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•17 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•17 years ago
|
||
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•17 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•17 years ago
|
Attachment #258469 -
Flags: superreview? → superreview?(vladimir)
Comment 9•17 years ago
|
||
So remove s-s flag if this is a benign (crash-only, not read-to-jump-thru-vtbl) ABR impurity. /be
Assignee | ||
Updated•17 years ago
|
Group: security
Assignee | ||
Comment 10•17 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•17 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•17 years ago
|
||
To be really sure, why not replace == with >= in: if (stackp == stack + MAX_BITS || code > MAX_BITS)
Assignee | ||
Comment 13•17 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•17 years ago
|
Attachment #258469 -
Flags: superreview?(vladimir)
Attachment #258469 -
Flags: superreview+
Attachment #258469 -
Flags: review?(tor)
Attachment #258469 -
Flags: review?(pavlov)
Comment 14•17 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•17 years ago
|
||
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•17 years ago
|
||
Perhaps it is also possible for code to be negative, actually. Stuart?
Comment 17•17 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•17 years ago
|
||
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•17 years ago
|
||
Attachment #258726 -
Flags: superreview?(pavlov)
Attachment #258726 -
Flags: review?
Assignee | ||
Comment 20•17 years ago
|
||
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•17 years ago
|
Attachment #258726 -
Attachment is obsolete: true
Attachment #258726 -
Flags: superreview?(pavlov)
Attachment #258726 -
Flags: review?
Updated•17 years ago
|
Attachment #258727 -
Flags: superreview?(pavlov) → superreview+
Reporter | ||
Updated•17 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•17 years ago
|
||
Comment on attachment 258727 [details] [diff] [review] for real this time Grr, fixing the review flag.
Attachment #258727 -
Flags: review? → review?(tor)
Attachment #258727 -
Flags: review?(tor) → review+
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 22•17 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?
Updated•17 years ago
|
Flags: in-testsuite?
Reporter | ||
Comment 23•17 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•17 years ago
|
||
Moz_1.8: GIF2.cpp: 1.52.8.1 Moz_1.8.0: GIF2.cpp: 1.52.12.2
Updated•17 years ago
|
Whiteboard: [checkin needed - 1.8 branches]
Assignee | ||
Comment 26•17 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]
Comment 27•17 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•