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

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

Firefox Tracking Flags

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

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

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 BitTorrent_WebUI_2@firefox.alexisbrunet.com, Firebug, and Sharemenot, so perhaps one of those is the underlying problem.
Flags: sec-bounty?
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.
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.
Assignee: nobody → continuation
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?
Should be pretty easy by dynamically setting the __proto__ of an XHR to a XMLHttpRequest.prototype from a different global, no?
(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|.
Posted patch test (obsolete) — Splinter Review
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.
Flags: in-testsuite?
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.
Doesn't leaving before WrapNative mean we're passing mixed-compartment stuff to WrapNative?
> 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
Blocks: 740069
Keywords: sec-critical
I would rather just make sure we always pass things in the right compartments to WrapNative, personally...
Ah, yeah, I just got confused. That JS obj argument is the compartment of cx, not the thing we're wrapping...
Attachment #702522 - Attachment is obsolete: true
Attachment #702520 - Flags: review?(bzbarsky)
Comment on attachment 702951 [details] [diff] [review]
enter the compartment

Try run was good.
https://tbpl.mozilla.org/?tree=Try&rev=4411b77b1b4d
Attachment #702951 - Flags: review?(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.
Attachment #702520 - Flags: review?(bzbarsky) → review+
Comment on attachment 702951 [details] [diff] [review]
enter the compartment

r=me
Attachment #702951 - Flags: review?(bzbarsky) → review+
Posted patch testSplinter Review
Removed copypasta, confirmed it still fails without patch and passes with test. Carrying forward bz's r+.
Attachment #702520 - Attachment is obsolete: true
Attachment #703397 - Flags: review+
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.
Attachment #702951 - Flags: sec-approval?
Attachment #702951 - Flags: sec-approval? → sec-approval+
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
Attachment #702951 - Flags: approval-mozilla-esr17?
Attachment #702951 - Flags: approval-mozilla-beta?
Attachment #702951 - Flags: approval-mozilla-b2g18?
Attachment #702951 - Flags: approval-mozilla-aurora?
Attachment #702951 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/b0c7f383f374
Status: NEW → RESOLVED
Closed: 7 years ago
tracking-b2g18: --- → ?
Resolution: --- → FIXED
Keywords: testcase
Target Milestone: --- → mozilla21
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.
Attachment #702951 - Flags: approval-mozilla-esr17?
Attachment #702951 - Flags: approval-mozilla-esr17+
Attachment #702951 - Flags: approval-mozilla-beta?
Attachment #702951 - Flags: approval-mozilla-beta+
Attachment #702951 - Flags: approval-mozilla-aurora?
Attachment #702951 - Flags: approval-mozilla-aurora+
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
Whiteboard: [adv-main19+][adv-esr1703+]
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 !
Group: core-security
Flags: in-testsuite?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.