826471 - compartment mismatch in nsWindowSH::NewResolve for _content with Xrays

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

I came across across 3 of these. These are on builds from 12/29 and 1/1.

The stacks vary, but they look kind of like this:

0 js::CompartmentChecker::fail 	js/src/jscntxtinlines.h:205
1 js::CompartmentChecker::check 	js/src/jscntxtinlines.h:221
2 JS_NewFunction 	js/src/jsapi.cpp:4839
3 nsWindowSH::NewResolve 	dom/base/nsDOMClassInfo.cpp:6902
4 xpc::XPCWrappedNativeXrayTraits::resolveOwnProperty 	js/xpconnect/wrappers/XrayWrapper.cpp:995
5 xpc::XrayWrapper<js::CrossCompartmentWrapper,xpc::XPCWrappedNativeXrayTraits>::g 

It looks like the window's global object is from a different compartment than the context passed into NewResolve.

I don't know if this is useful or not.

https://crash-stats.mozilla.com/report/index/81a84814-74ab-4437-9a59-dc3502130102
https://crash-stats.mozilla.com/report/index/011537dd-0895-4315-ba2b-6a85f2121230
https://crash-stats.mozilla.com/report/index/03a85589-01f4-475d-89ee-bdbe52121230

Comment 1

[Bobby Holley (:bholley)]

Oh yeah. Xrays generally don't enter the compartment of the target object, but that kind of falls apart when we need to call NewResolve on the wn (bz was complaining about this at some point IIRC).

Anyway, we definitely need a JSAutoCompartment in there. I'm surprised we haven't hit that more - seems like it should be very straightforward to write a reproducible crashtest, which we should probably write for this bug. We might also want to put a compartment assertion directly in the NewResolve hook, since it may be that we were only entering JSAPI in a subset of the NewResolve cases.

Good find!

Comment 2

[Andrew McCreight [:mccr8]]

I looked at this a little. Where should the JSAutoCompartment be?

I stuck these assertions at the start of nsWindowSH::NewResolve:
  MOZ_ASSERT(js::GetContextCompartment(cx) == js::GetObjectCompartment(obj));
  MOZ_ASSERT(js::GetContextCompartment(cx) == js::GetObjectCompartment(win->GetGlobalJSObject()));

The second one triggers at startup, with a stack similar to comment 0. The first assertion doesn't trigger, which I guess means that we're in the compartment of obj, but not the window's global. We call various JS API functions with cx and obj, so it seems like it isn't right to just enter the window's object's compartment. Do we just need to enter the window object's compartment for this weird _content case?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> I looked at this a little. Where should the JSAutoCompartment be?
> 
> I stuck these assertions at the start of nsWindowSH::NewResolve:
>   MOZ_ASSERT(js::GetContextCompartment(cx) == js::GetObjectCompartment(obj));
>   MOZ_ASSERT(js::GetContextCompartment(cx) ==
> js::GetObjectCompartment(win->GetGlobalJSObject()));
> 
> The second one triggers at startup, with a stack similar to comment 0. The
> first assertion doesn't trigger, which I guess means that we're in the
> compartment of obj, but not the window's global. We call various JS API
> functions with cx and obj, so it seems like it isn't right to just enter the
> window's object's compartment. Do we just need to enter the window object's
> compartment for this weird _content case?

Oh, wow. This stuff is even more broken than I thought. It looks like we pass the wrapper, not the holder, to NewResolve, and depend on that in a number of places (specifically, the various checks for IsNativeWrapper, which will return true for Xrays). At the same time though, |obj| is also as the target for defining properties, which means that nsDOMClassInfo ends up calling DefineProperty on the Xray proxy, which ends up defining the thing as an expando, rather than a property on the holder. Ugh.

Anyway, barring major catastrophes I think the plan is to limp forward on this stuff until we can get it converted to the new bindings. In which case, yeah, we should just fix the weird _content case, and maybe do a quick audit of the other NewResolve hooks to see if any of them do something similar.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: test and patch

Look at this super awesome test:
  Components.utils.evalInSandbox("window._content", sandbox);

Just that causes a fatal compartment mismatch! I don't know whether to laugh or cry. Anyways, this patch fixes it. I'll split apart the test and code once I fix the rest of it.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Look at this super awesome test:
>   Components.utils.evalInSandbox("window._content", sandbox);
> 
> Just that causes a fatal compartment mismatch!

The crashtest in bug 786142 is even simpler. ;-)

Comment 8

[Andrew McCreight [:mccr8]]

File bug 829310 and bug 829331 for the two other problems I found by inspection, and marked those comments private just in case this gets fixed and opened before the others.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: mighty patch

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: mighty test

To be landed later. Although this is hardly more obvious than the actual patch.

Comment 11

[Andrew McCreight [:mccr8]]

As I expected, this code has been like this for a long time[1], so it probably affects all supported branches.

[1] http://hg.mozilla.org/mozilla-central/annotate/cefda2b7296b/dom/base/nsDOMClassInfo.cpp

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 700769 [details] [diff] [review]
mighty patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? The problem is quite obvious, but the actual badness requires chrome code using a deprecated property on content, so how effective an exploit would be depends on how much it is used by addons or browser code. I've seen a few crash stacks involving it, so it isn't totally unused. It is a use-after-free on a node, so that could end badly.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? I'm not going to check in the test. Though the test isn't much more obvious than the code, to somebody who is willing to sit down and understand the C++.

Which older supported branches are affected by this flaw? Everything.

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

How likely is this patch to cause regressions; how much testing does it need? Seems very safe to me.

Comment 13

[bhavana bajaj [:bajaj]]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> Comment on attachment 700769 [details] [diff] [review]
> mighty patch
> 
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch? The problem
> is quite obvious, but the actual badness requires chrome code using a
> deprecated property on content, so how effective an exploit would be depends
> on how much it is used by addons or browser code. I've seen a few crash
> stacks involving it, so it isn't totally unused. It is a use-after-free on a
> node, so that could end badly.
> 
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem? I'm not going to check in
> the test. Though the test isn't much more obvious than the code, to somebody
> who is willing to sit down and understand the C++.
> 
> Which older supported branches are affected by this flaw? Everything.
> 
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be? Patch is trivial.
> 
> How likely is this patch to cause regressions; how much testing does it
> need? Seems very safe to me.

CAn you please go ahead and land this on m-c,branches asap ? Thanks

Comment 14

[Andrew McCreight [:mccr8]]

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

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 700769 [details] [diff] [review]
mighty patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): very old
User impact if declined: possibly security problems and crashes
Testing completed: partial try run
Risk to taking this patch (and alternatives if risky): should be pretty low
String or UUID changes made by this patch: none

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 700769 [details] [diff] [review]
mighty patch

Approving for all branches other than B2G - that branch isn't ready for 19+ landings yet.

Comment 17

[Andrew McCreight [:mccr8]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/59212ae7c2e6
https://hg.mozilla.org/releases/mozilla-esr17/rev/592f4f716fe7

This is also in my queue to land on Aurora once it reopens.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b7ceb5f0e974

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 700769 [details] [diff] [review]
mighty patch

To the best of my knowledge there's *no* way for this code to get executed on b2g. If that's an incorrect reading of this, please re-nominate.

Comment 21

[Andrew McCreight [:mccr8]]

jst: Isn't _content just a property on window?  I don't really understand why it couldn't be executed.

Comment 22

[bhavana bajaj [:bajaj]]

(In reply to Andrew McCreight [:mccr8] from comment #21)
> jst: Isn't _content just a property on window?  I don't really understand
> why it couldn't be executed.

woops, I just the status as "wontfix" based on comment# 20 .Please mark this as "affected" if the landscape changes.

Comment 23

[Bobby Holley (:bholley)]

Johnny, can you weigh on on comment 21? I'm not really following either.

Comment 24

[Andrew McCreight [:mccr8]]

Comment on attachment 700769 [details] [diff] [review]
mighty patch

I talked to jst, and we agree that this can actually affect B2G after all.  content can somehow get cross-compartment Xrays to its own origin.

Comment 25

[Andrew McCreight [:mccr8]]

See comment 15 for approval request details.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/74396dd3bb27

Comment 27

[Andrew McCreight [:mccr8]]

Please leave this bug as sec-sensitive for the moment, as the test case here is triggering another assertion (bug 861530).

Comment 28

[Andrew McCreight [:mccr8]]

Other assertion is probably not a sec problem.

Comment 29

[Andrew McCreight [:mccr8]]

Landed a test for this in bug 861530.