878751 - (CVE-2013-1691) out-of-bounds read at mozilla::image::nsGIFDecoder2::WriteInternal

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Aki Helin]

Attachment: repro

ASan reports a heap buffer overflow (read) when the attached image is opened in current aurora or beta builds. This looks a lot like 856616, but the testcase there didn't work in my build and the log showed a less severe null dereference, so failing on the safe side and filing this separately.

==4517== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7fc412193bdc at pc 0x7fc42ef6a445 bp 0x7fc417
7f71f0 sp 0x7fc4177f71e8
READ of size 1 at 0x7fc412193bdc thread T13
    #0 0x7fc42ef6a444 in mozilla::image::nsGIFDecoder2::WriteInternal(char const*, unsigned int) /home/aki/src/m
ozilla-aurora/image/decoders/nsGIFDecoder2.cpp:830
    #1 0x7fc42eeee694 in mozilla::image::Decoder::Write(char const*, unsigned int) /home/aki/src/mozilla-aurora/
image/src/Decoder.cpp:106
    #2 0x7fc42eefec1c in mozilla::image::RasterImage::WriteToDecoder(char const*, unsigned int) /home/aki/src/mo
zilla-aurora/image/src/RasterImage.cpp:2691
0x7fc412193bdc is located 0 bytes to the right of 860-byte region [0x7fc412193880,0x7fc412193bdc)
allocated by thread T0 here:
    #0 0x43db50 in __interceptor_malloc ??:0
    #1 0x7fc42ea08cc2 in nsTArrayFallibleAllocator::Malloc(unsigned long) /home/aki/src/mozilla-aurora/../../dis
t/include/nsTArray.h:174
    #2 0xff887206571
Thread T13 created by T0 here:
    #0 0x439d84 in pthread_create ??:0
    #1 0x7fc4386fa176 in _PR_CreateThread /home/aki/src/mozilla-aurora/nsprpub/pr/src/pthreads/ptthread.c:444
    #2 0x7fc4386f9c77 in PR_CreateThread /home/aki/src/mozilla-aurora/nsprpub/pr/src/pthreads/ptthread.c:527

Comment 1

[Christoph Diehl [:posidron]]

I only can reproduce the testcase in a debug build on MacOS which then results in the same assertion failure as in bug 856616.

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/2d5a92daf4f1

Comment 2

[Aki Helin]

That's odd. Both my aurora- and beta-builds reproduce it on every attempt, as does https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1370254300/firefox-24.0a1.en-US.linux-x86_64-asan.tar.bz2. I have an up-to-date 64-bit Debian wheezy and open the picture with:
  $ tmp/firefox/firefox ff-bofr-writeinternal.gif
  [...]
  ==22024== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fdd64def79c at pc 0x7fdd877958e3 bp 0x7fdd6efdd1f0 sp 0x7fdd6efdd1e8

I don't have Windows or OS X to test with.

Comment 3

[Christoph Diehl [:posidron]]

I can reproduce it now. My repository was up-to-date but the non-debug build had not compiled since a few days. This must have been introduced in the last week.

Comment 4

[Daniel Veditz [:dveditz]]

Appears to have been introduced in the last week... can we bisect this more precisely?

Comment 5

[Aki Helin]

Seems to reproduce also using currently oldest tinderbox asan build (May 1, https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1367436570/firefox-23.0a1.en-US.linux-x86_64-asan.tar.bz2).

Comment 6

[Joe Drew (not getting mail)]

I can't reproduce this on Linux at all, whether using clang 3.2 or trunk svn revision 183248. The GIF simply gets rejected for errors.

Er, wait - is this aurora only?!

Comment 7

[Aki Helin]

This happens here at least in beta and aurora. Are you sure this doesn't happen using the tinderbox builds linked above?

Comment 8

[Joe Drew (not getting mail)]

Yeah, I can reproduce on Aurora. Sorry for my lack of reading comprehension.

Comment 9

[Joe Drew (not getting mail)]

Attachment: make sure we have at least 3 bytes in gif_consume_netscape_extension

We always consume at least 3 bytes in gif_consume_netscape_extension, so we need to make sure we have those 3 bytes.

I have no idea why this doesn't crash on m-c. Presumably there's some different allocation strategy or something.

This was also fixed on Chrome in March, but it was mixed into an unrelated changeset: https://chromium.googlesource.com/chromium/blink/+/5711af117fcb7da1412bcb9dbe832f084e97fb8b

Comment 10

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

Have we figured out if this affects 21?

Comment 11

[Aki Helin]

abillings, yes, crash report from current 21.0 at https://crash-stats.mozilla.com/report/index/a35b0d3a-00f0-4543-a4e5-79ef22130607

Comment 12

[Justin Lebar (not reading bugmail)]

Sorry if this is a dumb question, but it does not look like we always read 3 bytes in consume_netscape_extension.  It looks like we only read three bytes if q[0] & 7 == 1.  Otherwise we read one or two bytes.

Comment 13

[Joe Drew (not getting mail)]

True. I can adjust the comment if you like!

Comment 14

[Justin Lebar (not reading bugmail)]

What about the code?  Is it not problematic that we're GETN'ing more bytes than we consume?

Comment 15

[Joe Drew (not getting mail)]

Apparently not. All definitions of the NETSCAPE2.0 application block that I can find say that it's 3 bytes, e.g.

http://www.vurdalakov.net/misc/gif/netscape-looping-application-extension:

    Sub-block Data Size - Indicates the number of data bytes to follow. The size of the block does not account for the size byte itself. This field contains the fixed value 0x03 (3).

The netscapeExtension 2 (buffering) apparently "isn't used at all", and Chromium hasn't seen any reason to change their copy of our GIF decoder too. We don't actually read the 5 bytes the buffering extension would require, so there's no danger of another buffer overflow.

Worst case, we start rejecting some invalid GIFs that we used to accept, which isn't too bad a result.

Comment 16

[Justin Lebar (not reading bugmail)]

Okay, sgtm!

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 758781 [details] [diff] [review]
make sure we have at least 3 bytes in gif_consume_netscape_extension

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't know. This reads 1 byte beyond the allocated buffer, and then puts it in a 16-bit loop count. It's not clear whether this can be exploited at all.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yeah, pretty much.

Which older supported branches are affected by this flaw?

All, probably going back to Netscape!

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Super-easy, super-safe.

How likely is this patch to cause regressions; how much testing does it need?

Worst case, we stop supporting some gifs that we used to support. Not a huge deal, especially considering Chrome already doesn't support them.

Comment 18

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

Comment on attachment 758781 [details] [diff] [review]
make sure we have at least 3 bytes in gif_consume_netscape_extension

sec-approval+ on trunk

Comment 19

[Joe Drew (not getting mail)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a36f160d685e

Comment 20

[Ed Morley [:emorley]]

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

Comment 22

[Alex Keybl [:akeybl]]

Today's the last day to take this fix in FF22. Please nominate for uplift asap.

Comment 23

[Joe Drew (not getting mail)]

Comment on attachment 758781 [details] [diff] [review]
make sure we have at least 3 bytes in gif_consume_netscape_extension

[Approval Request Comment]
Bug caused by (feature/regressing bug #): been with us since forever
User impact if declined: potential for security bug
Testing completed (on m-c, etc.): on m-c, similar code in chrome for a few months
Risk to taking this patch (and alternatives if risky): not very risky, but we might stop supporting GIFs we used to support
String or IDL/UUID changes made by this patch: none

Comment 24

[Alex Keybl [:akeybl]]

Comment on attachment 758781 [details] [diff] [review]
make sure we have at least 3 bytes in gif_consume_netscape_extension

Super safe and _maybe_ sec-critical. Approving for landing.

Comment 25

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

Dan, can you evaluate the rating on this again? Joe and I are both skeptical that this is a sec-critical, necessarily.

Comment 26

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f7f709c5d7c4
https://hg.mozilla.org/releases/mozilla-beta/rev/30c85f01eb0f

Comment 27

[Mihaela Velimiroviciu (:mihaelav)]

I cannot reproduce (firefox doesn't quit as it does with the build in comment #2) with released Firefox 22 beta 3, nor with debug beta version from the beginning of June. I couldn't reproduce with latest beta, either.

Can someone please provide more info on how to verify on beta?

Comment 28

[Joe Drew (not getting mail)]

Mihaela, this won't reproduce without an ASAN build. Are you using ASAN builds?

Comment 29

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #28)
> Mihaela, this won't reproduce without an ASAN build. Are you using ASAN
> builds?

No, I used normal builds. Where can I find ASAN builds for beta?

Comment 30

[bhavana bajaj [:bajaj]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #26)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/f7f709c5d7c4
> https://hg.mozilla.org/releases/mozilla-beta/rev/30c85f01eb0f

Can you please help with an esr branch asap as esr17 is affected ?

Comment 31

[Joe Drew (not getting mail)]

(next time please needinfo? me!)

Comment 32

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20130617 Firefox/24.0, build ID: 20130617031112

Verified using the latest night ASAN build: there are no reports of heap buffer overflow (read) when the image attached in bug description is opened.

Comment 33

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/7fafa197c622

Comment 34

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/2b2e69aa0942 because esr17 doesn't have std::max

Comment 35

[Daniel Veditz [:dveditz]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #15)
> The netscapeExtension 2 (buffering) apparently "isn't used at all",

We removed support for the undocumented Netscape extension in 2005; see bug 274391 comment 14 and later.

(In reply to Al Billings [:abillings] from comment #25)
> Dan, can you evaluate the rating on this again? Joe and I are both
> skeptical that this is a sec-critical, necessarily.

I agree. ASAN is so over-alarmist about "heap-buffer-overflow" for simple out of bounds reads that when I see that label now I immediately assume it's a false alarm. That's dangerous, of course, because sometimes it certainly is an exploitable problem. Doesn't appear to be in this case, though.

Comment 36

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on ASan FF22 2013-03-06.
Confirmed fixed ASan builds FF22/23, 2013-06-14.
Confirmed fixed ASan 17esr 2013-06-18.

Comment 37

[Daniel Veditz [:dveditz]]

(In reply to Daniel Veditz [:dveditz] from comment #35)
> when I see that label now I immediately assume it's
> a false alarm. That's dangerous, of course, because sometimes it certainly
> is an exploitable problem. Doesn't appear to be in this case, though.

I am exaggerating (much as ASAN exaggerates?), please don't take me literally or follow my lead. An out of bounds read certainly can be exploitable and often is: it depends on what is being read and what the code does next. I'm only objecting to ASAN's use of the term "overflow" because in many cases that's not at all what's going on with an OBR.

Comment 38

[Joe Drew (not getting mail)]

Does this need to be kept private? It's a sec-low, after all.