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

RESOLVED FIXED in Firefox -esr60

Status

()

defect
RESOLVED FIXED
Last year
13 days ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({sec-audit})

unspecified
mozilla62
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52- wontfix, firefox-esr6062+ fixed, firefox60 wontfix, firefox61- wontfix, firefox62+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main62-][adv-esr60.2-])

Attachments

(2 attachments, 1 obsolete attachment)

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.
> we would fail this assert at the top of XrayTraits::attachExpandoObject

And crash in an opt build too, which is extra-exciting.
So for bug 1466991 I need to either copy the expandos or make sure our XBL stops using them.  The former seems simpler...
Assignee: nobody → bzbarsky
> and then in the menu do environment > chrome

environment > Browser
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.
Attachment #8984640 - Flags: review?(mrbkap)
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.
> 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.
Attachment #8984640 - Flags: review?(mrbkap) → review+
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.
Attachment #8984640 - Flags: sec-approval?
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
Attachment #8984640 - Flags: sec-approval? → sec-approval+
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.
Attachment #8984640 - Flags: approval-mozilla-esr60?
Attachment #8985532 - Flags: approval-mozilla-esr60?
Attachment #8984640 - Flags: approval-mozilla-esr60?
Attachment #8985532 - Attachment is obsolete: true
Attachment #8985532 - Flags: approval-mozilla-esr60?
Attachment #8985536 - Flags: approval-mozilla-esr60?
https://hg.mozilla.org/mozilla-central/rev/9496b052a5d1
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Comment on attachment 8985536 [details] [diff] [review]
Backport to esr60

Blocks a sec-crit. Approved for ESR 60.2.
Attachment #8985536 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62-][adv-esr60.2-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.