874083 - subsumes-conditional prototype remapping in PrepareForWrapping can cause crashes during document.domain remapping

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This is a regression from bug 866823. It doesn't affect release, because we didn't backport this there. It was discovered when trying to backport that bug to esr17, where various incidental factors caused us to trigger an assertion.

The basic problem is that RemapWrappers can't handle a cross-compartment wrapper transforming into a regular same-compartment objects (consumers wishing to do that need to use JS_TransplantObject). So PrepareForWrapping needs to be consistent over time about whether it tells the caller to create a wrapper or whether it just returns a new object for the given scope. Since document.domain can cause security relationships between principals to mutate over time, we need to ignore domain here.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Ignore domain in PrepareForWrapping prototype remapping. v1

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=5243dc2e3cde

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

Review of attachment 751723 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jswrapper.cpp
@@ +980,5 @@
>      JS_ASSERT(Wrapper::wrappedObject(wobj) == newTarget);
>  
>      // Update the entry in the compartment's wrapper map to point to the old
>      // wrapper, which has now been updated (via reuse or swap).
> +    JS_ASSERT(wobj->isWrapper());

I only looked at the assertion, but it seems fine. We're already asserting this in the Wrapper::wrappedObject(wobj) call above it.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

Review of attachment 751723 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 5

[Bobby Holley (:bholley)]

I'm going to go with sec-high - it's a GC hazard, but probably very hard to exploit.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The browser can easily be made to assert (indeed, the test does just that). Constructing an exploit would probably be pretty hard, though.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes

Which older supported branches are affected by this flaw?

Everywhere we backported bug 866823, so aurora, beta, and b2g18.

If not all supported branches, which bug introduced the flaw?

Bug 866823.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

trivial.

How likely is this patch to cause regressions; how much testing does it need?

Not likely to cause regressions, but we should get this in right away, because we just introduced the regression on the branches a few days ago.

Comment 7

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

Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

sec-approval+ for m-c. Please prepare branch patches and nominate there ASAP when m-c is in. We should try to avoid shipping the security issue.

Comment 8

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6dc6f1460f0

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

Flagging for uplift. There might be minor context differences on the branches, but all this patch does is to call subsumesIgnoringDomain instead of subsumes, so it should be trivial to apply everywhere.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 866823
User impact if declined: sec
Testing completed: just pushed to m-i
Risk to taking this patch (and alternatives if risky): not risky
String or UUID changes made by this patch: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 751723 [details] [diff] [review]
Ignore domain in PrepareForWrapping prototype remapping. v1

Approving for landing to esr17 as well, please carry forward if you need to rework the patch (without significant risk) for landing to that branch.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/572f6f89614d
https://hg.mozilla.org/releases/mozilla-beta/rev/7bf4574f3ddc
https://hg.mozilla.org/releases/mozilla-esr17/rev/fc85578c1be0

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/17d02b299ee1

Comment 14

[Matt Wobensmith [:mwobensmith][:matt:]]

I'm assuming no test case here. For QA - based on that and comment 5 - I'd assume it's not worth verification.