1467970 - Forbid node adoption across the chrome/content boundary

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Consider the attached test change.

When I run this test in a debug build, I fail this assertion:

   MOZ_ASSERT(Wrapper::wrappedObject(wobj) == newTarget);

in js::RemapWrapper.  newTarget is a dead object proxy.  wobj is an an XrayWaiver for an HTMLSpanElement instance.

This is happening because the comments about tobj/wobj are wrong in this case.  They say:

    // If wrap() reused |wobj|, it will have overwritten it and returned with
    // |tobj == wobj|. Otherwise, |tobj| will point to a new wrapper and |wobj|
    // will still be nuked. In the latter case, we replace |wobj| with the
    // contents of the new wrapper in |tobj|.

And then we swap tobj with wobj.  We alredy nuked wobj, so this makes tobj nuked.

But that comment is wrong.  In this case tobj does _not_ point to a  new wrapper.  It's still pointing to newTarget, which is what tobj was initialized to.  So when we do this swap newTarget becomes a dead proxy.

Why is this happening?  Because we're remapping the CCW from the Xray compartment to the old Xray waiver to point to the new Xray waiver.  When we call wcompartment->rewrap(), where wcompartment is the Xray compartment, we see that the object involved (the new Xray waiver) is already in our compartment and return immediately:

         if (obj->compartment() == this)
             return true;

So at this callsite:

    if (!wcompartment->rewrap(cx, &tobj, wobj))

tobj and wobj both stay what they are (tobj being another name for newTarget at this point and wobj being a dead proxy), and then we swap them.

So one question is: what _should_ happen in this case?  We used to have two different representations of the HTMLSpanElement in the Xray compartment: the Xray and the WaiveXrayWrapper.  Once we adopt it, we should probably only have one representation.  The Xray gets swapped with the object itself so becomes the HTMLSpanElement.  What should the WaiveXrayWrapper become?  In an ideal world maybe that same HTMLSpanElement?

I guess I'll needinfo myself until Bobby is back...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Test change under discussion

Comment 3

[Bobby Holley (:bholley)]

Per IRC discussion, I think we should just forbid adopting nodes across Xray boundaries. For the case of XBL/UAWidget creating nodes, I think we should expose APIs to create them in the right place, rather than doing the whole adopt dance.

Comment 4

[Bobby Holley (:bholley)]

So it sounds like we agree that we should forbid adopting content nodes into chrome. Per bug 1488655 comment 6, we should also forbid adoption chrome nodes into content.

I think the sum total of these two issues is enough risk that it's worth having somebody work on this. Setting this to sec-high so that that happens.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) from comment #4)
> So it sounds like we agree that we should forbid adopting content nodes into
> chrome. Per bug 1488655 comment 6, we should also forbid adoption chrome
> nodes into content.

Note this is probably going to annoy the person in bug 1492041 whose last workaround for fragment sanitization in chrome documents is using DOMParser and adopting nodes from it. I'm not super concerned about that, but we may wind up having other code that relies on using DOMParser to create nodes for chrome documents.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #5)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > So it sounds like we agree that we should forbid adopting content nodes into
> > chrome. Per bug 1488655 comment 6, we should also forbid adoption chrome
> > nodes into content.
> 
> Note this is probably going to annoy the person in bug 1492041 whose last
> workaround for fragment sanitization in chrome documents is using DOMParser
> and adopting nodes from it.

Just to write it here since it wasn't obvious from comment 5, that is a thunderbird use-case, and not something we need to consider when weighing options for making Firefox more secure.

> I'm not super concerned about that, but we may
> wind up having other code that relies on using DOMParser to create nodes for
> chrome documents.

That would basically be somebody working around our security measures, right? Seems like we'd want to find and fix that code anyway.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) from comment #6)
> > I'm not super concerned about that, but we may
> > wind up having other code that relies on using DOMParser to create nodes for
> > chrome documents.
> 
> That would basically be somebody working around our security measures,
> right? Seems like we'd want to find and fix that code anyway.

Maybe. Or they might just be using DOMParser for some other reason, without giving it significant consideration. Either way, though, yeah, we'd probably want to find and fix that code.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Note that I have some partial patches for this in my tree, but I haven't had a chance to get them into try-able shape so far....

Comment 9

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

This could actually affect to the implementation plans in bug 1377999.

Comment 10

[Bobby Holley (:bholley)]

See also bug 1516237 comment 5, which has another potential solution to the issue in comment 0 (though I think we still want to forbid this kind of adoption for other reasons like comment 9).

Comment 11

[Andrew Overholt [:overholt]]

Has anything changed here given the same compartment realms work that's recently landed?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Not really related, no.

Comment 13

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

So did bug 1516237 comment 5 affect this one.

(bug 1377999 isn't security bug, nor it is clear it will ever happen)

Comment 14

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #13)

So did bug 1516237 comment 5 affect this one.

That did land. I haven't paged all the details back in, but at least at the time I was pretty sure that approach fixed the specific issue in comment 0.

I still think forbidding content/chrome adoption is an important defense-in-depth change.

Comment 15

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

Then the question is that how high priority this bug should be?

Comment 16

[Kris Maglione [:kmag]]

(In reply to Olli Pettay [:smaug] from comment #15)

Then the question is that how high priority this bug should be?

It seems like it should be a simple enough change with high enough pay-off for it to be relatively high priority.

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Kris Maglione [:kmag] from comment #16)

(In reply to Olli Pettay [:smaug] from comment #15)

Then the question is that how high priority this bug should be?

It seems like it should be a simple enough change with high enough pay-off for it to be relatively high priority.

Comment 18

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

(priority is still unclear. There isn't known concrete security issue here, so this is more like ensuring we won't have security issues)

Comment 19

[Kris Maglione [:kmag]]

One potential known security issue is that we sanitize any markup that we put through a fragment parser in chrome documents, so the only way to inject unsanitized markup into chrome documents is to parse it in a content document and then adopt it into a chrome document. I think it's only a matter of time before someone starts doing that, if they aren't already.

Comment 20

[Sean Feng [:sefeng]]

I am working on disabling cross docGroup adoption in bug 1377999

Comment 21

[Sean Feng [:sefeng]]

Just as what kmag said, we do a lot of fragment parsing and adopt the created fragment into the chrome document, and the created fragments have a different docGroup than the chrome document because the principles are different. See here. Once we start to unsupport cross docGroup adoption, one of the ways to fix it is using importNode to clone the node into the chrome document, and then do the adoption.

However, smaug raised a concern that if we start to prevent adopting cross-docgroups, perhaps worth to use system principal? I think bz also had some concerns about whether the DOMParser doc should in fact be in a separate docgroup from the document that created it. I'd like to know bz and smaug's thoughts here and see if we can agree on something.

Comment 22

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

Using importNode is memory heavy, so I'd rather see a way to not use that. But then, if one parses some string coming from a web page, that shouldn't ever use system principal/docGroup by default. (though, importNode can hide real origin of the dom fragment.)

Could we perhaps add some
[NewObject, ChromeOnly, Throws]
Document parseFromSafeString(DOMString str, SupportedType type);
to DOMParser and that would use system principal.
It should be relatively clear to the caller that it is up to it to ensure the string is safe to parse.

Comment 23

[Boris Zbarsky [:bzbarsky]]

The proposal in comment 22 seems reasonable to me.

If we take that approach, we could conceivably also:

1. Have parseFromSafeString return a DocumentFragment and create the nodes in the caller's document to start with. Probably not that big a deal if the caller can adopt.

2. Have importNode throw when importing from non-system into system, and fix existing callers as needed.

Comment 24

[Sean Feng [:sefeng]]

Attachment: Bug 1467970 - Unsupport cross docGroup adoption r=smaug

Comment 25

[Sean Feng [:sefeng]]

Comment on attachment 9090762 [details]
Bug 1467970 - Unsupport cross docGroup adoption r=smaug

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very hard. It's more like an enhancement to a potential security issue that only exposed internally.
• 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?: Not sure, maybe all branches? But again, these are no real security issue here.
• If not all supported branches, which bug introduced the flaw?: N/A
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Since there is no real security issue, it will be no different and not risky.
• How likely is this patch to cause regressions; how much testing does it need?: There should be no regressions. I don't think it requires any extra testing, as long as it passes all our test cases, it should be fine.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9090762 [details]
Bug 1467970 - Unsupport cross docGroup adoption r=smaug

sec-approval+ for mozilla-central

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/c8a2c27a1128

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/c8a2c27a1128

Comment 29

[Ryan VanderMeulen [:RyanVM]]

I was chatting with bz about this on IRC and he was leaning towards not trying to backport this fix, but he suggested I check with y'all about that. Do think we should let this ride the trains?

Comment 30

[Bobby Holley (:bholley)]

If we didn't uncover any concrete security issues in the process of developing and landing this, then I would treat this more as a defense-in-depth improvement, and not consider it critical for uplifting. So if we think the task of doing so is going to require non-trivial engineer time, I'd say we should skip it.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

It looks like the patch mostly grafts as-is to ESR68. I'm wondering if maybe we should give this a cycle to bake and revisit when 71 is on Beta in mid-November?

Comment 32

[Kris Maglione [:kmag]]

The only real concern I have about not uplifting is that the patch hints at one of the last remaining avenues for chrome-level XSS that we know about. That said, if we're confident that none of the ancillary code we had to change to get this landed is likely to be exploitable, we should probably be OK.

Comment 33

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

This is not a security bug as such, and didn't see any obvious concrete security issues, so riding the trains should be fine.

Comment 34

[Andrew McCreight [:mccr8]]

Given the last few comments, I'm going to downgrade this to sec-want.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Given the various regressions filed here so far, have we done a try run where we MOZ_CRASH in the cases when this throws and see whether there are any resulting failures?

Comment 36

[Sean Feng [:sefeng]]

I did run dozens of try pushes to make sure the patch looks good in general for all test cases and all platforms. Speaking of MOZ_CRASH, I did used it a few times, but it looks like I didn't do it sufficiently. What I am going to do is I'll look for the filed regressions and I'll also do a try run with MOZ_CRASH in the case.

Comment 37

[Sean Feng [:sefeng]]

Looks like we didn't catch these regressions during try pushes because the corresponding test cases were missing.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Yes, I'm not saying we should have caught those regressions: if there's no test coverage, then there is no test coverage. If you did try pushes with MOZ_CRASH, no more action needed here...

Comment 39

[Tomislav Jovanovic :zombie]

It seems like this bug started with a concern about adopting DOM nodes from chrome into a content document, or vice versa, but the patch in the end prevents any adoptions across docGroups, which seems to impact web extensions (bug 1585588, and bug 1576508 comment 25).

Given that we already discovered a few regressions in our own code in the couple of weeks since this landed, I'm worried this might be a common pattern in 3rd party extensions, and that it might break a lot of them when this hits release.

I understand this is generally not a pattern to be encouraged, but would it be possible to temporary allow cross-docGroup adoption when both sides are content (which I think might currently only be possible with extensions, though I'm at the edges of my understanding here)?

Preferably, we would be able to announce this change on the addons blog, and only put it into effect a few releases later.

Boris and Sean (and others), what do you think?

Comment 40

[Tomislav Jovanovic :zombie]

Hi Philipp, we will probably need some comms about this issue, either in the next regular blog post (if my proposal is accepted), or something out of band otherwise.

Comment 41

[Sean Feng [:sefeng]]

TBH, I am not sure how/whether it will impact web extensions.

My understanding was node adoption which cross the docGroup boundary was only exposed to chrome document, so web extensions itself shouldn't have this privilege anyway, but I'd like to hear from Boris and Olli.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Looking at bug 1585588, it looks like the code there does new DOMParser in a web extension content script. What docgroup does that DOMParser get? Does it get the same docgroup as the documents that content script can touch (and are all those in the same docgroup)?

Comment 43

[Tomislav Jovanovic :zombie]

Attachment: adoptNodeXdocGroup.zip

This is a reduced STR extension, unzip and load it as a temporary addon from about:debugging, or zipped from about:addons after you set the pref xpinstall.signatures.required to false (in a Nightly/local build).

(It seems bug 1576508 comment 25 is a different STR that doesn't use adoptNode , but I'm yet to figure out exactly what that extension is doing and reduce it to a usable STR)

(In reply to Sean Feng [:sefeng] from comment #41)

TBH, I am not sure how/whether it will impact web extensions.

My understanding was node adoption which cross the docGroup boundary was only exposed to chrome document, so web extensions itself shouldn't have this privilege anyway, but I'd like to hear from Boris and Olli.

It definitely impacts extensions (see bug 1585588, and bug 1576508 comment 25), because a content script on example.com can (could) insert an iframe from a moz-extension:// url, and adopt nodes between them, even though they seem to be in different docGroups.

Comment 44

[Tomislav Jovanovic :zombie]

(In reply to :Tomislav Jovanovic :zombie from comment #43)

This is a reduced STR extension, unzip and load it as a temporary addon from about:debugging, or zipped from about:addons after you set the pref xpinstall.signatures.required to false (in a Nightly/local build).

And then open http://example.com

Comment 45

[Tomislav Jovanovic :zombie]

Sean, above are STR and details you asked for in bug 1585588 comment 17.

Comment 46

[Kris Maglione [:kmag]]

Even without a DOMParser, it's possible (until Fission) for a content script to access nodes from both web content and extension docGroups at the same time, and for tabs (unfortunately) to access nodes from background pages, popups, and sidebars, which are in completely different tabGroups.

Comment 47

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

Sure, possible, but we don't want that. That is what this whole bug is about.

Temporarily we could limit the change to chrome/non-chrome boundary, even though that would prevent bug 1377999 to land,
and would expose the extensions to possible security issues.

Comment 48

[Kris Maglione [:kmag]]

Agreed, I don't want us to loosen the security restrictions for these cases.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Even without a DOMParser, it's possible (until Fission)

It seems like we should disable things that will stop working with Fission, generally speaking, even if we don't have Fission enabled yet.... Presumably this stuff works in Chrome somehow, with their site isolation bits, so I'm a little surprised we're exposing different semantics from them.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Also, we might be best served here in filing a new bug to track the extension impact of this change, not least so we can track disabling this change temporarily for 71, if needed, until we get things sorted out...

Comment 51

[Sean Feng [:sefeng]]

Thanks zombie.

Shall we just back the patch out to make it not happen for the next release?

I could also come up a patch to limit the change to chrome/non-chrome boundary, or maybe a pref to disable it temporarily?

Comment 52

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #49)

Even without a DOMParser, it's possible (until Fission)

It seems like we should disable things that will stop working with Fission, generally speaking, even if we don't have Fission enabled yet.... Presumably this stuff works in Chrome somehow, with their site isolation bits, so I'm a little surprised we're exposing different semantics from them.

The DOMParser stuff and the communication between tab pages and background pages/popups/sidebars do. The direct interaction between content scripts and extension frames does not. After Fission, only the last will stop working in Firefox. We're going to need to find a way to deal with the DocGroup/BrowsingContextGroup mismatch in the other cases sooner or later, since they break other invariant as well.

Comment 53

[Tomislav Jovanovic :zombie]

Thanks all, it seems we have a consensus that we should try to keep the security restrictions across content/chrome docGroups, and if we can, temporarily disable them for the content/content case (currently only possible with Web Extensions), and try to uplift that to Beta 71.

Filed bug 1590526 to track that work.

This will give us enough time to properly communicate the coming restrictions, with the intent of putting them into effect with Firefox 73 or 74.

Comment 54

[Sean Feng [:sefeng]]

zombie, do you have an idea about how we are going to communicate about the cross docGroup adoption changes to the addon community if we want to put them into effect? Like in how many releases? Whether this is acceptable or not from the addon team's point of view?

I am asking because bug 1377999 is ready to land, and if we want to keep supporting cross docGroup adoption, we need to make some changes into the patch, and we'd rather to not do that.

Comment 55

[Tomislav Jovanovic :zombie]

We considered explaining docGroups to extension developers was somewhat complicated, so we thought it would be better to do it together with the Fission intro/explainer. Once we're ready to invite people to try Fission on Nightly by flipping the pref and test their extensions, it should be easier to explain that content scripts won't be able to directly access documents in cross-origin (out of process) iframes, and deprecate cross-docGroup adoption (a subset) at the same time.

The plan was for that to already happen during March, but Fission progress is not at that point yet as a whole, and specifically we don't have OOPIF content scripts working yet. Not sure that answers your question, or how it aligns with your plans.

Comment 56

[David Lawrence [:dkl]]

Removing employee no longer with company from CC list of private bugs.