The attached testcase causes a heap uaf in m-c rev 20171010-77a4c52e9987. It also crashes in 57 rev 20171010-ed53ec40faf5. ==24073==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e00005c5a8 at pc 0x7f8552a84632 bp 0x7fff0d148960 sp 0x7fff0d148958 READ of size 8 at 0x60e00005c5a8 thread T0 (file:// Content) #0 0x7f8552a84631 in NonMappedAttrCount /builds/worker/workspace/build/src/dom/base/nsAttrAndChildArray.cpp:717:8 #1 0x7f8552a84631 in nsAttrAndChildArray::AttrCount() const /builds/worker/workspace/build/src/dom/base/nsAttrAndChildArray.cpp:297 #2 0x7f85559d68b8 in nsSVGElement::UpdateContentDeclarationBlock(mozilla::StyleBackendType) /builds/worker/workspace/build/src/dom/svg/nsSVGElement.cpp:1318:42 #3 0x7f8552b7a1e6 in nsDocument::ResolveScheduledSVGPresAttrs() /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:6481:10 #4 0x7f85568b0230 in ResolveMappedAttrDeclarationBlocks /builds/worker/workspace/build/src/layout/style/ServoStyleSet.cpp:405:29 #5 0x7f85568b0230 in mozilla::ServoStyleSet::PreTraverseSync() /builds/worker/workspace/build/src/layout/style/ServoStyleSet.cpp:416 #6 0x7f85568b425b in PreTraverse /builds/worker/workspace/build/src/layout/style/ServoStyleSet.cpp:459:3 #7 0x7f85568b425b in mozilla::ServoStyleSet::StyleDocument(mozilla::ServoTraversalFlags) /builds/worker/workspace/build/src/layout/style/ServoStyleSet.cpp:954 #8 0x7f8556c6c429 in mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1126:20 #9 0x7f8556c2c4a0 in ProcessPendingRestyles /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1213:3 #10 0x7f8556c2c4a0 in ProcessPendingRestyles /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RestyleManagerInlines.h:44 #11 0x7f8556c2c4a0 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4146 #12 0x7f8556ba11f0 in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:566:5 #13 0x7f8556ba11f0 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1926 #14 0x7f8556baef5b in TickDriver /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:337:13 #15 0x7f8556baef5b in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:307 #16 0x7f8556baec56 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:329:5 #17 0x7f8556bb11ab in RunRefreshDrivers /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:770:5 #18 0x7f8556bb11ab in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:683 #19 0x7f8556bb0db6 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:584:9 #20 0x7f85573e4612 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /builds/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:67:16 #21 0x7f8551003e21 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:155:20 #22 0x7f8550c69d6e in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1755:28 #23 0x7f8550bb6799 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2119:25 #24 0x7f8550bb37af in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2049:17 #25 0x7f8550bb4ee4 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1895:5 #26 0x7f8550bb5538 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1928:15 #27 0x7f854fdd1116 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1037:14 #28 0x7f854fdeaea8 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:524:10 #29 0x7f8550bbe411 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21 #30 0x7f8550b206db in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10 #31 0x7f8550b206db in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319 #32 0x7f8550b206db in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299 #33 0x7f85564c4c1f in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27 #34 0x7f855a807827 in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:877:22 #35 0x7f8550b206db in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10 #36 0x7f8550b206db in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319 #37 0x7f8550b206db in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299 #38 0x7f855a8071da in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:703:34 #39 0x4ec20e in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30 #40 0x4ec20e in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280 #41 0x7f856c831f69 in __libc_start_main (/usr/lib/libc.so.6+0x20f69) #42 0x41daf8 in _start (/home/truber/builds/m-c-1507626796-asan-opt/firefox+0x41daf8) 0x60e00005c5a8 is located 104 bytes inside of 160-byte region [0x60e00005c540,0x60e00005c5e0) freed by thread T0 (file:// Content) here: #0 0x4bc02b in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:47:3 #1 0x7f854fc68dc7 in SnowWhiteKiller::~SnowWhiteKiller() /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2729:25 #2 0x7f854fc7046b in FreeSnowWhite /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2917:3 #3 0x7f854fc7046b in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3925 #4 0x7f854fc6f983 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3746:9 #5 0x7f854fc737d0 in nsCycleCollector_collect(nsICycleCollectorListener*) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:4315:21 #6 0x7f8552c755ed in nsJSContext::CycleCollectNow(nsICycleCollectorListener*) /builds/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:1480:3 #7 0x7f85527bb52b in nsDOMWindowUtils::CycleCollect(nsICycleCollectorListener*) /builds/worker/workspace/build/src/dom/base/nsDOMWindowUtils.cpp:1434:3 #8 0x7f854fdfa151 in NS_InvokeByIndex /builds/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:129 #9 0x7f85515c8a20 in Invoke /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1996:12 #10 0x7f85515c8a20 in Call /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1315 #11 0x7f85515c8a20 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1282 #12 0x7f85515cf7af in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:929:12 #13 0x7f855aab6825 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #14 0x7f855aab6825 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495 #15 0x7f855aaa04df in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:546:12 #16 0x7f855aaa04df in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3085 #17 0x7f855aa87649 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:435:12 #18 0x7f855aab68fc in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:513:15 #19 0x7f855aab7252 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:559:10 #20 0x7f855b4ed403 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2916:12 #21 0x7f85514e6b4b in xpc::FunctionForwarder(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/xpconnect/src/ExportHelpers.cpp:315:18 #22 0x7f855aab6825 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #23 0x7f855aab6825 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495 #24 0x7f855aaa04df in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:546:12 #25 0x7f855aaa04df in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3085 #26 0x7f855aa87649 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:435:12 #27 0x7f855aab68fc in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:513:15 #28 0x7f855aab7252 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:559:10 #29 0x7f855b4ef28b 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:2975:12 #30 0x7f85540c0485 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:260:37 #31 0x7f8554a9cbc1 in Call<nsISupports *> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:362:12 #32 0x7f8554a9cbc1 in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /builds/worker/workspace/build/src/dom/events/JSEventHandler.cpp:215 #33 0x7f8554a65576 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1118:51 #34 0x7f8554a675a3 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1295:20 #35 0x7f8554a47201 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:462:16 #36 0x7f8554a4a6d2 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:822:9 #37 0x7f8556d085ae in nsDocumentViewer::LoadComplete(nsresult) /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:1065:7 previously allocated by thread T0 (file:// Content) here: #0 0x4bc37c in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3 #1 0x4ed78d in moz_xmalloc /builds/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:84:17 #2 0x7f85559c9279 in operator new /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:206:12 #3 0x7f85559c9279 in NS_NewSVGElement(mozilla::dom::Element**, already_AddRefed<mozilla::dom::NodeInfo>&&) /builds/worker/workspace/build/src/dom/svg/nsSVGElement.cpp:75 #4 0x7f855591e097 in NS_NewSVGUnknownElement(nsIContent**, already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser) /builds/worker/workspace/build/src/dom/svg/SVGElementFactory.cpp:138:17 #5 0x7f8551baf0f8 in nsHtml5TreeOperation::CreateSVGElement(nsAtom*, nsHtml5HtmlAttributes*, mozilla::dom::FromParser, nsNodeInfoManager*, nsHtml5DocumentBuilder*, nsresult (*)(nsIContent**, already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser)) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOperation.cpp:471:7 #6 0x7f8551bbbf72 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOperation.cpp:832:17 #7 0x7f8551bb8732 in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:460:27 #8 0x7f8551bc27bb in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:130:20 #9 0x7f854fdabb60 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:396:25 #10 0x7f854fdd1116 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1037:14 #11 0x7f854fdeaea8 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:524:10 #12 0x7f8550bbe411 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21 #13 0x7f8550b206db in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10 #14 0x7f8550b206db in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319 #15 0x7f8550b206db in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299 #16 0x7f85564c4c1f in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27 #17 0x7f855a807827 in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:877:22 #18 0x7f8550b206db in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10 #19 0x7f8550b206db in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319 #20 0x7f8550b206db in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299 #21 0x7f855a8071da in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:703:34 #22 0x4ec20e in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30 #23 0x4ec20e in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280 #24 0x7f856c831f69 in __libc_start_main (/usr/lib/libc.so.6+0x20f69) SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/dom/base/nsAttrAndChildArray.cpp:717:8 in NonMappedAttrCount Shadow bytes around the buggy address: 0x0c1c80003860: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa 0x0c1c80003870: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c1c80003880: fd fd fd fa fa fa fa fa fa fa fa fa 00 00 00 00 0x0c1c80003890: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c1c800038a0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd =>0x0c1c800038b0: fd fd fd fd fd[fd]fd fd fd fd fd fd fa fa fa fa 0x0c1c800038c0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00 0x0c1c800038d0: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa 0x0c1c800038e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c1c800038f0: 00 00 00 00 fa fa fa fa fa fa fa fa 00 00 00 00 0x0c1c80003900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 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 ==24073==ABORTING

This bisects back to when we first started building Stylo by default, unfortunately. On debug builds, I see an assertion too, but I'm not sure if it's related or not. ASSERTION: we already have a content declaration block: '!mContentDeclarationBlock', file /builds/worker/workspace/build/src/dom/svg/nsSVGElement.cpp, line 1314

Note: to install the fuzzer extension (required by the testcase), you need to build the XPI locally (by cloning https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension and running "make" in that directory) and then setting these prefs in the Firefox Nightly profile that you're using for testing: xpinstall.signatures.required = false extensions.legacy.enabled = true Then it should let you install the XPI.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1) > On debug builds, I see an assertion too, but I'm not sure if > it's related or not. It is related. In my (non-ASAN-enabled) debug build, we're failing that assertion because this nsSVGElement's mContentDeclarationBlock is non-null -- and it's specifically 0xe5e5e5e5e5e5e5e5, which I think is indicative that this object is deallocated, or something like that. So, this is the same sort of UAF. The problem here is that the element is getting cycle collected, BUT we retain a raw pointer to it (after it's been deleted!!!) in this "mLazySVGPresElements" structure (which seems to be stylo-specific). And then later on (after the element has been deleted), we call ResolveScheduledSVGPresAttrs, which traverses the contents of mLazySVGPresElements, including this pointer to a previously-deleted element. And we fail the assertion (because we have this bogus pointer value) and then crash from UAF. So presumably we need to ensure that the element gets removed from mLazySVGPresElements when it's cycle-collected/deleted.

So, this is a regression from https://hg.mozilla.org/mozilla-central/rev/33e225044544 (Bug 1329093), which is where we added this mLazySVGPresElements stuff. But stylo has to be enabled for this codepath to be active, hence the later de-facto regression range in comment 1.

(In reply to Daniel Holbert [:dholbert] from comment #3) > So presumably we need to ensure that the element gets removed from > mLazySVGPresElements when it's cycle-collected/deleted. It looks like we do actually try to do this (~nsSVGElement calls OwnerDoc()->UnscheduleSVGForPresAttrEvaluation(this)). BUT, the problem is that we're calling it on **a different document** from the document that we called ScheduleSVGForPresAttrEvaluation(this) on earlier. So, Unschedule silently fails to remove anything, and the other document is left with a raw pointer to this element still present in its mLazySVGPresElements structure, which gets flushed later on. I think nsSVGElement::NodeInfoChanged() needs to unregister the element from the old document, before registering it with the new document...

I guess nsSVGElement::NodeInfoChanged() does try to unregister from the old document, but then we change the document *again* afterwards, without calling NodeInfoChanged. Specifically: we end up in nsNodeUtils::CloneAndAdopt, which has two relevant places where it changes the mNodeInfo (and the document): - Line 529, where we call NodeInfoChanged afterwards: https://dxr.mozilla.org/mozilla-central/rev/2ed5e7fbf39e949693d8a7455d6313b14a7aeaf6/dom/base/nsNodeUtils.cpp#529-531 - Line 618, where we DO NOT call NodeInfoChanged afterwards: https://dxr.mozilla.org/mozilla-central/rev/2ed5e7fbf39e949693d8a7455d6313b14a7aeaf6/dom/base/nsNodeUtils.cpp#618 We end up changing the document (and unregistering/registering) via the first spot, and then changing the document again (and not doing any unregister/register dance!!!) via the second spot. So we're left with a stale registration in the wrong document.

Ugh. I think we should be calling NodeInfoChanged in that second block! This is basically similar to bug 1317409.

Oh, and this probably affects various old branches too. For example, nsMappedAttributeElement::NodeInfoChanged goes back to Firefox 55 and can leave stale pointers lying around if not called...

MozReview-Commit-ID: 71k1jv8thFA

Comment on attachment 8917238 [details] [diff] [review] Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node [Security approval request comment] How easily could an exploit be constructed based on the patch? Not terribly easily. This is an error codepath and for starters you have to figure out how to trigger that error. 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? Probably all of them. If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This should be pretty simple to backport. How likely is this patch to cause regressions; how much testing does it need? I don't think this needs very much testing.

Note that I have _not_ had a chance to test that patch on the actual testcase yet. I don't have an ASAN build around right this second.

Comment on attachment 8917238 [details] [diff] [review] Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node Review of attachment 8917238 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, I guess this was really a problem with the patch in bug 568188 (but might not have problematic at that point, depending on the NodeInfoChanged implementations).

[Tracking Requested - why for this release]: updating esr status per comments 8/10

Comment on attachment 8917238 [details] [diff] [review] Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node sec-approval+. Let's backport this to beta. Is it not clear if ESR52 is affected?

(In reply to Peter Van der Beken [:peterv] from comment #12) > Hmm, I guess this was really a problem with the patch in bug 568188 (but > might not have problematic at that point, depending on the NodeInfoChanged > implementations). Looking at other NodeInfoChanged impls, I think there's one other problematic one which does predate stylo/57: - nsMappedAttributeElement::NodeInfoChanged() calls... nsHTMLStyleSheet* sheet = OwnerDoc()->GetAttributeStyleSheet(); mAttrsAndChildren.SetMappedAttrStyleSheet(sheet); https://dxr.mozilla.org/mozilla-central/rev/a0488ecc201c04f2617e7b02f039344e8fbf0d9a/dom/base/nsMappedAttributeElement.cpp#42-45 That "SetMappedAttrStyleSheet(sheet)" call causes the element to store a raw pointer to the "new" document's stylesheet, via this code: https://dxr.mozilla.org/mozilla-central/rev/a0488ecc201c04f2617e7b02f039344e8fbf0d9a/dom/base/nsMappedAttributes.cpp#254 So -- then, if we switch the element back to the original doc (without calling NodeInfoChanged) as described in comment 6, then I expect the "new" doc could be destroyed (along with its nsHTMLStyleSheet), while the nsMappedAttributeElement lives on indefinitely with a raw pointer to the now-destroyed nsHTMLStyleSheet. So, it seems like that could trigger a UAF. This version of the problem goes back to Firefox 55, it looks like, since that's where this nsMappedAttributeElement::NodeInfoChanged() impl was added (in bug 1352898). (In reply to Al Billings [:abillings] from comment #14) > Is it not clear if ESR52 is affected? I *think* 52 is unaffected -- I'm not aware of any badness that this can cause that'd go back as far as 52.

Yeah, ESR52 only has two nonempty NodeInfoChanged implementations: https://dxr.mozilla.org/mozilla-esr52/search?q=NodeInfoChanged&redirect=true They are: https://dxr.mozilla.org/mozilla-esr52/rev/a0c13b815564700ddba946adf24b1462e761d5b3/dom/html/nsGenericHTMLElement.cpp#2847 https://dxr.mozilla.org/mozilla-esr52/rev/a0c13b815564700ddba946adf24b1462e761d5b3/dom/html/HTMLImageElement.cpp#715 ...which don't look problematic from this bug's perspective. (We might still have incorrect behavior there, from missing a call into those functions, but they're not saving/working-with raw pointers to data that's owned by something else. So they don't seem like they could trigger a UAF.)

I agree; 52 is not affected. The two implementations there are idempotent, and we just called it and haven't had a chance to do anything since, really, so even the behavior is correct on 52.

Alright, I have now tested this in an ASAN build and verified that the patch fixes the testcase.

(In reply to Al Billings [:abillings] from comment #14) > sec-approval+. Let's backport this to beta. (IIRC, bz still needs to request beta approval separately -- tagging him for that -- but let me know if that's somehow already implicitly granted by abillings' comment here^^ .)

I'll give Beta approval or Ritu can do so.

Comment on attachment 8917238 [details] [diff] [review] Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node Approval Request Comment [Feature/Bug causing the regression]: Longstanding problem exacerbated by servo [User impact if declined]: Likely-exploitable crashes [Is this code covered by automated tests?]: No, because of the security status. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: Probably good to double-check that the original testcase is fixed. See comment 0 and comment 2. You probably need an ASAN build and the fuzzing extension. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: I don't think so. [Why is the change risky/not risky?]: This is fixing a pretty clear bug in the adoption code, and doing that in a pretty simple way. [String changes made/needed]: None.

Comment on attachment 8917238 [details] [diff] [review] Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node Sec-crit, Beta57+

I followed the steps from comment 2 and description, however the fuzzPriv addon says it is not compatible with Fx 57 and Fx 58. Can you tell me a workaround? Thank you!

I think one or both of the prefs from Comment 2 don't have any effect in release builds -- only in Nightly builds (and maybe DevEdition) So, I'm not sure if it's possible to install the fuzzPriv addon in release/beta builds... maybe truber knows? (not sure what best-practices are for verifying uplifted fuzzer-XPI-dependent bugs, in light of this...) I can imagine that we could verify using a custom build from the release repo (with nightly branding turned on or something), but that's probably not ideal because it takes a bunch of extra work to generate something testable, and it'd also be testing a configuration that's different from the one that we ship.

I think the way to do this would be to use a fuzzing build. The easiest way to get the build is to use fuzzfetch[1] or grab it from TC[2]. Once you have your release fuzzing build then replace the calls to the fuzzPriv functions in the testcase with the correct fuzzing build functions (see https://bugzilla.mozilla.org/show_bug.cgi?id=1322400#c9) I hope that does it. [1] https://github.com/MozillaSecurity/fuzzfetch [2] https://tools.taskcluster.net/index/artifacts/gecko.v2.mozilla-release.latest.firefox/linux64-fuzzing-asan-opt

Yep, fuzzing build is the way to go. Note that you require a fuzzing build as well as a pref: fuzzing.enabled=true I'm working on a WebExtension which would shim these functions for fuzzing builds and add some of the other extension capabilities which are still possible in a WE.

I have managed to reproduce the issue mentioned in comment 0 using Firefox 58.0a1 (BuildId:20171010055346). This issue is no longer reproducible using Firefox 57.0.3 (BuildId:20171217111436), 58.0b12 (BuildId:20171215145727) and 59.0a1 (BuildId:20171217094207) ASAN fuzzing builds on Ubuntu 16.04 64bit.