gif with wrong block length crashes asan

VERIFIED FIXED in Firefox 17

Status

()

Core
ImageLib
VERIFIED FIXED
5 years ago
8 months ago

People

(Reporter: miaubiz, Assigned: Joe Drew (not getting mail))

Tracking

(Depends on: 1 bug, {crash, sec-low, testcase})

unspecified
mozilla18
x86
Linux
crash, sec-low, testcase
Points:
---
Bug Flags:
sec-bounty -
in-testsuite ?

Firefox Tracking Flags

(firefox16 wontfix, firefox17+ fixed, firefox18+ fixed, firefox-esr1017+ fixed)

Details

(Whiteboard: [asan][adv-track-main17-][adv-track-esr17-], crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
http://trac.webkit.org/export/117333/trunk/LayoutTests/fast/images/resources/wrong-block-length.gif

bad gif from webkit crashes browser

==1351== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7fffc3f52e08 at pc 0x7fffee09b910 bp 0x7fffffffada0 sp 0x7fffffffad98
READ of size 1 at 0x7fffc3f52e08 thread T0
    #0 0x7fffee09b910 in mozilla::image::nsGIFDecoder2::WriteInternal(char const*, unsigned int) /builds/slave/try-lnx64/build/image/decoders/nsGIFDecoder2.cpp:801
0x7fffc3f52e08 is located 0 bytes to the right of 7560-byte region [0x7fffc3f51080,0x7fffc3f52e08)
allocated by thread T0 here:
    #0 0x42aee1 in __interceptor_malloc ??:0
    #1 0x7fffee037dc7 in nsTArrayFallibleAllocator::Malloc(unsigned long) /builds/slave/try-lnx64/build/../../dist/include/nsTArray.h:41
==1351== ABORTING
(Reporter)

Comment 1

5 years ago
Created attachment 658863 [details]
the gif
(Reporter)

Comment 2

5 years ago
Created attachment 658864 [details]
asan log linux
Whiteboard: [asan]
(Reporter)

Comment 3

5 years ago
Created attachment 658865 [details]
minimized gif I made one day

==21925== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7fffcc3d3f9b at pc 0x7fffee09b910 bp 0x7fffffffa760 sp 0x7fffffffa758
READ of size 1 at 0x7fffcc3d3f9b thread T0
    #0 0x7fffee09b910 in mozilla::image::nsGIFDecoder2::WriteInternal(char const*, unsigned int) /builds/slave/try-lnx64/build/image/decoders/nsGIFDecoder2.cpp:801
0x7fffcc3d3f9b is located 2 bytes to the right of 793-byte region [0x7fffcc3d3c80,0x7fffcc3d3f99)
allocated by thread T0 here:
    #0 0x42aee1 in __interceptor_malloc ??:0
    #1 0x7fffee037dc7 in nsTArrayFallibleAllocator::Malloc(unsigned long) /builds/slave/try-lnx64/build/../../dist/include/nsTArray.h:41
==21925== ABORTING
(Reporter)

Comment 4

5 years ago
http://trac.webkit.org/changeset/117373
http://trac.webkit.org/changeset/117333

the original fix and the fix-of-the-fix
Reading too far doesn't sound too bad, but if we made the same mistake creating the buffer we're going to write into that could be a problem later.
Assignee: nobody → joe
Keywords: crash, testcase
(Assignee)

Comment 6

5 years ago
Created attachment 661918 [details] [diff] [review]
ensure we always check that there's enough data to satisfy the GIF spec

Yeah, we had exactly the same bug as WebKit, which isn't surprising given that their GIF decoder is based directly on ours. Hence, I just ported their fixes back to nsGIFDecoder2.cpp. :)
Attachment #661918 - Flags: review?(justin.lebar+bug)
Comment on attachment 661918 [details] [diff] [review]
ensure we always check that there's enough data to satisfy the GIF spec

r=adambarth is good enough for me.  :)
Attachment #661918 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 663185 [details] [diff] [review]
ensure we always check that there's enough data to satisfy the GIF spec

This failed to work on GIFs with comments because I actually misread the Webkit patch and thought the plaintext extension was equal to the comment extension. So I've added the plaintext extension to the switch statement, and made our decoder look more like WebKit's.
Attachment #661918 - Attachment is obsolete: true
Attachment #663185 - Flags: review?(justin.lebar+bug)
Comment on attachment 663185 [details] [diff] [review]
ensure we always check that there's enough data to satisfy the GIF spec

> +        case GIF_PLAIN_TEXT_LABEL:
> +          mGIFStruct.state = gif_skip_block;
> +          mGIFStruct.bytes_to_consume = NS_MAX(mGIFStruct.bytes_to_consume, 12u);
>
>         case GIF_COMMENT_LABEL:

You don't want a |break| in there?
(Assignee)

Comment 10

5 years ago
Er, yes I do.
(Assignee)

Comment 11

5 years ago
Created attachment 663529 [details] [diff] [review]
ensure we always check that there's enough data to satisfy the GIF spec
Attachment #663185 - Attachment is obsolete: true
Attachment #663185 - Flags: review?(justin.lebar+bug)
Attachment #663529 - Flags: review?(justin.lebar+bug)
Comment on attachment 663529 [details] [diff] [review]
ensure we always check that there's enough data to satisfy the GIF spec

I cursorally read the spec and convinced myself that the handling of GIF_COMMENT_LABEL differently than the others is probably right, so r=me, I guess.  :)
Attachment #663529 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e776f05c4f80
status-firefox18: --- → fixed
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/e776f05c4f80
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
This doesn't seem to crash non-ASAN builds. Is this ASAN only?
status-firefox18: fixed → ---
Target Milestone: mozilla18 → ---
Did you reset the milestone flags on purpose?
No, this was a session restore failure. My bad.
status-firefox18: --- → fixed
Target Milestone: --- → mozilla18
I've verified the fix now too using a mozilla-central ASAN build that I made this afternoon.
Status: RESOLVED → VERIFIED
status-firefox-esr10: --- → affected
status-firefox16: --- → wontfix
status-firefox17: --- → affected
tracking-firefox-esr10: --- → 17+
tracking-firefox17: --- → +
tracking-firefox18: --- → +

Updated

5 years ago
Crash Signature: [@ mozilla::image::nsGIFDecoder2::WriteInternal(char const*, unsigned int) ]
Please prepare a patch for the mozilla-esr10 branch the next opportunity you get. Thanks!
Need a beta approval nomination here as well please so we can fix on 17.0
(Assignee)

Comment 21

5 years ago
Comment on attachment 663529 [details] [diff] [review]
ensure we always check that there's enough data to satisfy the GIF spec

[Approval Request Comment]
User impact if declined: security bug
Fix Landed on Version: 18
Risk to taking this patch (and alternatives if risky): Could cause issues with specific GIFs.
String or UUID changes made by this patch: none
Attachment #663529 - Flags: approval-mozilla-esr10?
Attachment #663529 - Flags: approval-mozilla-beta?
Comment on attachment 663529 [details] [diff] [review]
ensure we always check that there's enough data to satisfy the GIF spec

Has been on 18 for a while, seems low risk enough to take on Beta/ESR, approving.
Attachment #663529 - Flags: approval-mozilla-esr10?
Attachment #663529 - Flags: approval-mozilla-esr10+
Attachment #663529 - Flags: approval-mozilla-beta?
Attachment #663529 - Flags: approval-mozilla-beta+
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/aa46c50b7747
https://hg.mozilla.org/releases/mozilla-esr10/rev/b2e284ac6486
status-firefox-esr10: affected → fixed
status-firefox17: affected → fixed
This does not appear particularly exploitable, at best if you avoid the crash you could get some extra bytes of data incorporated into your image.
Keywords: sec-low
Whiteboard: [asan] → [asan][adv-track-main17+][adv-track-esr17+]
Alias: CVE-2012-5834
Alias: CVE-2012-5834
Whiteboard: [asan][adv-track-main17+][adv-track-esr17+] → [asan][adv-track-main17-][adv-track-esr17-]
Group: core-security
Flags: sec-bounty-
Depends on: 819764
You need to log in before you can comment on or make changes to this bug.