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

RESOLVED FIXED in Firefox 22

Status

()

Core
ImageLib
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Atte Kettunen, Assigned: Joe Drew (not getting mail))

Tracking

(4 keywords)

Trunk
mozilla24
x86_64
All
crash, csectype-uaf, sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ verified, firefox23+ verified, firefox24+ fixed, firefox-esr17 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected)

Details

(Whiteboard: [adv-main22-])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 756929 [details]
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
.
.
.
Severity: normal → critical
Keywords: crash, csec-uaf, sec-critical, testcase
OS: Linux → All
Version: unspecified → Trunk
Flags: sec-bounty?
(Assignee)

Updated

5 years ago
Assignee: nobody → joe
(Assignee)

Comment 1

5 years ago
Created attachment 758153 [details] [diff] [review]
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.
Attachment #758153 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #758153 - Flags: review? → review?(seth)
status-firefox24: --- → affected
tracking-firefox24: --- → ?
Marking tracking and status flags for 22 and 23 based on conversation with Andrew.

Is 21 unaffected by this? How about ESR?
status-firefox22: --- → affected
status-firefox23: --- → affected
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
tracking-firefox24: ? → +
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?
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 759746 [details] [diff] [review]
don't leave mCurrentFrame alone if we have an error v2
Attachment #758153 - Attachment is obsolete: true
Attachment #758153 - Flags: review?(seth)
Attachment #759746 - Flags: review?(seth)
(Assignee)

Comment 6

5 years ago
(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.
Blocks: 716140
status-firefox21: --- → unaffected
status-firefox-esr17: --- → unaffected
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;
}
Attachment #759746 - Flags: review?(seth) → review+
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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.
Attachment #759746 - Flags: sec-approval?
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.
Attachment #759746 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 12

5 years ago
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
Attachment #759746 - Flags: approval-mozilla-beta?
Attachment #759746 - Flags: approval-mozilla-aurora?
Pre-approving on branches,please make sure to land on branches once the patch is merged to m-c.
tracking-firefox22: ? → +
tracking-firefox23: ? → +

Updated

5 years ago
Attachment #759746 - Flags: approval-mozilla-beta?
Attachment #759746 - Flags: approval-mozilla-beta+
Attachment #759746 - Flags: approval-mozilla-aurora?
Attachment #759746 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/668d268c6b05
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox24: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Flags: sec-bounty? → sec-bounty+
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).
status-firefox22: fixed → verified
Flags: needinfo?
(Assignee)

Comment 18

5 years ago
Mihaela, do you need any info? (you set the needinfo flag)
Flags: needinfo? → needinfo?(mihaela.velimiroviciu)
I set it by mistake. Anyway, I'm wondering if that was a valid verification, as I used normal builds...
Flags: needinfo?(mihaela.velimiroviciu) → needinfo?
(Assignee)

Comment 20

5 years ago
I have definitely had crashes from this bug, so yes, it's a valid verification!
Flags: needinfo?
Whiteboard: [adv-main22-]
Group: core-security
Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0
Build id: 20130703181823

Verified against firefox 23 beta 3.
status-firefox23: fixed → verified
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.
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → unaffected
You need to log in before you can comment on or make changes to this bug.