Closed Bug 1466991 Opened 2 years ago Closed 2 years ago

Assertion failure: slowNode == node (These should always be in sync!), at src/dom/base/nsINode.cpp:317

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 62+ verified
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 + verified

People

(Reporter: tsmith, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-critical, testcase, Whiteboard: [post-critsmash-triage][adv-main62+][adv-esr60.2+])

Attachments

(5 files)

Attached file testcase.html
Reduced with m-c:
BuildID=20180604095036
SourceStamp=fa5724780fe76d6ccbbd08d978342a1db6a43d49

Note: This testcase may require a couple reloads to trigger the assertion.

Assertion failure: slowNode == node (These should always be in sync!), at src/dom/base/nsINode.cpp:317

#0 nsINode::SubtreeRoot() const src/dom/base/nsINode.cpp:313:3
#1 nsIContent::IsInAnonymousSubtree() const src/obj-firefox/dist/include/nsIContentInlines.h:189:3
#2 mozilla::DoesNotParticipateInAutoDirection(mozilla::dom::Element const*) src/dom/base/DirectionalityUtils.cpp:244:22
#3 mozilla::DoesNotAffectDirectionOfAncestors(mozilla::dom::Element const*) src/dom/base/DirectionalityUtils.cpp:255:11
#4 mozilla::SetDirOnBind(mozilla::dom::Element*, nsIContent*) src/dom/base/DirectionalityUtils.cpp:1026:13
#5 mozilla::dom::Element::BindToTree(nsIDocument*, nsIContent*, nsIContent*, bool) src/dom/base/Element.cpp:1678:5
#6 nsGenericHTMLElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, bool) src/dom/html/nsGenericHTMLElement.cpp:427:43
#7 nsINode::doInsertChildAt(nsIContent*, unsigned int, bool, nsAttrAndChildArray&) src/dom/base/nsINode.cpp:1358:14
#8 nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:2264:14
#9 mozilla::dom::NodeBinding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/NodeBinding.cpp:943:45
#10 bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3285:13
#11 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/vm/JSContext-inl.h:274:15
#12 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:471:16
#13 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:520:12
#14 Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3122:18
#15 js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:421:12
#16 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:493:15
#17 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:520:12
#18 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:539:10
#19 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2969:12
#20 mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:264:37
#21 void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
#22 mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) src/dom/events/JSEventHandler.cpp:214:12
#23 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1124:52
#24 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*) src/dom/events/EventListenerManager.cpp:1298:20
#25 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:406:17
#26 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:620:16
#27 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1086:9
#28 nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1166:7
#29 nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7161:21
#30 nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6954:7
#31 non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp
#32 nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1309:3
#33 nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:852:14
#34 nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:741:9
#35 nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:627:5
#36 non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp
#37 mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:629:28
#38 nsIDocument::DoUnblockOnload() src/dom/base/nsDocument.cpp:8343:18
#39 nsDocument::UnblockOnload(bool) src/dom/base/nsDocument.cpp:8265:9
#40 nsIDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:5223:3
#41 mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1216:13
#42 mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
#43 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1088:14
#44 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
#45 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#46 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#47 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#48 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
#49 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:893:22
#50 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:269:9
#51 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#52 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#53 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:719:34
#54 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
#55 main src/browser/app/nsBrowserApp.cpp:282:18
#56 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#57 _start (firefox+0x423544)
Flags: in-testsuite?
I wonder if this warns in older builds. There used to be NS_ASSERTION.
oh, the assertion fires because we have some debug code using SubtreeRoot.
Yeah, this is on beta
###!!! ASSERTION: Must have binding parent when in native anonymous subtree which is in document.
Native anonymous subtree which is not in document must have native anonymous root.: '!IsInNativeAnonymousSubtree() || GetBindingParent() || (!IsInUncomposedDoc() && static_cast<nsIContent*>(SubtreeRoot())->IsInNativeAnonymousSubtree())', file /home/smaug/mozilla/hg/mozilla-beta/ff_build/dist/include/nsIContentInlines.h, line 191
Group: dom-core-security
The issue is that element is created in NAC-XBL by JS, so it is wrapped before it is bound to NAC.

I'm pondering if we should just force one to create all the NAC elements in this kind of case by placing them under <content>
videocontrols and type=time/date at least seem to create elements in NAC-XBL.
bz suggested reparenting nodes when adding them to NAC tree.
What are the security implications here? Can you suggest a rating?
Flags: needinfo?(bugs)
sec-crit. We let web pages to touch native anonymous content.
Flags: needinfo?(bugs)
Keywords: sec-critical
Assignee: nobody → bzbarsky
Depends on: 1467870
We're going to add another consumer in the next changeset.
Attachment #8984641 - Flags: review?(bugs)
Attachment #8984641 - Flags: review?(bugs) → review+
Attachment #8984642 - Flags: review?(bugs) → review+
Comment on attachment 8984642 [details] [diff] [review]
part 2.  Reparent nodes when they start being in the XBL scope

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

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

Which older supported branches are affected by this flaw? Probably all of them.

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

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

How likely is this patch to cause regressions; how much testing does it need?  I'd say medium risk.  I'd definitely want several weeks of testing before ship.
Attachment #8984642 - Flags: sec-approval?
We're making RC builds for 61 next week. If we want several weeks of testing before ship, we need to not take this on 61. Do you think it makes sense to take this on trunk (62) and not backport it until later in the next cycle, Boris?
Flags: needinfo?(bzbarsky)
Yes.  I would not be terribly comfortable backporting this to 61 at this point.

For that matter, I would probably be OK fixing this on 62 and ESR and not touching 61 at all...
Flags: needinfo?(bzbarsky)
Comment on attachment 8984642 [details] [diff] [review]
part 2.  Reparent nodes when they start being in the XBL scope

sec-approval=dveditz for nightly (62). Not going to make 61.
Attachment #8984642 - Flags: sec-approval? → sec-approval+
Attachment #8984641 - Flags: approval-mozilla-esr60?
Comment on attachment 8984642 [details] [diff] [review]
part 2.  Reparent nodes when they start being in the XBL scope

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-crit
User impact if declined: Web pages might be able to run arbitrary code...
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky): Medium risk, but this was
   the safest option we came up with.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8984642 - Flags: approval-mozilla-esr60?
Attachment #8985538 - Flags: approval-mozilla-esr60?
Attachment #8984642 - Flags: approval-mozilla-esr60?
https://hg.mozilla.org/mozilla-central/rev/9c8ca5323b16
https://hg.mozilla.org/mozilla-central/rev/419f6555e669
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1471095
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Comment on attachment 8984641 [details] [diff] [review]
part 1.  Factor out ShouldUseXBLScope

Fixes a sec-crit. Approved for ESR 60.2.
Attachment #8984641 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #8985538 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Hey Boris, could you post some STR for manual testing?
Thank you!
Flags: needinfo?(bzbarsky)
You just load the attached testcase in a debug build.
Flags: needinfo?(bzbarsky)
Managed to get the crash on a build from 20180604095036.
Verified on Ubuntu 18.04 for 20180827144429 for RC and 20180828172101 ESR builds and can confirm the issue is no longer present.
Tested on the latest ESR/RC builds over win-10 and macOS10.13 and did not encounter any issues.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Component: DOM → DOM: Core & HTML
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.