Tyson Smith [:tsmith] Reporter
	Description • 6 years ago

==11149==ERROR: AddressSanitizer: use-after-poison on address 0x6250002fc110 at pc 0x7f84b1135701 bp 0x7ffc8ac933d0 sp 0x7ffc8ac933c8
READ of size 8 at 0x6250002fc110 thread T0 (file:// Content)
    #0 0x7f84b1135700 in get /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:307:27
    #1 0x7f84b1135700 in operator mozilla::ComputedStyle * /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:320
    #2 0x7f84b1135700 in Style /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIFrame.h:739
    #3 0x7f84b1135700 in PresContext /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIFrame.h:587
    #4 0x7f84b1135700 in PresShell /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIFrame.h:591
    #5 0x7f84b1135700 in nsComboboxControlFrame::HandleRedisplayTextEvent() /builds/worker/workspace/build/src/layout/forms/nsComboboxControlFrame.cpp:1064
    #6 0x7f84b11351e3 in nsComboboxControlFrame::RedisplayTextEvent::Run() /builds/worker/workspace/build/src/layout/forms/nsComboboxControlFrame.cpp:67:20
    #7 0x7f84a9f6f8e7 in nsContentUtils::RemoveScriptBlocker() /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp:5672:15
    #8 0x7f84aa4c8016 in ~nsAutoScriptBlocker /builds/worker/workspace/build/src/obj-firefox/dist/include/nsContentUtils.h:3703:5
    #9 0x7f84aa4c8016 in nsIDocument::AdoptNode(nsINode&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:7232
    #10 0x7f84acc6dc8e in mozilla::dom::Document_Binding::adoptNode(JSContext*, JS::Handle<JSObject*>, nsIDocument*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/DocumentBinding.cpp:1710:45
    #11 0x7f84ad5f5c04 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3376:13
    #12 0x7f84b69ff3bd in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:468:15
    #13 0x7f84b69ff3bd in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:560
    #14 0x7f84b69e9047 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:620:12
    #15 0x7f84b69e9047 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3494
    #16 0x7f84b69cc606 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447:12
    #17 0x7f84b69ffd61 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:587:15
    #18 0x7f84b6a019e2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:634:10
    #19 0x7f84b59c4406 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2988:12
    #20 0x7f84acc01df9 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:265:37
    #21 0x7f84ade8ea59 in void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
    #22 0x7f84ade8bce9 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) /builds/worker/workspace/build/src/dom/events/JSEventHandler.cpp:214:12
    #23 0x7f84ade3fe2a in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1115:52
    #24 0x7f84ade42427 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1317:15
    #25 0x7f84ade23bd6 in HandleEvent /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EventListenerManager.h:390:5
    #26 0x7f84ade23bd6 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:425
    #27 0x7f84ade21e58 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:642:16
    #28 0x7f84ade288b0 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:1164:11
    #29 0x7f84ade2fae6 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp
    #30 0x7f84aa5b2c14 in nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/nsINode.cpp:1141:5
    #31 0x7f84ade53e5a in mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&) /builds/worker/workspace/build/src/dom/events/EventTarget.cpp:205:13
    #32 0x7f84add9e743 in mozilla::AsyncEventDispatcher::Run() /builds/worker/workspace/build/src/dom/events/AsyncEventDispatcher.cpp:72:12
    #33 0x7f84a6156655 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:337:32
    #34 0x7f84a6193a88 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1244:14
    #35 0x7f84a619c83d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:530:10
    #36 0x7f84a741759f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #37 0x7f84a73127be in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #38 0x7f84a73127be in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #39 0x7f84a73127be in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #40 0x7f84b02f3fa3 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27
    #41 0x7f84b4beffae in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:961:22
    #42 0x7f84a73127be in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #43 0x7f84a73127be in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #44 0x7f84a73127be in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #45 0x7f84b4bef001 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:787:34
    #46 0x562f0a888864 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #47 0x562f0a888864 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:287
    #48 0x7f84c9c4982f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #49 0x562f0a7adeec in _start (/home/user/workspace/browsers/m-c-20181126161820-fuzzing-asan-opt/firefox+0x2deec)

0x6250002fc110 is located 6160 bytes inside of 8192-byte region [0x6250002fa900,0x6250002fc900)
allocated by thread T0 (file:// Content) here:
    #0 0x562f0a855d93 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3
    #1 0x7f84a6130e00 in mozilla::ArenaAllocator<8192ul, 8ul>::AllocateChunk(unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:193:15
    #2 0x7f84a6126678 in InternalAllocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:228:25
    #3 0x7f84a6126678 in Allocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:75
    #4 0x7f84a6126678 in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:80
    #5 0x7f84b0d0650a in AllocateByFrameID /builds/worker/workspace/build/src/layout/base/nsPresArena.h:39:12
    #6 0x7f84b0d0650a in AllocateFrame /builds/worker/workspace/build/src/layout/base/nsIPresShell.h:207
    #7 0x7f84b0d0650a in operator new /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:34
    #8 0x7f84b0d0650a in NS_NewViewportFrame(nsIPresShell*, mozilla::ComputedStyle*) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:31
    #9 0x7f84b0b1a8b3 in nsCSSFrameConstructor::ConstructRootFrame() /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:2695:5
    #10 0x7f84b0a40b12 in mozilla::PresShell::Initialize() /builds/worker/workspace/build/src/layout/base/PresShell.cpp:1795:36
    #11 0x7f84aa3ef191 in nsContentSink::StartLayout(bool) /builds/worker/workspace/build/src/dom/base/nsContentSink.cpp:1276:26
    #12 0x7f84a8d29fb2 in nsHtml5TreeOpExecutor::StartLayout(bool*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:677:18
    #13 0x7f84a8d26277 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*, bool*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOperation.cpp:1198:17
    #14 0x7f84a8d2320a in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:493:17
    #15 0x7f84a8d2e66b in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:123:18
    #16 0x7f84a6156655 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:337:32
    #17 0x7f84a6193a88 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1244:14
    #18 0x7f84a619c83d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:530:10
    #19 0x7f84a7417594 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:125:5
    #20 0x7f84a73127be in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #21 0x7f84a73127be in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #22 0x7f84a73127be in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #23 0x7f84b02f3fa3 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27
    #24 0x7f84b4beffae in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:961:22
    #25 0x7f84a73127be in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #26 0x7f84a73127be in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #27 0x7f84a73127be in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #28 0x7f84b4bef001 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:787:34

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 1 • 6 years ago

It would make use-after-poison bug reports substantially easier to understand at a glance if they also included the stack of where the memory poisoning occurred (rather than just the allocation and use stacks).

	Ryan VanderMeulen [:RyanVM]
	Comment 2 • 6 years ago

This testcase crashes Firefox as far back as Fx49. Appears to have started with the 2016-04-27 Nightly.

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9ce31e9f90cb0e534611b0f617c5bbc232ffe748&tochange=ab0044bfa1df858919797bcd6a9aef76a668cd4a

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 3 • 6 years ago

Just looking at the code, https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/layout/forms/nsComboboxControlFrame.cpp#1062,1064 looks suspicious. 1062 does notifying DOM change.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 4 • 6 years ago

I guess I can fix this, since I can reproduce this even on a debug build.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 5 • 6 years ago

Ok, this was exactly what the stack trace hinted. The fix for this crash is the weakframe check. Classical layout code issue :)

And since I couldn't prove mDisplayContent is kept alive long enough, added effectively kungfuDeathGrip to ActuallyDisplayText.
(If event loop doesn't spin, mDisplayContent is kept alive.)

The patch is the safest possible I could think of. And the code is similar in esr60.


Separate, non-security bug is to figure out if SetText calls would work without notifying. Notifying calls can do random things.

	Emilio Cobos Álvarez (:emilio)
	Comment 6 • 6 years ago

Comment on attachment 9028286 [details] [diff] [review]
combobox_crash.diff

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

r=me. Given this is only frame poisoning, if you could land a crashtest it'd be great.

::: layout/forms/nsComboboxControlFrame.cpp
@@ +1055,4 @@
>    // Redirect frame insertions during this method (see GetContentInsertionFrame())
>    // so that any reframing that the frame constructor forces upon us is inserted
>    // into the correct parent (mDisplayFrame). See bug 282607.
>    MOZ_ASSERT(!mInRedisplayText, "Nested RedisplayText");

FWIW all this is pretty broken, looks like. It expects mDisplayText to be created synchronously under this method, but this will not happen, since we'll do frame construction async. I may have broken this in bug 1447506, I'll file a followup to see what this code is trying to do.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 7 • 6 years ago

Because of the kungfuDeathGrip-like part, I'm not convinced this is frame poisoning only.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 8 • 6 years ago

Comment on attachment 9028286 [details] [diff] [review]
combobox_crash.diff

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Exploit can be hard, but the patch does pinpoint very clearly that there is some issue with the code.

Commit message could be.  
-m " Bug 1510471, avoid useless FrameNeedsReflow call, r=emilio"

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

Which older supported branches are affected by this flaw?: all

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

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?: This should be super safe: null check and kungfuDeathGrip

	Al Billings [:abillings - ex-MoCo]
	Comment 9 • 6 years ago

Comment on attachment 9028286 [details] [diff] [review]
combobox_crash.diff

Sec-approval+ from conversation with Ryan. We'll want patches for affected branches too.

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4637f389efd53860d740eb5aa74f8973df37e95d

Please request Beta & ESR60 approval on this when you get a chance. It grafts cleanly to both as-landed.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 11 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/4637f389efd5

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 12 • 6 years ago

Comment on attachment 9028286 [details] [diff] [review]
combobox_crash.diff

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: old code

User impact if declined: Crashes, possibly sensitive crashes.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: Run debug build and open the testcase in this bug

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Adding effectively a null check + kungfuDeathGrip

String changes made/needed: NA

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Should be very safe fix for a crash, which is possibly security sensitive

User impact if declined: Crashes, possibly sensitive crashes.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Adding effectively a null check + kungfuDeathGrip

String or UUID changes made by this patch: NA

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 6 years ago

Comment on attachment 9028286 [details] [diff] [review]
combobox_crash.diff

[Triage Comment]
Fixes a sec-high. Approved for 64.0rc1 and 60.4.0esr.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/2cc5c3f56a46

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/20e31905de62

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 6 years ago
uplift

ESR60 bustage follow-up:
https://hg.mozilla.org/releases/mozilla-esr60/rev/c2832f98fe51

	Catalin Sasca, Desktop QA [:csasca]
	Comment 17 • 6 years ago

I managed to reproduce the crash on Firefox Nightly 65.0a1 (20181110213725) under Windows 10 (x64) using the attachment in Comment 0.

The issue is fixed on latest Nightly 65.0a1 (2018-12-02), latest beta from taskcluster (20181202212448), and latest ESR 60.3.1 build from taskcluster (20181202213303) using the attached file. Tests were performed under Windows 10 (x64), Ubuntu 18.04 (x64) and macOS 10.12.