Closed Bug 830595 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 - wontfix
firefox20 - fixed
firefox21 + fixed
firefox-esr10 --- wontfix
firefox-esr17 - wontfix
b2g18 --- wontfix

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Keywords: crash, csectype-dos, sec-low, Whiteboard: [adv-main20+])

Crash Data

Attachments

(1 file)

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
I'm not sure how to make a test case for this. I guess I'll investigate the stream listener stuff.
Assignee: nobody → continuation
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?)
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...
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)
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.
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.
Comment on attachment 702957 [details] [diff] [review]
enter the compartment

Re-requesting review in light of comment 9.
Attachment #702957 - Flags: review?(bzbarsky)
mccr8, should we add that signature to this bug then (and add the crash keyword), so it gets linked with this?
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+
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?
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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
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+
Whiteboard: [adv-main20+]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.