801366 - (CVE-2013-0772) out-of-bounds-read in mozilla::image::RasterImage::DrawFrameTo

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Atte Kettunen]

Attachment: Repro-file

The ASAN-report from this bug looks almost identical to https://bugzilla.mozilla.org/show_bug.cgi?id=758200 which should be fixed. I also tried the repro-files from the earlier bug but those don't cause crash anymore.

Tested with build from https://people.mozilla.com/~choller/firefox/asan/20121013-mozilla-central-linux64-debug-0be7bfea4744+asan.html

To reproduce open the attached gif-image with ASAN built Firefox didn't affect Firefox 16.0.1 official-build. 

ASAN-report: 

==11528== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f43552f8bbc at pc 0x7f4375acb440 bp 0x7fffc641a070 sp 0x7fffc641a068
READ of size 4 at 0x7f43552f8bbc thread T0
    #0 0x7f4375acb43f in mozilla::image::RasterImage::DrawFrameTo(imgFrame*, imgFrame*, nsIntRect&) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:2173
    #1 0x7f4375ac0fa7 in mozilla::image::RasterImage::DoComposite(nsIntRect*, imgFrame*, imgFrame*, int) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:1979
    #2 0x7f4375abfcc4 in mozilla::image::RasterImage::AdvanceFrame(mozilla::TimeStamp, nsIntRect*) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:442
    #3 0x7f4375ac166b in mozilla::image::RasterImage::RequestRefresh(mozilla::TimeStamp const&) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:485
    #4 0x7f4375ce774c in nsRefreshDriver::ImageRequestEnumerator(nsISupportsHashKey*, void*) /builds/slave/try-lnx64-dbg/build/layout/base/nsRefreshDriver.cpp:469
    #5 0x7f4377f204a3 in PL_DHashTableEnumerate /builds/slave/try-lnx64-dbg/build/obj-firefox/xpcom/build/pldhash.cpp:716
    #6 0x7f4375ce756c in nsTHashtable<nsISupportsHashKey>::EnumerateEntries(PLDHashOperator (*)(nsISupportsHashKey*, void*), void*) /builds/slave/try-lnx64-dbg/build/../../dist/include/nsTHashtable.h:237
    #7 0x7f4375ce622f in nsRefreshDriver::Notify(nsITimer*) /builds/slave/try-lnx64-dbg/build/layout/base/nsRefreshDriver.cpp:417
    #8 0x7f4377fd97e7 in nsTimerImpl::Fire() /builds/slave/try-lnx64-dbg/build/xpcom/threads/nsTimerImpl.cpp:475
    #9 0x7f4377fda1ae in nsTimerEvent::Run() /builds/slave/try-lnx64-dbg/build/xpcom/threads/nsTimerImpl.cpp:555
    #10 0x7f4377fcd22b in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/try-lnx64-dbg/build/xpcom/threads/nsThread.cpp:612
    #11 0x7f4377f1ba12 in NS_ProcessNextEvent_P(nsIThread*, bool) /builds/slave/try-lnx64-dbg/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:220
    #12 0x7f4377b6442b in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/try-lnx64-dbg/build/ipc/glue/MessagePump.cpp:82
    #13 0x7f437805a851 in MessageLoop::RunInternal() /builds/slave/try-lnx64-dbg/build/ipc/chromium/src/base/message_loop.cc:215
    #14 0x7f437805a74e in MessageLoop::Run() /builds/slave/try-lnx64-dbg/build/ipc/chromium/src/base/message_loop.cc:182
    #15 0x7f43778fcd31 in nsBaseAppShell::Run() /builds/slave/try-lnx64-dbg/build/widget/xpwidgets/nsBaseAppShell.cpp:163
    #16 0x7f43774a3df0 in nsAppStartup::Run() /builds/slave/try-lnx64-dbg/build/toolkit/components/startup/nsAppStartup.cpp:290
    #17 0x7f4375544084 in XREMain::XRE_mainRun() /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3792
    #18 0x7f437554516d in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3858
    #19 0x7f4375545b21 in XRE_main /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3933
    #20 0x409943 in do_main(int, char**) /builds/slave/try-lnx64-dbg/build/browser/app/nsBrowserApp.cpp:174
    #21 0x409127 in main /builds/slave/try-lnx64-dbg/build/browser/app/nsBrowserApp.cpp:279
    #22 0x7f4380c0376c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
0x7f43552f8bbc is located 27 bytes to the right of 33-byte region [0x7f43552f8b80,0x7f43552f8ba1)
allocated by thread T0 here:
    #0 0x4354c0 in __interceptor_malloc ??:0
    #1 0x7f4375ae3adb in imgFrame::Init(int, int, int, int, gfxASurface::gfxImageFormat, unsigned char) /builds/slave/try-lnx64-dbg/build/image/src/imgFrame.cpp:169
    #2 0x7f4375ac6afe in mozilla::image::RasterImage::InternalAddFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char, unsigned char**, unsigned int*, unsigned int**, unsigned int*) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:1110
    #3 0x7f4375ac73dd in mozilla::image::RasterImage::EnsureFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char, unsigned char**, unsigned int*, unsigned int**, unsigned int*) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:1232
    #4 0x7f4375b2381f in mozilla::image::nsGIFDecoder2::BeginImageFrame(unsigned short) /builds/slave/try-lnx64-dbg/build/image/decoders/nsGIFDecoder2.cpp:185
    #5 0x7f4375b27c1f in mozilla::image::nsGIFDecoder2::WriteInternal(char const*, unsigned int) /builds/slave/try-lnx64-dbg/build/image/decoders/nsGIFDecoder2.cpp:956
    #6 0x7f4375ac902d in mozilla::image::RasterImage::WriteToDecoder(char const*, unsigned int) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:2478
    #7 0x7f4375acfdc6 in mozilla::image::RasterImage::DecodeSomeData(unsigned int) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:3086
    #8 0x7f4375ad06f2 in mozilla::image::RasterImage::DecodeWorker::DecodeSomeOfImage(mozilla::image::RasterImage*, mozilla::image::RasterImage::DecodeWorker::DecodeType) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:3398
    #9 0x7f4375ac9446 in mozilla::image::RasterImage::SourceDataComplete() /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:1672
    #10 0x7f4375b05e96 in imgRequest::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) /builds/slave/try-lnx64-dbg/build/image/src/imgRequest.cpp:582
    #11 0x7f437661a231 in mozilla::dom::MediaDocumentStreamListener::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) /builds/slave/try-lnx64-dbg/build/content/html/document/src/MediaDocument.cpp:67
    #12 0x7f4377378616 in nsDocumentOpenInfo::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) /builds/slave/try-lnx64-dbg/build/uriloader/base/nsURILoader.cpp:293
    #13 0x7f4375593ce1 in nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) /builds/slave/try-lnx64-dbg/build/netwerk/base/src/nsBaseChannel.cpp:757
    #14 0x7f4375593dfc in non-virtual thunk to nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) /builds/slave/try-lnx64-dbg/build/media/libvpx/vp8/encoder/x86/quantize_mmx.asm:0
    #15 0x7f43755ba214 in nsInputStreamPump::OnStateStop() /builds/slave/try-lnx64-dbg/build/netwerk/base/src/nsInputStreamPump.cpp:552
    #16 0x7f43755b8ea7 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /builds/slave/try-lnx64-dbg/build/netwerk/base/src/nsInputStreamPump.cpp:374
    #17 0x7f4377f9802d in nsInputStreamReadyEvent::Run() /builds/slave/try-lnx64-dbg/build/xpcom/io/nsStreamUtils.cpp:82
    #18 0x7f4377fcd22b in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/try-lnx64-dbg/build/xpcom/threads/nsThread.cpp:612
    #19 0x7f4377f1ba12 in NS_ProcessNextEvent_P(nsIThread*, bool) /builds/slave/try-lnx64-dbg/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:220
    #20 0x7f4377b6442b in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/try-lnx64-dbg/build/ipc/glue/MessagePump.cpp:82
    #21 0x7f437805a851 in MessageLoop::RunInternal() /builds/slave/try-lnx64-dbg/build/ipc/chromium/src/base/message_loop.cc:215
    #22 0x7f437805a74e in MessageLoop::Run() /builds/slave/try-lnx64-dbg/build/ipc/chromium/src/base/message_loop.cc:182
    #23 0x7f43778fcd31 in nsBaseAppShell::Run() /builds/slave/try-lnx64-dbg/build/widget/xpwidgets/nsBaseAppShell.cpp:163
    #24 0x7f43774a3df0 in nsAppStartup::Run() /builds/slave/try-lnx64-dbg/build/toolkit/components/startup/nsAppStartup.cpp:290
Shadow byte and word:
  0x1fe86aa5f177: fb
  0x1fe86aa5f170: 00 00 00 00 01 fb fb fb
More shadow bytes:
  0x1fe86aa5f150: 00 00 00 00 00 00 00 00
  0x1fe86aa5f158: 00 00 00 00 00 00 fb fb
  0x1fe86aa5f160: fa fa fa fa fa fa fa fa
  0x1fe86aa5f168: fa fa fa fa fa fa fa fa
=>0x1fe86aa5f170: 00 00 00 00 01 fb fb fb
  0x1fe86aa5f178: fb fb fb fb fb fb fb fb
  0x1fe86aa5f180: fa fa fa fa fa fa fa fa
  0x1fe86aa5f188: fa fa fa fa fa fa fa fa
  0x1fe86aa5f190: 00 00 00 00 00 00 00 00
Stats: 225M malloced (243M for red zones) by 348346 calls
Stats: 33M realloced by 21083 calls
Stats: 197M freed by 224732 calls
Stats: 63M really freed by 132000 calls
Stats: 448M (114771 full pages) mmaped in 112 calls
  mmaps   by size class: 8:245745; 9:32764; 10:16380; 11:12282; 12:3072; 13:1536; 14:1280; 15:256; 16:512; 17:1280; 18:192; 19:40; 20:20;
  mallocs by size class: 8:274234; 9:34150; 10:18865; 11:12880; 12:2580; 13:1750; 14:1449; 15:269; 16:634; 17:1280; 18:198; 19:37; 20:20;
  frees   by size class: 8:170193; 9:23848; 10:14205; 11:9794; 12:1575; 13:1564; 14:1294; 15:223; 16:565; 17:1267; 18:150; 19:36; 20:18;
  rfrees  by size class: 8:108284; 9:8216; 10:7581; 11:5595; 12:528; 13:477; 14:478; 15:100; 16:346; 17:384; 18:6; 19:4; 20:1;
Stats: malloc large: 1535 small slow: 1841
==11528== ABORTING

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Did we regress this recently, perhaps?

Comment 2

[Josh Matthews [:jdm]]

Yes, it would be very useful to find out if this error appears on builds from Oct 12.

Comment 3

[Alex Keybl [:akeybl]]

Matt - can you take a look?

Josh - what's the security rating of this bug? I'm assuming high/critical since this was nominated for tracking.

Comment 4

[Josh Matthews [:jdm]]

I didn't notice the question; sorry for the delay. I'm not experienced at rating these yet, so I would rather somebody else evaluated it.

Comment 5

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

(In reply to Josh Matthews [:jdm] from comment #2)
> Yes, it would be very useful to find out if this error appears on builds
> from Oct 12.

Yes, it still crashes with Oct 12 builds from
https://people.mozilla.com/~choller/firefox/asan/20121012-mozilla-central-macosx64-debug-cfbf2d4218c8+asan.html

What are you suspecting, maybe that changeset is too late in the day?

Comment 6

[Josh Matthews [:jdm]]

I was wondering whether bug 505385 was at fault, but since that landed on mozilla-central on the 13th, it shouldn't be.

Comment 7

[Andrew McCreight [:mccr8]]

It looks like the question for QA has been resolved.

Comment 8

[Alex Keybl [:akeybl]]

This doesn't have a known major user impact, and hasn't been designated critical. No reason to track yet.

Comment 9

[Andrew McCreight [:mccr8]]

Assigning to Joe because he fixed bug 758200. Feel free to reassign as appropriate.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Assigning to JP for gfx triage.

Comment 11

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Assigning to Josh to look into whether this is related to (or even fixed by?) recent fixes in this area such as bug 801453 and bug 802485.

Comment 12

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Matt: please see if this affects earlier releases. I know we have at least Aurora ASAN builds available. for other branches you'd have to learn how to build your own.

Comment 13

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

I can repro on Aurora 2012-11-16, ASAN build, on Ubuntu 11.10.

Which branches are left that require confirmation of this bug?

Comment 14

[Josh Matthews [:jdm]]

Ok, I can still reproduce on a recent m-c, so I'll dig into fixing this.

Comment 15

[Josh Matthews [:jdm]]

Attachment: Restrict GIF pixels to the range of the colormap.

Comment 16

[Josh Matthews [:jdm]]

I would expect this to affect all supported builds, since this error appears to date back to the original patch in 2007.

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 684464 [details] [diff] [review]
Restrict GIF pixels to the range of the colormap.

Review of attachment 684464 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgFrame.cpp
@@ +167,5 @@
>      }
>  
>      // Use the fallible allocator here
>      mPalettedImageData = (uint8_t*)moz_malloc(PaletteDataLength() + GetImageDataLength());
> +    printf("allocated %p (%u) for frame %p\n", mPalettedImageData, PaletteDataLength() + GetImageDataLength(), this);

r-

Comment 18

[Josh Matthews [:jdm]]

Attachment: Restrict GIF pixels to the range of the colormap.

Comment 19

[Josh Matthews [:jdm]]

Comment on attachment 684471 [details] [diff] [review]
Restrict GIF pixels to the range of the colormap.

[Security approval request comment]
How easily can the security issue be deduced from the patch?
The general problem is evident; the specific problem being fixed is less so.

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

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should apply to every branch.

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

Comment 20

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

What can this be exploited to do? I haven't read through the surrounding code, but the patch implies we were reading outside the colormap. That is, potentially incorporating bits of private data into the image (which could then be observed by the page). Is there any danger of /writing/ outside the buffer as implied by "heap-buffer-overflow" in the summary? If not the security severity should be lowered to sec-high or sec-moderate.

Comment 21

[Josh Matthews [:jdm]]

No, there is no danger of writing outside the buffer.

Comment 22

[Alex Keybl [:akeybl]]

We don't typically track sec-moderates (this didn't even have to go through sec-approval in the end). Please nominate for uplift when the patch is ready, if low risk.

Comment 23

[Atte Kettunen]

Just to make sure of the sec rating, if you can't write outside of the buffer, how much can you read?

Comment 24

[Atte Kettunen]

To be more precise. How much and on what range? ASAN doesn't provide much info about the amount, but for the range I have multiple repro-files that have different locations read in relation to the region ASAN detected the overflow with.

Examples:

0x7f7633d0d330 is located 162 bytes to the right of 526-byte region [0x7f7633d0d080,0x7f7633d0d28e)
0x7f37e187cd50 is located 49 bytes to the right of 159-byte region [0x7f37e187cc80,0x7f37e187cd1f)
0x7fc4ad56a7d4 is located 18 bytes to the right of 322-byte region [0x7fc4ad56a680,0x7fc4ad56a7c2)
0x7fb3026ba64c is located 52 bytes to the left of 136-byte region [0x7fb3026ba680,0x7fb3026ba708)

Comment 25

[Josh Matthews [:jdm]]

Each invalid read is one dword up to 255 dwords beyond the end of the combined pixel and colormap data for the GIF frame.

Comment 26

[Josh Matthews [:jdm]]

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

Comment 27

[Josh Matthews [:jdm]]

Comment on attachment 684471 [details] [diff] [review]
Restrict GIF pixels to the range of the colormap.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Lost in the sands of time.
User impact if declined: Infrequent crashes when viewing invalid GIFs.
Testing completed (on m-c, etc.): No ASAN failures.
Risk to taking this patch (and alternatives if risky): Potential for GIF decoding regressions if we've understood the code wrong, but unlikely. 
String or UUID changes made by this patch: None.

I'm inclined not to bother uplifting this beyond beta, given the security rating, but I don't know what the policy is here.

Comment 28

[Ed Morley [:emorley]]

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

Comment 29

[Alex Keybl [:akeybl]]

Comment on attachment 684471 [details] [diff] [review]
Restrict GIF pixels to the range of the colormap.

[Triage Comment]
Since this is only sec-moderate, let's uplift to Aurora 19 and leave Beta 18 as is.

Comment 30

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd7507d7423c

Comment 31

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Shouldn't we add this as a crashtest? It may not reliably crash even when the bug wasn't fixed, but we regularly run our testsuites on ASan builds and will eventually make it an automated thing.

Comment 32

[Josh Matthews [:jdm]]

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

Comment 33

[Ed Morley [:emorley]]

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