See the test-case in bug 1505875.

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

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

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

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

Don't want to loose track of this.

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.

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.

(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.

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 on attachment 9025450 [details] [diff] [review] Properly work around bug 1450219 I landed the fix for that, so this shouldn't matter anymore.

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 >

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)?

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

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

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

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.

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

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.