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.

All 3 of these crashes have Babylon Toolbar and Free Download Manager.

Try run looked okay: https://tbpl.mozilla.org/?tree=Try&rev=4411b77b1b4d More stacks, now that the compartment checker has propagated to Aurora: https://crash-stats.mozilla.com/report/index/9380ab36-54ed-4162-8e59-22d9e2130116 https://crash-stats.mozilla.com/report/index/629ef0c2-0b43-4302-a0cb-39b972130116 https://crash-stats.mozilla.com/report/index/cb7aa21e-fb6c-4108-a729-80eac2130115 https://crash-stats.mozilla.com/report/index/90d338dc-f33e-4e53-aceb-26be22130112 https://crash-stats.mozilla.com/report/index/d9fdfa7e-b7e3-47c9-b6a7-35e1b2130112 These look similar. All but 1 have Babylon and FDM.

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.

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.

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.

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

Sure.

Comment on attachment 702957 [details] [diff] [review] enter the compartment r=me

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.

Comment on attachment 702957 [details] [diff] [review] enter the compartment sec-approving this. Since it is sec-low, we may not take it elsewhere.

You are right, this is only worth backporting as a crash fix. (#45 on 19 when I looked before.)

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.

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.