compartment mismatch in nsJSContext::ExecuteScript (with Babylon toolbar and Free Download Manager?)

RESOLVED FIXED in Firefox 20

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug, {crash, csectype-dos, sec-low})

unspecified
mozilla21
crash, csectype-dos, sec-low
Points:
---

Firefox Tracking Flags

(firefox18 wontfix, firefox19- wontfix, firefox20- fixed, firefox21+ fixed, firefox-esr10 affected, firefox-esr17- wontfix, b2g18 wontfix)

Details

(Whiteboard: [adv-main20+], crash signature)

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
This function does an autorequest, but does not enter the compartment it is about to start executing in. bholley suggested the fix is to just enter the compartment there.

This seems to cause badness when nsXULDocument::OnStreamComplete calls into nsXULDocument::ExecuteScript:
https://crash-stats.mozilla.com/report/index/54ae0c21-948c-4cb9-8e9b-b1b692130106
https://crash-stats.mozilla.com/report/index/88677c28-4597-4d7d-9b16-50b502130112
This is similar, though the stack looks junky:
https://crash-stats.mozilla.com/report/index/34337f2a-e82d-4c86-8a40-61ea72130113
(Assignee)

Updated

6 years ago
Keywords: sec-critical
(Assignee)

Comment 1

6 years ago
Created attachment 702957 [details] [diff] [review]
enter the compartment

I'm not sure how to make a test case for this. I guess I'll investigate the stream listener stuff.
Assignee: nobody → continuation
(Assignee)

Comment 2

6 years ago
All 3 of these crashes have Babylon Toolbar and Free Download Manager.
Summary: compartment mismatch in nsJSContext::ExecuteScript → compartment mismatch in nsJSContext::ExecuteScript (with Babylon toolbar and Free Download Manager?)
(Assignee)

Comment 5

6 years ago
Comment on attachment 702957 [details] [diff] [review]
enter the compartment

Suggestions on how to test this would be appreciated. The trick of using Xrays doesn't seem to help on this one.
Attachment #702957 - Flags: review?(bzbarsky)
Are we guaranteed that aScopeObject and aScriptObject are in the same compartment?
For the rest, all you need is for aContext to not match the compartment of mScriptGlobalObject.  For example, having a JS-implemented protocol handler (which is what seems to be going on here) that's running on the cx but is in some random compartment, would do it...
(Assignee)

Comment 8

6 years ago
Comment on attachment 702957 [details] [diff] [review]
enter the compartment

> Are we guaranteed that aScopeObject and aScriptObject are in the same compartment?

Oh, good point!  No, we aren't. For instance, if you pass in NULL aScopeObject, aScopeObject ends up being the global of mContext, so we're not getting anything out of it.  I think I recall some other execute script thing checking if various compartments are equal or not, and doing different things based on that, so I'll try figuring out what we should do here, and try to throw together a test case. Thanks.
Attachment #702957 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

6 years ago
Okay, so now I remember where I saw that script compartment checking thing: JS_ExecuteScript (which nsJSContext::ExecuteScript calls) actually checks to see if the script isn't in the same compartment as the scope, and if it does, it clones the script into the scope compartment. So it should be okay to completely not worry about the compartment of aScriptObject here.

I've been digging into Free Download Manager, and it looks like its onStopRequest includes a call into a DLL, which I could imagine being problematic.
(Assignee)

Comment 10

6 years ago
Hilariously I just noticed that this isn't technically a security problem, because in bug 746036 dmandelin added a dynamic check that crashes when it detects precisely this compartment mismatch:

assertSameCompartment(cx, obj);
if (cx->compartment != obj->compartment())
  *(volatile int *) 0 = 0xf0;

These show up as the signature JS_ExecuteScript: https://crash-stats.mozilla.com/report/list?range_value=7&range_unit=days&date=2013-01-17&signature=JS_ExecuteScript%28JSContext*%2C%20JSObject*%2C%20JSScript*%2C%20JS%3A%3AValue*%29&version=Firefox%3A19.0b1

This is currently the #45 top crasher on Fx19, so it would be good to fix anyways.

I'll leave this closed for now until more of its sibling bugs are fixed.
Keywords: sec-critical → csec-dos, sec-low
(Assignee)

Comment 11

6 years ago
Comment on attachment 702957 [details] [diff] [review]
enter the compartment

Re-requesting review in light of comment 9.
Attachment #702957 - Flags: review?(bzbarsky)

Comment 12

6 years ago
mccr8, should we add that signature to this bug then (and add the crash keyword), so it gets linked with this?
(Assignee)

Comment 13

6 years ago
Sure.
Crash Signature: [@ JS_ExecuteScript(JSContext*, JSObject*, JSScript*, JS::Value*)]
Keywords: crash
Comment on attachment 702957 [details] [diff] [review]
enter the compartment

r=me
Attachment #702957 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

6 years ago
Comment on attachment 702957 [details] [diff] [review]
enter the compartment

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Probably not that easily, plus it seems to require certain addons.

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

Which older supported branches are affected by this flaw? Probably all of them.

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

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

How likely is this patch to cause regressions; how much testing does it need? Doesn't seem that dangerous.
Attachment #702957 - Flags: sec-approval?

Updated

6 years ago
status-firefox-esr10: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox-esr17: --- → affected
tracking-firefox19: --- → ?
tracking-firefox20: --- → ?
tracking-firefox21: --- → +
tracking-firefox-esr17: --- → ?

Comment 16

6 years ago
Comment on attachment 702957 [details] [diff] [review]
enter the compartment

sec-approving this. Since it is sec-low, we may not take it elsewhere.
Attachment #702957 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 17

6 years ago
You are right, this is only worth backporting as a crash fix.  (#45 on 19 when I looked before.)
https://hg.mozilla.org/mozilla-central/rev/fa7dc7b5ca52
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox21: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Comment 20

6 years ago
Comment on attachment 702957 [details] [diff] [review]
enter the compartment

[Approval Request Comment]
Bug caused by (feature/regressing bug #): old
User impact if declined: non-security-sensitive crashes (#77 crash on 18, #27 on 19) with various addons.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

We've lived with this for a few release cycles, so not getting it in more places isn't the end of the world, but it would be nice to get it out faster.  I could easily land it on esr17 and b2g18, but it doesn't show up in the top crashes list for esr17, and it seems addon related, so it probably doesn't matter either way for b2g18.
Attachment #702957 - Flags: approval-mozilla-beta?
Attachment #702957 - Flags: approval-mozilla-aurora?
We can take this on aurora since it's low risk, not tracking though as it's a sec-low we aren't very concerned about and it doesn't meet ESR criteria.
status-firefox19: affected → wontfix
status-firefox-esr17: affected → wontfix
tracking-firefox19: ? → -
tracking-firefox20: ? → -
tracking-firefox-esr17: ? → -
Comment on attachment 702957 [details] [diff] [review]
enter the compartment

Please go ahead with aurora uplift.
Attachment #702957 - Flags: approval-mozilla-beta?
Attachment #702957 - Flags: approval-mozilla-beta-
Attachment #702957 - Flags: approval-mozilla-aurora?
Attachment #702957 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 23

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/5b565584a2fd
status-firefox20: affected → fixed
status-b2g18: --- → wontfix
status-firefox18: affected → wontfix

Updated

5 years ago
Whiteboard: [adv-main20+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.