Heap-use-after-free in nsAsyncDOMEvent::Run

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: joe, Assigned: joe)

Tracking

({crash, csectype-uaf, sec-critical})

Trunk
mozilla22
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox19 wontfix, firefox20+ fixed, firefox21+ fixed, firefox22+ fixed, firefox-esr1720+ verified, b2g1820+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 affected)

Details

(Whiteboard: [asan][adv-main20+][adv-esr1705+] first noted in bug 716140 comment 93)

Attachments

(1 attachment)

+++ This bug was initially created as a clone of Bug #835814 +++

Unfortunately this bug still exists after the patch of bug 835814, it just comes up on fewer occasions. (It's also probably harder to create a testcase for.)

Further unfortunately, it blocks my work on bug 716140. I'm getting a build up and running on the record-and-replay box.

>==6846== ERROR: AddressSanitizer: heap-use-after-free on address 0x7f1873a98298 at pc 0x7f18a5b0eb72 bp 0x7fff6a3d1ad0 sp 0x7fff6a3d1ac8
>READ of size 8 at 0x7f1873a98298 thread T0
>    #0 0x7f18a5b0eb71 in nsCOMPtr<nsINodeInfo>::get() const src/../../dist/include/nsCOMPtr.h:764
>    #1 0x7f18a5b0eda9 in nsCOMPtr<nsINodeInfo>::operator->() const src/../../dist/include/nsCOMPtr.h:784
>    #2 0x7f18a5ab6ff3 in nsINode::OwnerDoc() const src/../../dist/include/nsINode.h:476
>    #3 0x7f18973cc3c0 in nsAsyncDOMEvent::Run() src/content/events/src/nsAsyncDOMEvent.cpp:35
>    #4 0x7f18a477fbdf in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
>    #5 0x7f189205ef95 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
>    #6 0x7f18a259a7dc in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
>    #7 0x7f18945742c2 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:215
>    #8 0x7f1894573429 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:208
>    #9 0x7f18945795ae in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
>    #10 0x7f1896aa0f47 in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
>    #11 0x7f1895007b35 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:288
>    #12 0x7f189eb04534 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3823
>    #13 0x7f189eb0f92a in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3890
>    #14 0x7f1891a692b0 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4093
>    #15 0x4520f3 in do_main(int, char**, nsIFile*) src/browser/app/nsBrowserApp.cpp:185
>    #16 0x4d9392 in main src/browser/app/nsBrowserApp.cpp:377
>    #17 0x7f18b37c876c in
>0x7f1873a98298 is located 24 bytes inside of 208-byte region [0x7f1873a98280,0x7f1873a98350)
>freed by thread T0 here:
>    #0 0x40f082 in __interceptor_free
>    #1 0x7f18b47e54b9 in moz_free src/memory/mozalloc/mozalloc.cpp:48
>    #2 0x7f18a02cf830 in operator delete(void*) src/../../../../dist/include/mozilla/mozalloc.h:224
>    #3 0x7f18a02cf830 in mozilla::dom::HTMLImageElement::~HTMLImageElement() src/content/html/content/src/HTMLImageElement.cpp:83
>    #4 0x7f18948851f7 in nsNodeUtils::LastRelease(nsINode*) src/content/base/src/nsNodeUtils.cpp:258
>    #5 0x7f18a045dc10 in mozilla::dom::FragmentOrElement::Release() src/content/base/src/FragmentOrElement.cpp:1685
>    #6 0x7f18a02c7f8a in mozilla::dom::HTMLImageElement::Release() src/content/html/content/src/HTMLImageElement.cpp:89
>    #7 0x7f1895c1fa9f in nsCOMPtr_base::~nsCOMPtr_base() src/objdir-ff-asan-sym/media/webrtc/signaling/signaling_ecc/../../../../dist/include/nsCOMPtr.h:410
>    #8 0x7f18a411c48c in nsCOMPtr<nsINode>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
>    #9 0x7f18a411c379 in nsCOMPtr<nsINode>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
>    #10 0x7f18973ce1a7 in nsAsyncDOMEvent::~nsAsyncDOMEvent() src/../../../dist/include/nsAsyncDOMEvent.h:23
>    #11 0x7f189b565ce0 in nsLoadBlockingAsyncDOMEvent::~nsLoadBlockingAsyncDOMEvent() src/content/events/src/nsAsyncDOMEvent.cpp:63
>    #12 0x7f189b565a3f in nsLoadBlockingAsyncDOMEvent::~nsLoadBlockingAsyncDOMEvent() src/content/events/src/nsAsyncDOMEvent.cpp:59
>    #13 0x7f1894468a35 in nsRunnable::Release() src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:31
>    #14 0x7f1895c1fa9f in nsCOMPtr_base::~nsCOMPtr_base() src/objdir-ff-asan-sym/media/webrtc/signaling/signaling_ecc/../../../../dist/include/nsCOMPtr.h:410
>    #15 0x7f18a3e0c58c in nsCOMPtr<nsIRunnable>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
>    #16 0x7f18a3e0c479 in nsCOMPtr<nsIRunnable>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
>    #17 0x7f18a477fcdf in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:633
>    #18 0x7f189205ef95 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
>    #19 0x7f18a259a7dc in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
>    #20 0x7f18945742c2 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:215
>    #21 0x7f1894573429 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:208
>    #22 0x7f18945795ae in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
>    #23 0x7f1896aa0f47 in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
>    #24 0x7f1895007b35 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:288
>    #25 0x7f189eb04534 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3823
>    #26 0x7f189eb0f92a in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3890
>    #27 0x7f1891a692b0 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4093
>    #28 0x4520f3 in do_main(int, char**, nsIFile*) src/browser/app/nsBrowserApp.cpp:185
>    #29 0x4d9392 in main src/browser/app/nsBrowserApp.cpp:377
>previously allocated by thread T0 here:
>    #0 0x40f162 in __interceptor_malloc
>    #1 0x7f18b47e5604 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
>    #2 0x7f18920c186f in operator new(unsigned long) src/../../../../dist/include/mozilla/mozalloc.h:200
>    #3 0x7f18920c186f in NS_NewHTMLImageElement(already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/html/content/src/HTMLImageElement.cpp:66
>    #4 0x7f18958d8c95 in nsVideoFrame::CreateAnonymousContent(nsTArray<nsIAnonymousContentCreator::ContentInfo>&) src/layout/generic/nsVideoFrame.cpp:75
>    #5 0x7f18a68218d9 in non-virtual thunk to nsVideoFrame::CreateAnonymousContent(nsTArray<nsIAnonymousContentCreator::ContentInfo>&) src/layout/generic/nsVideoFrame.cpp:116
>    #6 0x7f189a43bf19 in nsCSSFrameConstructor::GetAnonymousContent(nsIContent*, nsIFrame*, nsTArray<nsIAnonymousContentCreator::ContentInfo>&) src/layout/base/nsCSSFrameConstructor.cpp:3903
>    #7 0x7f189a41c7b6 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsIFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) src/layout/base/nsCSSFrameConstructor.cpp:9967
>    #8 0x7f189a49b656 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:3745
>    #9 0x7f189a46d015 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:5542
>    #10 0x7f189a49211b in nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:9903
>    #11 0x7f189a4194d6 in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsIContent*, bool) src/layout/base/nsCSSFrameConstructor.cpp:6725
>    #12 0x7f189a4367e8 in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) src/layout/base/nsCSSFrameConstructor.cpp:6380
>    #13 0x7f189a436c61 in nsCSSFrameConstructor::CreateNeededFrames() src/layout/base/nsCSSFrameConstructor.cpp:6405
>    #14 0x7f18a48722c2 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/nsPresShell.cpp:3867
>    #15 0x7f18a48710d3 in PresShell::FlushPendingNotifications(mozFlushType) src/layout/base/nsPresShell.cpp:3757
>    #16 0x7f18942da2cd in nsDocument::FlushPendingNotifications(mozFlushType) src/content/base/src/nsDocument.cpp:6727
>    #17 0x7f1894729766 in nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:729
>    #18 0x7f18947240f5 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) src/uriloader/base/nsDocLoader.cpp:659
>    #19 0x7f18a763055b in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) src/uriloader/base/nsDocLoader.cpp:663
>    #20 0x7f1894823461 in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult) src/netwerk/base/src/nsLoadGroup.cpp:676
>    #21 0x7f189428efd0 in nsDocument::DoUnblockOnload() src/content/base/src/nsDocument.cpp:7600
>    #22 0x7f1894280a41 in nsDocument::UnblockOnload(bool) src/content/base/src/nsDocument.cpp:7542
>    #23 0x7f18942de953 in nsDocument::DispatchContentLoadedEvents() src/content/base/src/nsDocument.cpp:4408
>    #24 0x7f189a1a3062 in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() src/../../../dist/include/nsThreadUtils.h:367
>    #25 0x7f18a477fbdf in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
>Shadow bytes around the buggy address:
>  0x1fe30e753000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1fe30e753010: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fe30e753020: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fe30e753030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1fe30e753040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>=>0x1fe30e753050: fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fe30e753060: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fe30e753070: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1fe30e753080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1fe30e753090: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fe30e7530a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>Shadow byte legend (one shadow byte represents 8 application bytes):
>  Addressable:           00
>  Partially addressable: 01 02 03 04 05 06 07
>  Heap left redzone:     fa
>  Heap righ redzone:     fb
>  Freed Heap region:     fd
>  Stack left redzone:    f1
>  Stack mid redzone:     f2
>  Stack right redzone:   f3
>  Stack partial redzone: f4
>  Stack after return:    f5
>  Stack use after scope: f8
>  Global redzone:        f9
>  Global init order:     f6
>  Poisoned by user:      f7
>  ASan internal:         fe
>Stats: 274M malloced (295M for red zones) by 451143 calls
>Stats: 48M realloced by 25728 calls
>Stats: 245M freed by 320606 calls
>Stats: 110M really freed by 202828 calls
>Stats: 492M (492M-0M) mmaped; 123 maps, 0 unmaps
>  mmaps   by size class: 8:294894; 9:40955; 10:12285; 11:14329; 12:4096; 13:1536; 14:1280; 15:384; 16:1216; 17:1312; 18:64; 19:40; 20:24;
>  mallocs by size class: 8:368736; 9:40833; 10:12926; 11:17620; 12:3670; 13:2004; 14:1736; 15:459; 16:1577; 17:1440; 18:77; 19:42; 20:23;
>  frees   by size class: 8:256057; 9:30657; 10:9412; 11:15499; 12:2386; 13:1682; 14:1564; 15:329; 16:1481; 17:1417; 18:63; 19:39; 20:20;
>  rfrees  by size class: 8:175259; 9:10652; 10:2677; 11:10244; 12:779; 13:679; 14:773; 15:177; 16:980; 17:577; 18:26; 19:4; 20:1;
>Stats: malloc large: 1582 small slow: 2700
>Stats: StackDepot: 0 ids; 0M mapped
>==6846== ABORTING
>
To be clear, this comes up at least when you run mochitest-browser-chrome with my patches from bug 716140 applied. I don't know how to reproduce this without those patches.
The fundamental problem here is that we're trying to notify something that's in the middle of destroying itself. It's even *told us* that it's destroying itself by calling CancelAndForgetObserver(). Some leak problems made us hesitate to ignore OnStopRequest in bug 393936, where that interface method was introduced, but I think we're OK to do so now.

This patch is running (alone) through try: https://tbpl.mozilla.org/?tree=Try&rev=c126127e3f03

The thing we'll need to look out for most is leaks because of something that calls CancelAndForgetObserver while still wanting OnStopRequest.
Assignee: nobody → joe
Attachment #713549 - Flags: review?(khuey)
One other question for Kyle is whether we should revert the change in bug 835814 in favour of this one.
Joe do you think OMT-ImageDecode might land in 21?
No, definitely not. (If it did, by some miracle, I'd back it out of Aurora.)
(In reply to David Bolter [:davidb] from comment #4)
> Joe do you think OMT-ImageDecode might land in 21?

BTW - this is an underlying bug that is easily exposed with the patches on bug OMT-ImageDecode, but I expect that it is exploitable without those patches.
(In reply to Joe Drew (:JOEDREW! \o/) from comment #6)
> (In reply to David Bolter [:davidb] from comment #4)
> > Joe do you think OMT-ImageDecode might land in 21?
> 
> BTW - this is an underlying bug that is easily exposed with the patches on
> bug OMT-ImageDecode, but I expect that it is exploitable without those
> patches.

Oh darn and my flags are wrong. Does it affect all branches?
Almost certainly. But with no testcase.
Comment on attachment 713549 [details] [diff] [review]
don't send OnStopRequest() if we're canceled

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

Skipping just this one notification seems wrong.  Why not skip them all?
This is adding OnStopRequest to the (vast majority) of notifications that we skip if (mCanceled).
(The only one we don't skip is OnStartRequest, I believe.)
That lack of symmetry is a little weird, though, don't you think? Intuitively you'd expect to get OnStopRequest if you get OnStartRequest.
Certainly. The problem is that, since we drop our ref to the listener *before* OnStopRequest (to avoid leaks), if a listener Cancel()s us and then goes away, there's no object there to get confused.

The real solution here is cycle collecting Imagelib observers, but that's hard.
Comment on attachment 713549 [details] [diff] [review]
don't send OnStopRequest() if we're canceled

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

With some difficulty. We don't actually have a testcase that exercises this bug on trunk, but the patches in bug make it happen with mochitest-browser-chrome. A very determined (or observant—we've had similar bugs in the past) attacker could probably reverse-engineer an exploit, but I'm not sure how.

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

No.

Which older supported branches are affected by this flaw?

Likely all.

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

In most cases we'd crash in the cases that this patch would have any effect, but I can conceive of ways that it could affect correctness. I'd want it to bake on mozilla-central for a little bit if we were going to uplift it.
Attachment #713549 - Flags: sec-approval?
Attachment #713549 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/40b4bc5cac71

Can we get a test for this too once it hits all the necessary branches?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
I have no idea how to test this without my patches in bug 716140. With it, we already have tests that crash. :-)
Flags: in-testsuite? → in-testsuite-
Please nominate this patch for uplift to Aurora/Beta/ESR17 when you get the chance. Thanks!
This is marked tracking+ for 20, which is closing its doors soon. Joe, what's the plan on uplift here?
Flags: needinfo?(joe)
Comment on attachment 713549 [details] [diff] [review]
don't send OnStopRequest() if we're canceled

Sorry, I missed Alex's comment! 

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression
User impact if declined: Security bug
Testing completed (on m-c, etc.): On m-c for a few days
Risk to taking this patch (and alternatives if risky): Not terribly risky.
String or UUID changes made by this patch: none
Attachment #713549 - Flags: approval-mozilla-esr17?
Attachment #713549 - Flags: approval-mozilla-beta?
Attachment #713549 - Flags: approval-mozilla-aurora?
Flags: needinfo?(joe)
Comment on attachment 713549 [details] [diff] [review]
don't send OnStopRequest() if we're canceled

Thanks Joe - we'll get this into our fourth week Beta if it lands today.
Attachment #713549 - Flags: approval-mozilla-esr17?
Attachment #713549 - Flags: approval-mozilla-esr17+
Attachment #713549 - Flags: approval-mozilla-beta?
Attachment #713549 - Flags: approval-mozilla-beta+
Attachment #713549 - Flags: approval-mozilla-aurora?
Attachment #713549 - Flags: approval-mozilla-aurora+
Whiteboard: [asan] first noted in bug 716140 comment 93 → [asan][adv-main20+][adv-esr1705+] first noted in bug 716140 comment 93
Because there is no repository of old ASan builds for 17esr - or any repository at all - reproducing this crash on 17esr is more difficult. It does not repro on non-ASan 17.0.4esr. However, to see the crash, I can use a nightly m-c build (with ASan) from around that date to see the crash. I can also use my own 17.0.5esr ASan build to verify that it's fixed.

Also, used bug file from related bug 835814.

Confirmed crash with m-c 2012-02-11 (ASan)
Confirmed fixed with 17.0.5esr 2013-03-28 (custom local ASan build, changeset cd7d672b40ea)
Attachment #713549 - Flags: approval-mozilla-b2g18+
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.