Last Comment Bug 789046 - gif with wrong block length crashes asan
: gif with wrong block length crashes asan
Status: VERIFIED FIXED
[asan][adv-track-main17-][adv-track-e...
: crash, sec-low, testcase
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla18
Assigned To: Joe Drew (not getting mail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-06 06:57 PDT by miaubiz
Modified: 2013-03-18 13:08 PDT (History)
11 users (show)
dveditz: sec‑bounty-
ryanvm: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
17+
fixed


Attachments
the gif (7.38 KB, image/gif)
2012-09-06 06:59 PDT, miaubiz
no flags Details
asan log linux (1.97 KB, text/plain)
2012-09-06 06:59 PDT, miaubiz
no flags Details
minimized gif I made one day (785 bytes, image/gif)
2012-09-06 07:06 PDT, miaubiz
no flags Details
ensure we always check that there's enough data to satisfy the GIF spec (2.40 KB, patch)
2012-09-17 14:20 PDT, Joe Drew (not getting mail)
justin.lebar+bug: review+
Details | Diff | Review
ensure we always check that there's enough data to satisfy the GIF spec (2.58 KB, patch)
2012-09-20 15:27 PDT, Joe Drew (not getting mail)
no flags Details | Diff | Review
ensure we always check that there's enough data to satisfy the GIF spec (2.60 KB, patch)
2012-09-21 13:09 PDT, Joe Drew (not getting mail)
justin.lebar+bug: review+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Review

Description miaubiz 2012-09-06 06:57:29 PDT
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
Comment 1 miaubiz 2012-09-06 06:59:04 PDT
Created attachment 658863 [details]
the gif
Comment 2 miaubiz 2012-09-06 06:59:32 PDT
Created attachment 658864 [details]
asan log linux
Comment 3 miaubiz 2012-09-06 07:06:16 PDT
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
Comment 4 miaubiz 2012-09-06 07:07:17 PDT
http://trac.webkit.org/changeset/117373
http://trac.webkit.org/changeset/117333

the original fix and the fix-of-the-fix
Comment 5 Daniel Veditz [:dveditz] 2012-09-12 11:05:57 PDT
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.
Comment 6 Joe Drew (not getting mail) 2012-09-17 14:20:04 PDT
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. :)
Comment 7 Justin Lebar (not reading bugmail) 2012-09-17 18:22:47 PDT
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.  :)
Comment 8 Joe Drew (not getting mail) 2012-09-20 15:27:19 PDT
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.
Comment 9 Justin Lebar (not reading bugmail) 2012-09-21 11:42:40 PDT
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?
Comment 10 Joe Drew (not getting mail) 2012-09-21 13:07:58 PDT
Er, yes I do.
Comment 11 Joe Drew (not getting mail) 2012-09-21 13:09:30 PDT
Created attachment 663529 [details] [diff] [review]
ensure we always check that there's enough data to satisfy the GIF spec
Comment 12 Justin Lebar (not reading bugmail) 2012-09-21 14:17:29 PDT
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.  :)
Comment 13 Joe Drew (not getting mail) 2012-09-21 15:35:36 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e776f05c4f80
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-09-22 05:47:57 PDT
https://hg.mozilla.org/mozilla-central/rev/e776f05c4f80
Comment 15 Al Billings [:abillings] 2012-09-25 11:13:45 PDT
This doesn't seem to crash non-ASAN builds. Is this ASAN only?
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-09-25 11:32:39 PDT
Did you reset the milestone flags on purpose?
Comment 17 Al Billings [:abillings] 2012-09-25 17:51:38 PDT
No, this was a session restore failure. My bad.
Comment 18 Al Billings [:abillings] 2012-09-25 17:54:37 PDT
I've verified the fix now too using a mozilla-central ASAN build that I made this afternoon.
Comment 19 Alex Keybl [:akeybl] 2012-10-18 16:27:54 PDT
Please prepare a patch for the mozilla-esr10 branch the next opportunity you get. Thanks!
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 09:53:40 PDT
Need a beta approval nomination here as well please so we can fix on 17.0
Comment 21 Joe Drew (not getting mail) 2012-10-21 14:02:08 PDT
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
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-22 11:50:03 PDT
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.
Comment 24 Daniel Veditz [:dveditz] 2012-10-30 13:01:34 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.