1505887 - Can insert content inside a UA widget shadow root and XBL anon tree (ranges are exposed in window.getSelection())

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

See the test-case in bug 1505875.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I'll move my needinfo from there to here :)

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase (beware: crashes nightly with dom.ua_widget.enabled until bug 1505875 lands).

This is also a problem for the XBL implementation, lol.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Marking as s-s for now per conversation with Olli.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

And not related (per se) to UA widget after all, so clearing tim's ni?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Don't want to loose track of this.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So nsRange has a bunch of sanity-checks already that should be working but are not... I tried something that seems to work, probably needs feedback.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1505887 - Make UAWidgets chromeonly content.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1505887 - Make CanCallerAccess(nsINode*) know about chromeonly content.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Properly work around bug 1450219

I'll fix that in a bit, but I didn't want to block on that. See my last comment on that bug to see what's going on.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(the xbl-specific test needs the same diff applied)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Daniel, any chance I could get a sec-rating for this (and based on that, if needed, sec-approval for the patches?)

It affects all release channels, and it's a bug we've had since forever. It allows to inject random content in our form controls, which is not great, though I haven't tried to come up with an exploit that actually could cause real badness, there's a chance that some of the XBL / JS implemented form controls and such could get confused and do bad stuff if you inject the right content.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Fix FindChromeAccessOnlySubtreeOwner so that we handle UA widget being ChromeOnlyAccess root.

A single video controls test crashed without this, while dereferencing a null anonOwnerRelated in:
    
  https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/dom/base/FragmentOrElement.cpp#964
    
I think this is the right fix for it, but the code that uses this is kind of complex... I can also just add a null-check there or something, but...

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9025450 [details] [diff] [review]
Properly work around bug 1450219

I landed the fix for that, so this shouldn't matter anymore.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9025603 [details] [diff] [review]
Fix FindChromeAccessOnlySubtreeOwner so that we handle UA widget being ChromeOnlyAccess root.

>-static nsIContent*
>-FindChromeAccessOnlySubtreeOwner(nsIContent* aContent)
>-{
>-  if (aContent->ChromeOnlyAccess()) {
>-    bool chromeAccessOnly = false;
>-    while (aContent && !chromeAccessOnly) {
>-      chromeAccessOnly = aContent->IsRootOfChromeAccessOnlySubtree();
>-      aContent = aContent->GetParent();
>-    }
>+static nsINode*
>+FindChromeAccessOnlySubtreeOwner(nsINode* aNode)
>+{
>+  if (!aNode->ChromeOnlyAccess()) {
>+    return aNode;
>+  }
>+
>+  while (aNode && !aNode->IsRootOfChromeAccessOnlySubtree()) {
>+    aNode = aNode->GetParentNode();
>   }
So here aNode is either null or root-of-chrome-access-only-subtree

>-  return aContent;
>+
>+  return aNode ? aNode->GetParentOrHostNode() : nullptr;
Here, if it is not null, we don't return that node, but i


> }
> 
> already_AddRefed<nsINode>
> FindChromeAccessOnlySubtreeOwner(EventTarget* aTarget)
> {
>   nsCOMPtr<nsINode> node = do_QueryInterface(aTarget);
>   if (!node || !node->ChromeOnlyAccess()) {
>     return node.forget();
>   }
> 
>-  if (!node->IsContent()) {
>-    return nullptr;
>-  }
>-
>-  node = FindChromeAccessOnlySubtreeOwner(node->AsContent());
>+  node = FindChromeAccessOnlySubtreeOwner(node);
>   return node.forget();
> }
> 
> void
> nsIContent::GetEventTargetParent(EventChainPreVisitor& aVisitor)
> {
>   //FIXME! Document how this event retargeting works, Bug 329124.
>   aVisitor.mCanHandle = true;
>@@ -946,19 +943,19 @@ nsIContent::GetEventTargetParent(EventChainPreVisitor& aVisitor)
>       // target is descendant of an element which is anonymous for events,
>       // we may want to stop event propagation.
>       // If this is the original target, aVisitor.mRelatedTargetIsInAnon
>       // must be updated.
>       if (isAnonForEvents || aVisitor.mRelatedTargetIsInAnon ||
>           (aVisitor.mEvent->mOriginalTarget == this &&
>            (aVisitor.mRelatedTargetIsInAnon =
>             relatedTarget->ChromeOnlyAccess()))) {
>-        nsIContent* anonOwner = FindChromeAccessOnlySubtreeOwner(this);
>+        nsINode* anonOwner = FindChromeAccessOnlySubtreeOwner(this);
>         if (anonOwner) {
>-          nsIContent* anonOwnerRelated =
>+          nsINode* anonOwnerRelated =
>             FindChromeAccessOnlySubtreeOwner(relatedTarget);
>           if (anonOwnerRelated) {
>             // Note, anonOwnerRelated may still be inside some other
>             // native anonymous subtree. The case where anonOwner is still
>             // inside native anonymous subtree will be handled when event
>             // propagates up in the DOM tree.
>             while (anonOwner != anonOwnerRelated &&
>                    anonOwnerRelated->ChromeOnlyAccess()) {
>diff --git a/dom/base/nsIContent.h b/dom/base/nsIContent.h
>index d8818ce80c55..497fe518e46a 100644
>--- a/dom/base/nsIContent.h
>+++ b/dom/base/nsIContent.h
>@@ -173,36 +173,16 @@ public:
>    *
>    * @note calling this method with eAllButXBL will return children that are
>    *  also in the eAllButXBL and eAllChildren child lists of other descendants
>    *  of this node in the tree, but those other nodes cannot be reached from the
>    *  eAllButXBL child list.
>    */
>   virtual already_AddRefed<nsINodeList> GetChildren(uint32_t aFilter) = 0;
> 
>-  /**
>-   * Get whether this content is C++-generated anonymous content
>-   * @see nsIAnonymousContentCreator
>-   * @return whether this content is anonymous
>-   */
>-  bool IsRootOfNativeAnonymousSubtree() const
>-  {
>-    NS_ASSERTION(!HasFlag(NODE_IS_NATIVE_ANONYMOUS_ROOT) ||
>-                 (HasFlag(NODE_IS_ANONYMOUS_ROOT) &&
>-                  HasFlag(NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE)),
>-                 "Some flags seem to be missing!");
>-    return HasFlag(NODE_IS_NATIVE_ANONYMOUS_ROOT);
>-  }
>-
>-  bool IsRootOfChromeAccessOnlySubtree() const
>-  {
>-    return HasFlag(NODE_IS_NATIVE_ANONYMOUS_ROOT |
>-                   NODE_IS_ROOT_OF_CHROME_ONLY_ACCESS);
>-  }
>-
>   /**
>    * Makes this content anonymous
>    * @see nsIAnonymousContentCreator
>    */
>   void SetIsNativeAnonymousRoot()
>   {
>     SetFlags(NODE_IS_ANONYMOUS_ROOT | NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE |
>              NODE_IS_NATIVE_ANONYMOUS_ROOT);
>diff --git a/dom/base/nsINode.h b/dom/base/nsINode.h
>index 3a77c0c603aa..14c8abd03a80 100644
>--- a/dom/base/nsINode.h
>+++ b/dom/base/nsINode.h
>@@ -1250,16 +1250,36 @@ public:
>     return HasFlag(NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE | NODE_CHROME_ONLY_ACCESS);
>   }
> 
>   bool IsInShadowTree() const
>   {
>     return HasFlag(NODE_IS_IN_SHADOW_TREE);
>   }
> 
>+  /**
>+   * Get whether this node is C++-generated anonymous content
>+   * @see nsIAnonymousContentCreator
>+   * @return whether this content is anonymous
>+   */
>+  bool IsRootOfNativeAnonymousSubtree() const
>+  {
>+    NS_ASSERTION(!HasFlag(NODE_IS_NATIVE_ANONYMOUS_ROOT) ||
>+                 (HasFlag(NODE_IS_ANONYMOUS_ROOT) &&
>+                  HasFlag(NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE)),
>+                 "Some flags seem to be missing!");
>+    return HasFlag(NODE_IS_NATIVE_ANONYMOUS_ROOT);
>+  }
>+
>+  bool IsRootOfChromeAccessOnlySubtree() const
>+  {
>+    return HasFlag(NODE_IS_NATIVE_ANONYMOUS_ROOT |
>+                   NODE_IS_ROOT_OF_CHROME_ONLY_ACCESS);
>+  }
>+
>   /**
>    * Returns true if |this| node is the common ancestor of the start/end
>    * nodes of a Range in a Selection or a descendant of such a common ancestor.
>    * This node is definitely not selected when |false| is returned, but it may
>    * or may not be selected when |true| is returned.
>    */
>   bool IsSelectionDescendant() const
>   {
>-- 
>2.19.1
>

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

crap. Sorry. I wasn't going to add any comment :) The comments there are just notes to myself.
r+

Comment 16

[Daniel Veditz [:dveditz]]

makes me nervous, but short of a known exploitable use calling it sec-moderate.

Should we try to uplift this to Fx64 just in case (or does that have too much risk of breakage)?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I don't think this is too risky, at first glance....

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15da6e919d804d770219f720fb93b7c2d42e5f10
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9afff4ff11f683d9a5e46ae92d80d6e9e7add3
https://hg.mozilla.org/integration/mozilla-inbound/rev/490a99122a7fc04ad0ec6bf9e32036c36b92d631
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6c022e9fe127bd0d32e25e3206f79e3d0d954c

Backed out for failing crashtests on tests/layout/style/crashtests/1509989.html:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc298299ebadd4b20eb58ea1ee6404b416eed422

And relanded without the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8221df0f4e50bbdebe7cd65c1b3adf799a1aa6a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad4ea72ce4d4f7c9150575f6173fa0fbb05e967c
https://hg.mozilla.org/integration/mozilla-inbound/rev/07789ba4c3d73b0318cbd9fa10c8e7f110565801

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8221df0f4e50
https://hg.mozilla.org/mozilla-central/rev/ad4ea72ce4d4
https://hg.mozilla.org/mozilla-central/rev/07789ba4c3d7

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Could this bug be the cause of bug 1513600 comment 4? I already have a patch to workaround it but perhaps we want to hide the |relatedTarget| property behind [Func="IsNotUAWidget"] if is not supported there.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

It sounds plausible, though note that XBL, using NAC, should already behave that way before hand.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

XBL behaves correctly from my tests. So yeah, given that this fixes XBL anonymous content too, perhaps they are not related.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Calling this wontfix for esr60 since this is sec-moderate seems riskier than we'd really want to take there at this point. Feel free to argue in favor of uplift if you feel strongly otherwise, though.