878392 - Heap-use-after-free in imgFrame::MarkImageDataDirty

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Atte Kettunen]

Attachment: Repro-file

Tested on:

OS: Ubuntu 12.04

Firefox: ASAN debug-build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-dbg-asan/1370061653/

ASAN-report:

==17560== ERROR: AddressSanitizer: heap-use-after-free on address 0x7fa676aa7140 at pc 0x7fa6a47a51b7 bp 0x7fff044eafe0 sp 0x7fff044eafd8
READ of size 8 at 0x7fa676aa7140 thread T0
    #0 0x7fa6a47a51b6 in nsRefPtr<gfxImageSurface>::get() const /builds/slave/m-cen-l64-dbg-asan-00000000000/build/../../dist/include/nsAutoPtr.h:1009
    #1 0x7fa6a47b9539 in imgFrame::MarkImageDataDirty() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/imgFrame.cpp:680
    #2 0x7fa6a4797f84 in mozilla::image::RasterImage::FinishedSomeDecoding(mozilla::image::RasterImage::eShutdownIntent, mozilla::image::RasterImage::DecodeRequest*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/RasterImage.cpp:3486
    #3 0x7fa6a479d06e in mozilla::image::RasterImage::DecodePool::DecodeABitOf(mozilla::image::RasterImage*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/RasterImage.cpp:3681
    #4 0x7fa6a479c9ff in mozilla::image::RasterImage::RequestDecodeCore(mozilla::image::RasterImage::RequestDecodeType) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/RasterImage.cpp:2880
    #5 0x7fa6a500b124 in nsImageLoadingContent::OnStopRequest(imgIRequest*, tag_nsresult) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/base/src/nsImageLoadingContent.cpp:239
    #6 0x7fa6a500a7da in nsImageLoadingContent::Notify(imgIRequest*, int, nsIntRect const*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/base/src/nsImageLoadingContent.cpp:164
    #7 0x7fa6a47def02 in imgRequestProxy::OnStopRequest(bool) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/imgRequestProxy.cpp:848
    #8 0x7fa6a47e3c6c in imgStatusTracker::SyncNotifyState(nsTObserverArray<imgRequestProxy*>&, bool, unsigned int, nsIntRect&, bool) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/imgStatusTracker.cpp:518
    #9 0x7fa6a47e47f1 in imgStatusTracker::SyncNotifyDifference(imgStatusTracker::StatusDiff) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/imgStatusTracker.cpp:581
.
.
.
freed by thread T0 here:
    #0 0x43afe0 in free ??:0
    #1 0x7fa6a47952e1 in operator delete(void*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/../../dist/include/mozilla/mozalloc.h:225
    #2 0x7fa6a4796398 in mozilla::image::RasterImage::EnsureFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char, unsigned char**, unsigned int*, unsigned int**, unsigned int*, imgFrame**) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/RasterImage.cpp:1440
    #3 0x7fa6a47965ba in mozilla::image::RasterImage::EnsureFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char**, unsigned int*, imgFrame**) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/RasterImage.cpp:1457
    #4 0x7fa6a47853ce in mozilla::image::Decoder::AllocateFrame() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/Decoder.cpp:210
    #5 0x7fa6a47841d9 in mozilla::image::Decoder::Write(char const*, unsigned int) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/Decoder.cpp:111
    #6 0x7fa6a4797cc4 in mozilla::image::RasterImage::WriteToDecoder(char const*, unsigned int) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/RasterImage.cpp:2722
    #7 0x7fa6a479d4e6 in mozilla::image::RasterImage::DecodeSomeData(unsigned int) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/RasterImage.cpp:3337
    #8 0x7fa6a47a0b26 in mozilla::image::RasterImage::DecodePool::DecodeSomeOfImage(mozilla::image::RasterImage*, mozilla::image::RasterImage::DecodePool::DecodeType, unsigned int) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/image/src/RasterImage.cpp:3878
.
.
.

Comment 1

[Joe Drew (not getting mail)]

Attachment: don't leave mCurrentFrame alone if we have an error

This is simple forgetting to reset the imgFrame pointer if we have an error creating the image. I changed the code to make that kind of error less likely in the future.

Comment 2

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

Marking tracking and status flags for 22 and 23 based on conversation with Andrew.

Is 21 unaffected by this? How about ESR?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 758153 [details] [diff] [review]
don't leave mCurrentFrame alone if we have an error

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

::: image/src/Decoder.cpp
@@ +220,3 @@
>    } else if (NS_FAILED(rv)) {
>      PostDataError();
> +    mCurrentFrame = nullptr;

What about the else {} - should that be covered for mCurrentFrame?  Or perhaps mCurrenFrame be set to nullptr even before the if?

Comment 4

[Joe Drew (not getting mail)]

I had a simply *great* time trying to post this comment, hitting *exactly* this bug because the repro file was being previewed by bugzilla.js. :-)

Yes! You're right! I'm going to set up a parallel if, because one is for notification and one is for setting mCurrentFrame.

Comment 5

[Joe Drew (not getting mail)]

Attachment: don't leave mCurrentFrame alone if we have an error v2

Comment 6

[Joe Drew (not getting mail)]

(In reply to Al Billings [:abillings] from comment #2)
> Is 21 unaffected by this? How about ESR?

This is a regression from bug 716140, so unless we have a parallel bug we shouldn't have this problem in 21/esr17.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 759746 [details] [diff] [review]
don't leave mCurrentFrame alone if we have an error v2

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

::: image/src/Decoder.cpp
@@ +217,5 @@
> +  if (NS_SUCCEEDED(rv)) {
> +    mCurrentFrame = frame;
> +  } else {
> +    mCurrentFrame = nullptr;
> +  }

Wouldn't it be equivalent to dispense with |frame| altogether and just null out mCurrentFrame on failure? Something like this:

if (NS_FAILED(rv)) {
  mCurrentFrame = nullptr;
}

Comment 8

[Joe Drew (not getting mail)]

Yes, definitely. But I wanted to make it 100% clear when we were modifying mCurrentFrame so there was nothing implicit about it, and so we didn't accidentally leave ourselves in an inconsistent state.

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 759746 [details] [diff] [review]
don't leave mCurrentFrame alone if we have an error v2

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

Relatively easily. Once you know you have to make EnsureFrame fail, you could audit the code and see the delete call, and from there it should be a short hop.

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

No more than implied by above.

Which older supported branches are affected by this flaw?

mozilla-aurora and mozilla-beta only.

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

bug 716140.

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

super-easy.

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

Not very likely at all.

Comment 10

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

Comment on attachment 759746 [details] [diff] [review]
don't leave mCurrentFrame alone if we have an error v2

Sec-approval+ for trunk. Please nominate for branches so we can try to avoid shipping the vulnerable code.

Comment 11

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/668d268c6b05

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 759746 [details] [diff] [review]
don't leave mCurrentFrame alone if we have an error v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 716140
User impact if declined: security hole, crashes
Testing completed (on m-c, etc.): Tested locally, try, and just pushed to inbound
Risk to taking this patch (and alternatives if risky): Very low risk. If something was broken, all animated images would break.
String or IDL/UUID changes made by this patch: none

Comment 13

[bhavana bajaj [:bajaj]]

Pre-approving on branches,please make sure to land on branches once the patch is merged to m-c.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/668d268c6b05

Comment 15

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c0035802660
https://hg.mozilla.org/releases/mozilla-beta/rev/3cf1f6155bf0

Comment 17

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (X11; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0

Verified the fix on Firefox 22 beta 5 (non-debug), by loading the Repro-file attached in bug description: page loads and no sudden quits occur (like it did on the build attached in bug description).

Comment 18

[Joe Drew (not getting mail)]

Mihaela, do you need any info? (you set the needinfo flag)

Comment 19

[Mihaela Velimiroviciu (:mihaelav)]

I set it by mistake. Anyway, I'm wondering if that was a valid verification, as I used normal builds...

Comment 20

[Joe Drew (not getting mail)]

I have definitely had crashes from this bug, so yes, it's a valid verification!

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0
Build id: 20130703181823

Verified against firefox 23 beta 3.

Comment 22

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0

Verified on the rest of the main platforms, as well.