Closed Bug 1510471 Opened 2 years ago Closed 2 years ago

use-after-poison in [@ nsComboboxControlFrame::HandleRedisplayTextEvent]

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox-esr60 64+ verified
firefox63 --- wontfix
firefox64 + verified
firefox65 + verified

People

(Reporter: tsmith, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Keywords: crash, sec-high, testcase, Whiteboard: [post-critsmash-triage][adv-main64+][adv-esr60.4+])

Attachments

(2 files)

Attached file testcase.html
==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
Flags: in-testsuite?
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).
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
Has Regression Range: --- → yes
I guess I can fix this, since I can reproduce this even on a debug build.
Assignee: nobody → bugs
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.
Attachment #9028286 - Flags: review?(emilio)
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.
Attachment #9028286 - Flags: review?(emilio) → review+
Because of the kungfuDeathGrip-like part, I'm not convinced this is frame poisoning only.
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
Attachment #9028286 - Flags: sec-approval?
Comment on attachment 9028286 [details] [diff] [review]
combobox_crash.diff

Sec-approval+ from conversation with Ryan. We'll want patches for affected branches too.
Attachment #9028286 - Flags: sec-approval? → sec-approval+
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.
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/4637f389efd5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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
Flags: needinfo?(bugs)
Attachment #9028286 - Flags: approval-mozilla-esr60?
Attachment #9028286 - Flags: approval-mozilla-beta?
Comment on attachment 9028286 [details] [diff] [review]
combobox_crash.diff

[Triage Comment]
Fixes a sec-high. Approved for 64.0rc1 and 60.4.0esr.
Attachment #9028286 - Flags: approval-mozilla-esr60?
Attachment #9028286 - Flags: approval-mozilla-esr60+
Attachment #9028286 - Flags: approval-mozilla-beta?
Attachment #9028286 - Flags: approval-mozilla-beta+
Group: layout-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main64+][adv-esr60.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.