789046 - gif with wrong block length crashes asan

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[miaubiz]

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]

Attachment: the gif

Comment 2

[miaubiz]

Attachment: asan log linux

Comment 3

[miaubiz]

Attachment: 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]

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]]

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)]

Attachment: 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)]

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)]

Attachment: 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)]

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)]

Er, yes I do.

Comment 11

[Joe Drew (not getting mail)]

Attachment: ensure we always check that there's enough data to satisfy the GIF spec

Comment 12

[Justin Lebar (not reading bugmail)]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e776f05c4f80

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e776f05c4f80

Comment 15

[Al Billings [:abillings - ex-MoCo]]

This doesn't seem to crash non-ASAN builds. Is this ASAN only?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Did you reset the milestone flags on purpose?

Comment 17

[Al Billings [:abillings - ex-MoCo]]

No, this was a session restore failure. My bad.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

I've verified the fix now too using a mozilla-central ASAN build that I made this afternoon.

Comment 19

[Alex Keybl [:akeybl]]

Please prepare a patch for the mozilla-esr10 branch the next opportunity you get. Thanks!

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Need a beta approval nomination here as well please so we can fix on 17.0

Comment 21

[Joe Drew (not getting mail)]

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]

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 23

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-beta/rev/aa46c50b7747
https://hg.mozilla.org/releases/mozilla-esr10/rev/b2e284ac6486

Comment 24

[Daniel Veditz [:dveditz]]

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.