1467870 - Sort out what should happen with Xray expandos when a node is adopted into the Xray compartment

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Consider this testcase:

1) Start Firefox
2) Ensure the "devtools.chrome.enabled" preference is true.
3) Load "about:robots"
4) Open the scratchpad devtool in "chrome" mode (tools > web developer > scratchpad, and then in the menu do environment > chrome).
5) Evaluate: 

  var div = content.document.createElement("div");
  div.textContent= "HELLO"
  div.foo = 5;
  document.adoptNode(div);
  alert(div.foo)

This alerts undefined right now; the expando got lost.

That happens because we no longer have an Xray to the div; we now have a new reflector.  The props on the old reflector got copied.  And Xray expando chains got cloned, but we're not accessing it via Xrays anymore so it doesn't matter that the Xray expando is hanging off it.

For an additional wrinkle, if we were "adopting" into a global that does the exclusive expando thing, we would fail this assert at the top of XrayTraits::attachExpandoObject:

    MOZ_ASSERT_IF(exclusiveWrapper, !js::IsObjectInContextCompartment(exclusiveWrapper, cx));

I think we have a few principled things we could do:

1)  Always silently lose the expandos.  That is, in XrayTraits::cloneExpandoChain detect when dst would not be getting Xrays from the compartment(s) an expando object represents and just drop that expando object from the chain.

2)  Copy the expandos to the reflector.  Again, detect when dst is not getting Xrays and JS_CopyPropertiesFrom(cx, dst, oldHead) while not creating a corresponding expando chain item.

Thoughts?

Security-sensitive because of the assertion failure, but I'm not sure we actually need to keep this hidden.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> we would fail this assert at the top of XrayTraits::attachExpandoObject

And crash in an opt build too, which is extra-exciting.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So for bug 1466991 I need to either copy the expandos or make sure our XBL stops using them.  The former seems simpler...

Comment 3

[Boris Zbarsky [:bzbarsky]]

> and then in the menu do environment > chrome

environment > Browser

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: When cloning Xray expando chains for an adopt into the Xray compartment, don't lose expandos

This does mean that if the node is adopted back into the original document the
expandos would end up on the reflector at that point.  I think that's OK;
people shouldn't be adopting things in that direction.  Futhermore, if they
adopt, then set expandos, then adopt back they already get this behaviorl

So if we decide we don't want that, we should probably just avoid copying own
props when reparenting into a global whose principals don't subsume the old
global.

Comment 5

[Kris Maglione [:kmag]]

Do we actually do this anywhere? If not, the simplest solution would probably be to just prevent adopting cross-origin nodes into any compartment with x-rays enabled.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Do we actually do this anywhere?

I'm doing it in my fix for bug 1466991.  That's how I ran into this.

The context there is an XBL binding attached to native anon content that does this, more or less.

   var node = document.createElement("span");
   this.appendChild(node);

"node" start off as an Xray to a reflector that lives in the document's global.  Then we append that node into our anon content.

At this point, the reflector is still in the document's global.  But if it gets gced and then we poke the node from script again it would be in the XBL global (because we're in an anon content subtree now).  That's moderately broken and the right solution, imo, is to move the reflector to the XBL global at the point when the anon content state changes.  That's what the patches in bug 1467870 do.  It so happens that the binding involved sets expandos on "node" before the appendChild call.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8984640 [details] [diff] [review]
When cloning Xray expando chains for an adopt into the Xray compartment, don't lose expandos

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not very easily at all.  I don't know offhand how web code could trigger this situation.

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? All of them, I expect.  That said, this patch likely does not apply to things older than Firefox 56, because it's on top of bug 1355109.

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 be pretty simple to backport.

How likely is this patch to cause regressions; how much testing does it need?  I think this is medium-risk.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 8984640 [details] [diff] [review]
When cloning Xray expando chains for an adopt into the Xray compartment, don't lose expandos

sec-approval=dveditz

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8984640 [details] [diff] [review]
When cloning Xray expando chains for an adopt into the Xray compartment, don't lose expandos

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It's blocking a sec-crit bug.
User impact if declined: Can't land fix for bug 1466991
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky): Risk is medium.  The alternative it to not take it.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Backport to esr60

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Backport to esr60

Comment 12

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9496b052a5d18a752ad186fa3c28dd182342f1b7

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9496b052a5d1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8985536 [details] [diff] [review]
Backport to esr60

Blocks a sec-crit. Approved for ESR 60.2.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/c3a6baa14bfc