Closed Bug 1505887 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: csectype-other, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main65+])

Attachments

(4 files, 1 obsolete file)

See the test-case in bug 1505875.
I'll move my needinfo from there to here :)
Flags: needinfo?(timdream)
This is also a problem for the XBL implementation, lol.
Attachment #9023756 - Attachment mime type: text/plain → text/html
Attachment #9023756 - Attachment description: Testcase. → Testcase (beware: crashes nightly with dom.ua_widget.enabled until bug 1505875 lands).
Summary: Can insert content inside a UA widget shadow root (ranges are exposed in window.getSelection()) → Can insert content inside a UA widget shadow root and XBL anon tree (ranges are exposed in window.getSelection())
Marking as s-s for now per conversation with Olli.
Group: core-security
And not related (per se) to UA widget after all, so clearing tim's ni?
Flags: needinfo?(timdream)
Don't want to loose track of this.
Flags: needinfo?(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.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Group: core-security → dom-core-security
Depends on: 1450219
Attached patch Properly work around bug 1450219 (obsolete) — Splinter Review
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.
Attachment #9025450 - Flags: review?(bugs)
(the xbl-specific test needs the same diff applied)
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.
Flags: needinfo?(dveditz)
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...
Attachment #9025603 - Flags: review?(bugs)
Comment on attachment 9025450 [details] [diff] [review]
Properly work around bug 1450219

I landed the fix for that, so this shouldn't matter anymore.
Attachment #9025450 - Attachment is obsolete: true
Attachment #9025450 - Flags: review?(bugs)
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
>
Attachment #9025603 - Flags: review?(bugs) → review+
crap. Sorry. I wasn't going to add any comment :) The comments there are just notes to myself.
r+
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)?
Flags: needinfo?(dveditz) → needinfo?(bzbarsky)
I don't think this is too risky, at first glance....
Flags: needinfo?(bzbarsky)
Blocks: 1509989
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/8221df0f4e50
https://hg.mozilla.org/mozilla-central/rev/ad4ea72ce4d4
https://hg.mozilla.org/mozilla-central/rev/07789ba4c3d7
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Depends on: 1510485
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.
Flags: needinfo?(emilio)
It sounds plausible, though note that XBL, using NAC, should already behave that way before hand.
Flags: needinfo?(emilio)
XBL behaves correctly from my tests. So yeah, given that this fixes XBL anonymous content too, perhaps they are not related.
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.
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main65+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.