Closed Bug 1371657 Opened 8 years ago Closed 8 years ago

Heap use-after-free in @[nsFrameSelection::cycleCollection::TraverseNative(void*, nsCycleCollectionTraversalCallback&) /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:613]

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Version:
43 Branch
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 56+ fixed
firefox54 --- wontfix
firefox55 + wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: jkratzer, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main56+][adv-esr52.4+][post-critsmash-triage])

Attachments

(4 files, 5 obsolete files)

Testcase
1.23 KB, text/html
Details
for esr52
7.20 KB, patch
jcristau
: approval-mozilla-esr52+
Details | Diff | Splinter Review
editor_root_v5.diff
7.88 KB, patch
Details | Diff | Splinter Review
editor_root_v5_beta.diff
7.62 KB, patch
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review
Found while fuzzing mozilla-central rev 20170602-aeb3d0ca558f. Will update with a testcase shortly. ==10192==ERROR: AddressSanitizer: heap-use-after-free on address 0x622000807108 at pc 0x7fd02eab9ba1 bp 0x7ffde84db860 sp 0x7ffde84db858 READ of size 8 at 0x622000807108 thread T0 #0 0x7fd02eab9ba0 in get /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:742:48 #1 0x7fd02eab9ba0 in operator nsIDocument * /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:750 #2 0x7fd02eab9ba0 in GetDocument /home/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:265 #3 0x7fd02eab9ba0 in nsFrameSelection::cycleCollection::TraverseNative(void*, nsCycleCollectionTraversalCallback&) /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:613 #4 0x7fd027c5af1f in TraverseNativeAndJS /home/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:128:19 #5 0x7fd027c5af1f in CCGraphBuilder::BuildGraph(js::SliceBudget&) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2269 #6 0x7fd027c620c7 in nsCycleCollector::MarkRoots(js::SliceBudget&) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2865:33 #7 0x7fd027c680ab in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3654:9 #8 0x7fd027c6be60 in nsCycleCollector_collect(nsICycleCollectorListener*) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:4187:21 #9 0x7fd02a958520 in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int) /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:1448:3 #10 0x7fd02a957b61 in nsJSEnvironmentObserver::Observe(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:344:7 #11 0x7fd027ccdecc in nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/xpcom/ds/nsObserverList.cpp:112:19 #12 0x7fd027cd1644 in nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/xpcom/ds/nsObserverService.cpp:281:19 #13 0x7fd027dda9b1 in NS_InvokeByIndex /home/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:129 #14 0x7fd02964521b in Invoke /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1996:12 #15 0x7fd02964521b in Call /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1315 #16 0x7fd02964521b in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1282 #17 0x7fd02964c2ff in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:982:12 #18 0x7fd031cce423 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #19 0x7fd031cce423 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470 #20 0x7fd031cb6f6b in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:521:12 #21 0x7fd031cb6f6b in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3028 #22 0x7fd031c9d198 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:410:12 #23 0x7fd031cce5a8 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:488:15 #24 0x7fd031ccedd2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534:10 #25 0x7fd032634973 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2832:12 #26 0x7fd02956d49b in xpc::FunctionForwarder(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/ExportHelpers.cpp:319:18 #27 0x7fd031cce423 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #28 0x7fd031cce423 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470 #29 0x7fd031ef1c2d in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:2453:14 #30 0x7fcfd6f13382 (<unknown module>) 0x622000807108 is located 8 bytes inside of 5776-byte region [0x622000807100,0x622000808790) freed by thread T0 here: #0 0x4bb62b in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:47:3 #1 0x7fd02e69fdec in Release /home/worker/workspace/build/src/layout/base/PresShell.cpp:892:1 #2 0x7fd02e69fdec in non-virtual thunk to mozilla::PresShell::Release() /home/worker/workspace/build/src/layout/base/PresShell.cpp:892 #3 0x7fd02e0a4c15 in ~nsCOMPtr_base /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:294:7 #4 0x7fd02e0a4c15 in nsTextServicesDocument::~nsTextServicesDocument() /home/worker/workspace/build/src/editor/txtsvc/nsTextServicesDocument.cpp:96 #5 0x7fd02e0a500d in nsTextServicesDocument::~nsTextServicesDocument() /home/worker/workspace/build/src/editor/txtsvc/nsTextServicesDocument.cpp:94:1 #6 0x7fd027c61b57 in SnowWhiteKiller::~SnowWhiteKiller() /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2651:25 #7 0x7fd027c68b3b in FreeSnowWhite /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2826:3 #8 0x7fd027c68b3b in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3829 #9 0x7fd027c68053 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3650:9 #10 0x7fd027c6be60 in nsCycleCollector_collect(nsICycleCollectorListener*) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:4187:21 #11 0x7fd02a958520 in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int) /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:1448:3 #12 0x7fd02a957b61 in nsJSEnvironmentObserver::Observe(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:344:7 #13 0x7fd027ccdecc in nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/xpcom/ds/nsObserverList.cpp:112:19 #14 0x7fd027cd1644 in nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/xpcom/ds/nsObserverService.cpp:281:19 #15 0x7fd027dda9b1 in NS_InvokeByIndex /home/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:129 #16 0x7fd02964521b in Invoke /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1996:12 #17 0x7fd02964521b in Call /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1315 #18 0x7fd02964521b in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1282 #19 0x7fd02964c2ff in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:982:12 #20 0x7fd031cce423 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #21 0x7fd031cce423 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470 #22 0x7fd031cb6f6b in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:521:12 #23 0x7fd031cb6f6b in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3028 #24 0x7fd031c9d198 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:410:12 #25 0x7fd031cce5a8 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:488:15 #26 0x7fd031ccedd2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534:10 #27 0x7fd032634973 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2832:12 #28 0x7fd02956d49b in xpc::FunctionForwarder(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/ExportHelpers.cpp:319:18 #29 0x7fd031cce423 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #30 0x7fd031cce423 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470 #31 0x7fd031ef1c2d in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:2453:14 #32 0x7fcfd6f13382 (<unknown module>) #33 0x6210020905ff (<unknown module>) #34 0x7fcfd6f088a5 (<unknown module>) #35 0x7fd031f19812 in EnterBaseline(JSContext*, js::jit::EnterJitData&) /home/worker/workspace/build/src/js/src/jit/BaselineJIT.cpp:160:9 #36 0x7fd031f1a9a5 in js::jit::EnterBaselineAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) /home/worker/workspace/build/src/js/src/jit/BaselineJIT.cpp:268:28 #37 0x7fd031cc0c7f in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:1979:28 previously allocated by thread T0 here: #0 0x4bb97c in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3 #1 0x4ece9d in moz_xmalloc /home/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:83:17 #2 0x7fd02a83ed26 in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12 #3 0x7fd02a83ed26 in nsDocument::CreateShell(nsPresContext*, nsViewManager*, mozilla::StyleSetHandle) /home/worker/workspace/build/src/dom/base/nsDocument.cpp:3872 #4 0x7fd02e791d27 in nsDocumentViewer::InitPresentationStuff(bool) /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:706:27 #5 0x7fd02e791609 in nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool) /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:964:10 #6 0x7fd02e790817 in nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:690:10 #7 0x7fd030d3f43b in nsDocShell::SetupNewViewer(nsIContentViewer*) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:9469:7 #8 0x7fd030d3dcb6 in nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7294:17 #9 0x7fd030cd4841 in nsDocShell::CreateContentViewer(nsACString const&, nsIRequest*, nsIStreamListener**) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:9274:3 #10 0x7fd030cd171b in nsDSURIContentListener::DoContent(nsACString const&, bool, nsIRequest*, nsIStreamListener**, bool*) /home/worker/workspace/build/src/docshell/base/nsDSURIContentListener.cpp:129:21 #11 0x7fd029849186 in nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) /home/worker/workspace/build/src/uriloader/base/nsURILoader.cpp:756:28 #12 0x7fd02984635c in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) /home/worker/workspace/build/src/uriloader/base/nsURILoader.cpp:434:30 #13 0x7fd029844b73 in nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) /home/worker/workspace/build/src/uriloader/base/nsURILoader.cpp:296:8 #14 0x7fd027f23877 in nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) /home/worker/workspace/build/src/netwerk/base/nsBaseChannel.cpp:849:25 #15 0x7fd027f6fcb0 in nsInputStreamPump::OnStateStart() /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:539:25 #16 0x7fd027f6f1fe in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:441:25 #17 0x7fd027d5babd in nsInputStreamReadyEvent::Run() /home/worker/workspace/build/src/xpcom/io/nsStreamUtils.cpp:96:20 #18 0x7fd027dbdd0e in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1321:14 #19 0x7fd027dca158 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:472:10 #20 0x7fd028b940f1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21 #21 0x7fd028af1660 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:10 #22 0x7fd028af1660 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #23 0x7fd028af1660 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #24 0x7fd02dfacbdf in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:27 #25 0x7fd03163e8c1 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:30 #26 0x7fd03180da54 in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4571:22 #27 0x7fd03180f5dc in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4751:8 #28 0x7fd031810931 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4846:21 #29 0x4eb5a3 in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:236:22 #30 0x4eb5a3 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:309 #31 0x7fd043ac982f in __libc_start_main /build/glibc-Qz8a69/glibc-2.23/csu/../csu/libc-start.c:291 SUMMARY: AddressSanitizer: heap-use-after-free /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:742:48 in get Shadow bytes around the buggy address: 0x0c44800f8dd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c44800f8de0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c44800f8df0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c44800f8e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c44800f8e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa =>0x0c44800f8e20: fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c44800f8e30: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c44800f8e40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c44800f8e50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c44800f8e60: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c44800f8e70: 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 right 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 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==10192==ABORTING
Attached file Testcase —
Flags: in-qa-testsuite?
Keywords: testcase
Summary: Intermittent heap use-after-free in @[nsFrameSelection::cycleCollection::TraverseNative(void*, nsCycleCollectionTraversalCallback&) /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:613] → Heap use-after-free in @[nsFrameSelection::cycleCollection::TraverseNative(void*, nsCycleCollectionTraversalCallback&) /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:613]
Can be a bit of a PITA to reproduce, requiring some refreshing and/or letting it sit for a bit before eventually crashing. INFO: Last good revision: e7d613b3bcfe1e865378bfac37de64560d1234ec (2015-09-17) INFO: First bad revision: 11dc79e232110ba6de5179e46dfbda77b52a88c3 (2015-09-18) INFO: Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e7d613b3bcfe1e865378bfac37de64560d1234ec&tochange=11dc79e232110ba6de5179e46dfbda77b52a88c3 Too old to get a more narrow regression range via mozregression, unfortunately :(. Hopefully someone else sees a culprit in that range.
Group: core-security → layout-core-security
status-firefox54: --- → wontfix
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
tracking-firefox-esr52: --- → ?
Flags: in-qa-testsuite? → in-testsuite?
Version: unspecified → 43 Branch
I don't see anything suspicious in this range given the testcase... We may need to do bisect build in this range to know exactly what's happening.
track 55/56 for this uaf
tracking-firefox55: ? → +
tracking-firefox56: ? → +
Keywords: sec-high
Cornel, can you help narrow down the regression window? I cc-d you since I'm not sure who else from QE might be checking sec bugs.
Flags: needinfo?(cornel.ionce)
Adding Cristian in the loop, he will try to investigate this and provide the required information.
Flags: needinfo?(cornel.ionce) → needinfo?(cristian.comorasu)
Hi everyone. Although I`ve updated mozregression to the latest version I could not check the versions between last-good and first bad. However I could reproduce this issue on some earlier builds. The regression I found is: Last good: 20150913030204 First bad: 20150914030205 Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68718290640b0ca195344fa5cc4c55fbc260e19e&tochange=9ed17db42e3e46f1c712e4dffd62d54e915e0fac Crash reports: https://crash-stats.mozilla.com/report/index/1ef0fb50-6ffc-4129-9c4e-9e5b00170731 https://crash-stats.mozilla.com/report/index/d8c2bb91-16e4-4181-b31c-542b80170731 https://crash-stats.mozilla.com/report/index/7abdc21f-a3c0-4eb1-a0ff-8b6920170731 If there is a workaround with the mozregression issue or I missed something please let me know. Cheers!
Flags: needinfo?(cristian.comorasu)
It looks like this is a UAF of a document, so I'm guessing this is happening here: if (tmp->mShell && tmp->mShell->GetDocument() && nsCCUncollectableMarker::InGeneration(cb, tmp->mShell->GetDocument()-> GetMarkedCCGeneration())) { It looks like mShell is weak. I'm not sure how the lifetime of this is supposed to work.
Attached patch quick and dirty (obsolete) — — Splinter Review
Attachment #8892151 - Flags: review?(masayuki)
That is a quick and dirty patch which should be safe for branches too, and even if some other approach would be taken too, I definitely want to keep that patch. We really must unbind native anonymous elements editor has created.
Comment on attachment 8892151 [details] [diff] [review] quick and dirty Hmm... Editor may be destroyed without calling PreDestroy()???
Attachment #8892151 - Flags: review?(masayuki) → review+
Comment on attachment 8892151 [details] [diff] [review] quick and dirty [Security approval request comment] How easily could an exploit be constructed based on the patch? I'd say not very easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Commit message could be "Bug 1371657, ensure editing UI is hidden when disabling editing, r=masayuki" Which older supported branches are affected by this flaw? all Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch should apply everywhere How likely is this patch to cause regressions; how much testing does it need? Should be quite safe. We're normally calling the method in PreDestroy.
Attachment #8892151 - Flags: sec-approval?
Attachment #8892151 - Flags: approval-mozilla-esr52?
Attachment #8892151 - Flags: approval-mozilla-beta?
Comment on attachment 8892151 [details] [diff] [review] quick and dirty Sec-approval+ for trunk. Ritu will approve elsewhere.
Attachment #8892151 - Flags: sec-approval? → sec-approval+
Al pinged me about this one. Let's take this in 55.0 RC2 and ESR52.3
tracking-firefox-esr52: ? → 55+
Comment on attachment 8892151 [details] [diff] [review] quick and dirty Let's uplift to Beta56, Release55 (for RC), ESR52.3
Attachment #8892151 - Flags: approval-mozilla-release+
Attachment #8892151 - Flags: approval-mozilla-esr52?
Attachment #8892151 - Flags: approval-mozilla-esr52+
Attachment #8892151 - Flags: approval-mozilla-beta?
Attachment #8892151 - Flags: approval-mozilla-beta+
Whiteboard: [adv-main55+][adv-esr52.3+]
https://hg.mozilla.org/mozilla-central/rev/eb6f73d12931
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: layout-core-security → core-security-release
(In reply to Cristian Comorasu [:CristiComo] from comment #20) > Hi everyone, > I could still reproduce this crash using build 20170803100352 . ( > https://crash-stats.mozilla.com/report/index/846f6969-4a16-4251-9c2f- > befa70170803 ) > I tried again with build 20170806100257 and could still reproduce it: > - > https://crash-stats.mozilla.com/report/index/3847dd5e-cf9b-4a15-90a9- > 2d2050170807 > - > https://crash-stats.mozilla.com/report/index/5d9df8b4-9acb-4ba3-963a- > baa900170807 > - > https://crash-stats.mozilla.com/report/index/f9b6295d-143b-4382-a5c2- > 634a80170807 > > Am I missing something? I just tested with mc-asan-opt rev 20170807-47248637eafa and can confirm that this issue still reproduces.
Flags: needinfo?(jkratzer)
Flags: needinfo?(bugs)
Status: RESOLVED → REOPENED
status-firefox55: fixed → affected
status-firefox56: fixed → affected
status-firefox57: fixed → affected
status-firefox-esr52: fixed → affected
Resolution: FIXED → ---
Target Milestone: mozilla57 → ---
Whaat, odd, I'm getting now assertions I definitely didn't get earlier. Looking.
er, idiot me. The patch was for bug 1380284.
Flags: needinfo?(bugs)
Attachment #8892151 - Attachment is obsolete: true
Component: Layout: HTML Frames → Editor
Pretty unlikely we'll ship this in Fx55/ESR52.3 at this point.
status-firefox55: affected → wontfix
tracking-firefox-esr52: 55+ → ?
tracking-firefox-esr52: ? → 56+
Whiteboard: [adv-main55+][adv-esr52.3+]
Attached patch WIP (obsolete) — — Splinter Review
Comment on attachment 8895058 [details] [diff] [review] WIP The issue here is that nsContentUtils::AddScriptRunner is used, and while replaceChild is called, there is first a removal and then insertion and the runner runs only afterwards, yet selection listeners etc are notified, and editor clearly can't deal with the case when GetBody returns something bogus. I'm not quite happy with this. We had similar-ish issue in bug 767169 which added the scriptrunner usage. Couldn't think of anything simpler, and this doesn't regress bug 545775. But feel free to suggest something else :)
Attachment #8895058 - Flags: review?(masayuki)
Comment on attachment 8895058 [details] [diff] [review] WIP Although, I don't know if this approach can fix this bug, how about to stop caching mRootElement at first call of HTMLEditor::GetRootElement()? I think that mRootElement should be actual root element every time. So, in EditorBase::Init(), it should be initialized properly: https://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/editor/libeditor/EditorBase.cpp#257-258 # FYI: If it's an HTMLEditor, aRoot is nullptr Then, HTMLEditor::ContentRemoved() should clear mRootElement and HTMLEditor::DoContentInserted() set new root element. Finally, HTMLEditor::ResetRootElementAndEventTarget() should do nothing about mRootElement. If HTMLEditor::DoContentInserted() puts off to set mRootElement until HTMLEditor::ResetRootElementAndEventTarget(), then, the behavior should be same as your patch even without the new bool member.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #27) > Comment on attachment 8895058 [details] [diff] [review] > WIP > > Although, I don't know if this approach can fix this bug, how about to stop > caching mRootElement at first call of HTMLEditor::GetRootElement()? That would be probably a bit too slow. > Then, HTMLEditor::ContentRemoved() should clear mRootElement and > HTMLEditor::DoContentInserted() set new root element. Finally, > HTMLEditor::ResetRootElementAndEventTarget() should do nothing about > mRootElement. > > If HTMLEditor::DoContentInserted() puts off to set mRootElement until > HTMLEditor::ResetRootElementAndEventTarget(), then, the behavior should be > same as your patch even without the new bool member. Ah, this could work.
Except that calling GetRoot() before ResetRootElementAndEventTarget would reget the root element.
And that would lead to odd setup. We would have root element pointing to something, but there would be pending ResetRootElementAndEventTarget in script runner queue. I think I prefer my patch.
(In reply to Olli Pettay [:smaug] from comment #29) > Except that calling GetRoot() before ResetRootElementAndEventTarget would > reget the root element. I meant that when GetRootElement() shouldn't modify the mRootElement anymore. mRootElement should be set by: EditorBase::Init(), HTMLEditor::DoContentInserted() and HTMLEditor::ContentRemoved(). So, my idea makes mRootElement always exactly same (actual) root element any time. I.e., GetRootElement(), GetRoot() should just return mRootElement even when it's nullptr.
FYI: I'll be offline tomorrow due to Japanese national holiday.
oh, now I see what you mean. yeah, that should work the same was as my patch.
Hmm, what if body is the root element and then that it removed. html element should become the root. /me thinks
Attached patch v2 (obsolete) — — Splinter Review
This way then? I couldn't see reason for RemoveEventListeners/InstallEventListeners calls.
Attachment #8895964 - Flags: review?(masayuki)
Attachment #8895058 - Flags: review?(masayuki)
Comment on attachment 8895964 [details] [diff] [review] v2 > void >-HTMLEditor::ResetRootElementAndEventTarget() >+HTMLEditor::NotifyRootChanged() > { >+ if (!mRootElement) { >+ return; >+ } >+ > nsCOMPtr<nsIMutationObserver> kungFuDeathGrip(this); > >- // Need to remove the event listeners first because BeginningOfDocument >- // could set a new root (and event target is set by InstallEventListeners()) >- // and we won't be able to remove them from the old event target then. >- RemoveEventListeners(); Yeah, all event listeners are added to the document node. So, we don't need to reinstall them when its root is replaced. On the other hand, RemoveEventListener() commits composition: https://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/editor/libeditor/EditorBase.cpp#379,385,388 > EditorBase::RemoveEventListeners() > { > if (!IsInitialized() || !mEventListener) { > return; > } > reinterpret_cast<EditorEventListener*>(mEventListener.get())->Disconnect(); > if (mComposition) { > // Even if this is called, don't release mComposition because this is > // may be reused after reframing. > mComposition->EndHandlingComposition(this); This *might* be necessary but I should check this with actual IME and would fix this in a followup bug if necessary.
Attachment #8895964 - Flags: review?(masayuki) → review+
Ah, hrm, better to keep remove/installEventListeners then, for now.
Attached patch v3 (obsolete) — — Splinter Review
Better to reduce risk for regression in security bugs.
Attachment #8895964 - Attachment is obsolete: true
Attachment #8896409 - Flags: review?(masayuki)
Comment on attachment 8896409 [details] [diff] [review] v3 Oops, sorry for the delay. I marked the request email "as read" unexpectedly.
Attachment #8896409 - Flags: review?(masayuki) → review+
Comment on attachment 8896409 [details] [diff] [review] v3 [Security approval request comment] How easily could an exploit be constructed based on the patch? I'd say not easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Commit message could be -m "Bug 1371657, ensure we use the right root element in editor, r=masayuki" Which older supported branches are affected by this flaw? all If not all supported branches, which bug introduced the flaw? old code Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch seems to apply to beta at least. esr52 would need a new patch. Quite some merging issues there. How likely is this patch to cause regressions; how much testing does it need? This affects relatively rarely used replace-the-root-element case. Testing various text editing frameworks implemented in web technologies would be still good.
Attachment #8896409 - Flags: sec-approval?
Hmm, the patch doesn't work in esr52, I mean even after merging there is a crash.
Attachment #8896409 - Flags: sec-approval?
Attached patch editor_root_v4.diff (obsolete) — — Splinter Review
Sorry, still one. Need to mark root null in ContentRemoved because bug 1361268 and bug 1364180 aren't in esr52 and I want to keep trunk and esr52 patches as close to each others as possible.
Attachment #8896409 - Attachment is obsolete: true
Attachment #8898458 - Flags: review?(masayuki)
Comment on attachment 8898458 [details] [diff] [review] editor_root_v4.diff But could you also change: Make EditorBase::GetRoot() won't call GetRootElement(nsIDOMElement**) and make it inline? (i.e., moving the implementation into EditorBase.h?)
Attachment #8898458 - Flags: review?(masayuki) → review+
I'm trying to keep security fixes as small as possible, so such inlining should be done in a separate bug.
Comment on attachment 8898458 [details] [diff] [review] editor_root_v4.diff [Security approval request comment] How easily could an exploit be constructed based on the patch? I'd say not easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Commit message could be -m "Bug 1371657, ensure we use the right root element in editor, r=masayuki" Which older supported branches are affected by this flaw? all If not all supported branches, which bug introduced the flaw? old code Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch seems to apply to beta at least. there is a separate patch for esr52. How likely is this patch to cause regressions; how much testing does it need? This affects relatively rarely used replace-the-root-element case. Testing various text editing frameworks implemented in web technologies would be still good.
Attachment #8898458 - Flags: sec-approval?
sec-approval for checked on August 28, three weeks into the current cycle. We'll want beta and esr52 patches made and nominated as well to land afterwards.
Whiteboard: [checkin on 8/28]
Attachment #8898458 - Flags: sec-approval? → sec-approval+
Attachment #8895058 - Attachment is obsolete: true
This patch needs rebasing for trunk. Looks like Beta will need a rebased patch as well.
Flags: needinfo?(bugs)
Whiteboard: [checkin on 8/28]
This is for m-i (the patch did apply with some fuzz). Another one for beta coming.
Attachment #8898458 - Attachment is obsolete: true
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/08c89ceda404 Please nominate the Beta/ESR52 patches for approval when you get a chance.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox57: affected → fixed
Flags: needinfo?(bugs)
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8901943 [details] [diff] [review] editor_root_v5_beta.diff Approval Request Comment See comment 47 [String changes made/needed]: NA
Flags: needinfo?(bugs)
Attachment #8901943 - Flags: approval-mozilla-beta?
Comment on attachment 8898459 [details] [diff] [review] for esr52 Approval Request Comment See comment 47 [String changes made/needed]: NA
Attachment #8898459 - Flags: approval-mozilla-esr52?
Comment on attachment 8901943 [details] [diff] [review] editor_root_v5_beta.diff Fix for a sec-high issue, let's land this for the beta 8 build tomorrow morning.
Attachment #8901943 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8898459 [details] [diff] [review] for esr52 sec-high fix for editor, esr52.4+
Attachment #8898459 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main56+][adv-esr52.4+]
Flags: qe-verify-
Whiteboard: [adv-main56+][adv-esr52.4+] → [adv-main56+][adv-esr52.4+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: