830399 - compartment mismatch in nsXMLHttpRequest::GetInterface (with addon?)

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

There are a number of these, but they all appear to be from the same Linux user, a few minutes apart.

2 JS_GetGlobalForObject 	js/src/jsapi.cpp:2241
3 nsXMLHttpRequest::GetInterface 	content/base/src/nsXMLHttpRequest.cpp:3874
4 mozilla::dom::XMLHttpRequestBinding::getInterface 	obj-firefox/dom/bindings/XMLHttpRequestBinding.cpp:1197
5 mozilla::dom::XMLHttpRequestBinding::genericMethod 	obj-firefox/dom/bindings/XMLHttpRequestBinding.cpp:1277

https://crash-stats.mozilla.com/report/index/48f6700a-c15b-4093-8450-82d6b2130112
https://crash-stats.mozilla.com/report/index/6a822b01-4346-417e-862b-113142130112
https://crash-stats.mozilla.com/report/index/29b85a8f-c538-42f0-afa2-f4de42130112
https://crash-stats.mozilla.com/report/index/623f5ac7-5823-4427-95d1-5222d2130112

This user has [e-mail], Firebug, and Sharemenot, so perhaps one of those is the underlying problem.

Comment 1

[Andrew McCreight [:mccr8]]

The problem is here:
  JSObject* global = JS_GetGlobalForObject(aCx, GetWrapper());
  aRv = nsContentUtils::WrapNative(aCx, global, result, iid, &v);

The surrounding code in the bindings makes it look like a result in another compartment is expected, but perhaps that's just the result of glue generated code:
  result = self->GetInterface(cx, arg0, rv);
  [...]
  *vp = result;
  if (!MaybeWrapValue(cx, vp)) {

bz, does it make sense to you to just wrap the nsContentUtils::WrapNative call with an auto enter compartment?  I think that's what bholley was suggesting to me.

Comment 2

[Boris Zbarsky [:bzbarsky]]

We don't trust anything about the compartment of the result, yeah.  It could be anything.

The correct thing to do here is to enter GetWrapper()'s compartment around the JS_GetGlobalForObject and WrapNative stuff, as you said.

Comment 3

[Bobby Holley (:bholley)]

bz - The main question I wanted to bounce to you was whether you could think of any way in which this might happen. There's no Xray on the stack, and XHR can't be adopted, right?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Should be pretty easy by dynamically setting the __proto__ of an XHR to a XMLHttpRequest.prototype from a different global, no?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Should be pretty easy by dynamically setting the __proto__ of an XHR to a
> XMLHttpRequest.prototype from a different global, no?

Ah, right. We enter that compartment and then the bindings just unwrap |this|.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: test

While the original crash stacks don't seem to involve Xrays, we can use Xrays to get an assert in the same place, so I just did that.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: enter compartment in nsXHR::GetInterface

Simple enough fix. I leave the compartment before calling WrapNative so that the result is hopefully in the original context. It doesn't matter here, because the bindings code calls MaybeWrapValue on the result, so we end up in the right compartment either way, but it seems better to return something from GetInterface that isn't in a random compartment.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Doesn't leaving before WrapNative mean we're passing mixed-compartment stuff to WrapNative?

Comment 9

[Andrew McCreight [:mccr8]]

> Doesn't leaving before WrapNative mean we're passing mixed-compartment stuff to WrapNative?

Yes, cx won't be in the same compartment as the object we're passing in, but I assumed that was okay because the goal of WrapNative was to encapsulate stuff like that? I could be wrong.

Judging from the hg blame, this is a regression from the original XHR bindings landing.

Marking as sec-critical because it is a cross-compartment problem, but maybe it is mitigated a bit by requiring an addon do something weird? Right now it just seems like one person is having this problem.

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

I would rather just make sure we always pass things in the right compartments to WrapNative, personally...

Comment 11

[Andrew McCreight [:mccr8]]

Ah, yeah, I just got confused. That JS obj argument is the compartment of cx, not the thing we're wrapping...

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: enter the compartment

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 702951 [details] [diff] [review]
enter the compartment

Try run was good.
https://tbpl.mozilla.org/?tree=Try&rev=4411b77b1b4d

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 702520 [details] [diff] [review]
test

> +      Components.utils.evalInSandbox("document.defaultView.XMLHttpRequest = function() {};", sandbox);

You don't need this line; take it out, please.

r=me with that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 702951 [details] [diff] [review]
enter the compartment

r=me

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: test

Removed copypasta, confirmed it still fails without patch and passes with test. Carrying forward bz's r+.

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 702951 [details] [diff] [review]
enter the compartment

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Probably not very easily, plus it would seem to only be a problem when certain addons are used.

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 checking in the test, and there are no comments.

Which older supported branches are affected by this flaw? 14+

If not all supported branches, which bug introduced the flaw? new XHR bindings (whatever I marked as being blocked by this)

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

How likely is this patch to cause regressions; how much testing does it need? Seems pretty low risk.

Comment 18

[Andrew McCreight [:mccr8]]

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

Comment 19

[Andrew McCreight [:mccr8]]

Comment on attachment 702951 [details] [diff] [review]
enter the compartment

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

Comment 20

[Andrew McCreight [:mccr8]]

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

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 702951 [details] [diff] [review]
enter the compartment

This is a fairly low risk sg:crit fix, and we're now coming up on beta 3.

Comment 22

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5a7e4960edf3
https://hg.mozilla.org/releases/mozilla-aurora/rev/e700932a28bf

Still needs landing on ESR17, but it is currently burning.

Comment 23

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/a9ec206d85ee

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

Comment 25

[bhavana bajaj [:bajaj]]

Can you please uplift the patch for b2g18 & land it on mozilla-b2g18_v1_0_1 as well (should be identical fixes)  asap. thank you !

Comment 26

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f644851a99d8

Comment 27

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ec066945565b