compartment mismatch in nsWindowSH::NewResolve for _content with Xrays

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({sec-critical, testcase})

unspecified
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 wontfix, firefox19+ fixed, firefox20+ fixed, firefox21+ fixed, firefox-esr1719+ fixed, b2g1819+ fixed, b2g18-v1.0.0 affected)

Details

(Whiteboard: [adv-main19+][adv-esr1703+])

Attachments

(2 attachments, 1 obsolete attachment)

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
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!
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?
(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.
Assignee: nobody → continuation
Summary: cross compartment pointers in xpc::XPCWrappedNativeXrayTraits::resolveOwnProperty → compartment mismatch in nsWindowSH::NewResolve with Xrays
Posted patch test and patch (obsolete) — Splinter Review
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.
(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. ;-)
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.
Summary: compartment mismatch in nsWindowSH::NewResolve with Xrays → compartment mismatch in nsWindowSH::NewResolve for _content with Xrays
Posted patch mighty patchSplinter Review
Attachment #698975 - Attachment is obsolete: true
Attachment #700769 - Flags: review?(bobbyholley+bmo)
Posted patch mighty testSplinter Review
To be landed later. Although this is hardly more obvious than the actual patch.
Attachment #700770 - Flags: review?(bobbyholley+bmo)
Flags: in-testsuite?
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
Attachment #700770 - Flags: review?(bobbyholley+bmo) → review+
Attachment #700769 - Flags: review?(bobbyholley+bmo) → review+
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.
Attachment #700769 - Flags: sec-approval?
Attachment #700769 - Flags: sec-approval? → sec-approval+
(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 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
Attachment #700769 - Flags: approval-mozilla-esr17?
Attachment #700769 - Flags: approval-mozilla-beta?
Attachment #700769 - Flags: approval-mozilla-b2g18?
Attachment #700769 - Flags: approval-mozilla-aurora?
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.
Attachment #700769 - Flags: approval-mozilla-esr17?
Attachment #700769 - Flags: approval-mozilla-esr17+
Attachment #700769 - Flags: approval-mozilla-beta?
Attachment #700769 - Flags: approval-mozilla-beta+
Attachment #700769 - Flags: approval-mozilla-aurora?
Attachment #700769 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/c31858da7548
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: testcase
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Attachment #700769 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
jst: Isn't _content just a property on window?  I don't really understand why it couldn't be executed.
(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.
Johnny, can you weigh on on comment 21? I'm not really following either.
Flags: needinfo?(jst)
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.
Attachment #700769 - Flags: approval-mozilla-b2g18- → approval-mozilla-b2g18?
Flags: needinfo?(jst)
See comment 15 for approval request details.
Attachment #700769 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Whiteboard: [adv-main19+][adv-esr1703+]
Please leave this bug as sec-sensitive for the moment, as the test case here is triggering another assertion (bug 861530).
Whiteboard: [adv-main19+][adv-esr1703+] → [adv-main19+][adv-esr1703+][leave closed]
Other assertion is probably not a sec problem.
Whiteboard: [adv-main19+][adv-esr1703+][leave closed] → [adv-main19+][adv-esr1703+]
Landed a test for this in bug 861530.
Flags: in-testsuite? → in-testsuite+
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.