Closed Bug 826392 Opened 7 years ago Closed 7 years ago

compartment checker fail in mozilla::plugins::parent::_evaluate

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED INVALID
Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 - wontfix
firefox21 --- fixed
firefox-esr17 - wontfix
b2g18 --- wontfix
b2g18-v1.0.0 --- wontfix

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Keywords: crash, sec-other, Whiteboard: [adv-main21-][fixed in 21 by 824864][probably bogus assertion])

I found 3 crashes like this when searching through some crash stacks for the cross-compartment pointer checker.

0 js::CompartmentChecker::fail 	js/src/jscntxtinlines.h:205
1 JS_GetStringCharsZAndLength 	js/src/jsapi.cpp:6055
2 JSValToNPVariant 	dom/plugins/base/nsJSNPRuntime.cpp:435
3 mozilla::plugins::parent::_evaluate 	dom/plugins/base/nsNPAPIPlugin.cpp:1599
4 mozilla::plugins::PluginScriptableObjectParent::AnswerNPN_Evaluate 	dom/plugins/ipc/PluginScriptableObjectParent.cpp:1197
5 mozilla::plugins::PPluginScriptableObjectParent::OnCallReceived 	obj-firefox/ipc/ipdl/PPluginScriptableObjectParent.cpp:776
6 mozilla::plugins::PPluginModuleParent::OnCallReceived 	obj-firefox/ipc/ipdl/PPluginModuleParent.cpp:1288

I'm not sure exactly what is going wrong, because the string comes from |scx->EvaluateStringWithValue|, where |scx| is the |GetScriptContextFromJSContext| of the context we're calling JS_GetStringCharsZAndLength on.

https://crash-stats.mozilla.com/report/index/b5621c05-db57-412e-bdb1-b475c2130101
https://crash-stats.mozilla.com/report/index/f97d5d0f-f881-4f45-b755-efad72121229
https://crash-stats.mozilla.com/report/index/995820a8-b2b8-40c5-b050-785ab2130101
(In reply to Andrew McCreight [:mccr8] from comment #0)

> I'm not sure exactly what is going wrong, because the string comes from
> |scx->EvaluateStringWithValue|, where |scx| is the
> |GetScriptContextFromJSContext| of the context we're calling
> JS_GetStringCharsZAndLength on.

That doesn't mean the cx is still in that compartment. In fact, it doesn't even mean that that cx is at the top of the cx stack anymore, because EvaluteStringWithValue does both a cx push (which includes an implicit scoped JS_SaveFrameChain/JS_RestoreFrameChain (temporarily making us look as if we'd entered no compartment at all)), and a JSAutoEnterCompartment.

I'm refatoring this code in bug 824864. But I think we probably need to enter the compartment of the returned jsval here. We might even want to push the cx, though I'm not sure about that.
Ah, good point!
Keywords: sec-critical
See also bug 803228 which could be related. I promised to try and brainstorm some testcases there but haven't yet :-(
Assignee: nobody → benjamin
Keywords: testcase-wanted
bsmedberg is presumbly super busy, and I can write a patch for this, but not a test case, so I'll grab this for now.
Assignee: benjamin → continuation
It looks like this is an old problem.
Bill suggested that this might be fixed by bug 824864, and indeed I haven't seen any crashes in recent Nightly builds.  Unfortunately, that patch is huge, so we probably don't want to backport it everywhere.

Bill said you shouldn't enter the compartment of a string (because you can end up in the atoms compartment), so the simple solution here isn't going to work.
Whiteboard: [fixed in 21 by 824864?]
Presumably we don't care about NPAPI plugins on b2g, and we're not going to get anything together in time for 19.
Not sure what release management is going to want to do here. Compartment failures can theoretically be exploited, but we don't have a specific way to reproduce this particular one. The "fix" in bug 824864 is bigger than we'd like to back-port to ESR-17 -- might even depend on other changes between 17 and 21 in unexpected ways.

We don't need to push this into Firefox 20, it's ESR-17 where we need to make the call since Firefox 21 will contain a fix (we think).

I guess we could look for this signature on ESR-17 and see if it even happens there.
Depends on: 824864
Keywords: crash
Yes, this relies on special release mode assertions that are only enabled on Nightly and Aurora.
Bill, is there any chance I could write a hacky string compartment entering thing that explicitly checks if it is the atoms compartment?  I'm not sure what else to do here.
I think we just need to change EvaluateStringWithValue to wrap the result its returning in whatever the current compartment is. I think that's all the fix in bug 824864 is buying us here. Maybe I'm wrong though. Bobby?
Well, there was also a fair amount of fallout from that bug. Can't we just add a call to JS_WrapValue after the call to scx->EvaluateString?
(In reply to Bobby Holley (:bholley) from comment #15)
> Well, there was also a fair amount of fallout from that bug. Can't we just
> add a call to JS_WrapValue after the call to scx->EvaluateString?

Yeah, that seems fine too.
As comment 10 mentions, we don't need to push this to FF 20 and since we haven't any indication (in crash reports) that this is even occurring on ESR we'll wontfix there and if it shows up we can look into options for safe backporting.
Whiteboard: [fixed in 21 by 824864?] → [fixed in 21 by 824864?][Reprioritize if we see ESR17 crashes]
Well, these crashes will never show up on ESR17, because they only show up on Nightly and Aurora because of special runtime checks we added there.  The suggested fix in comment 15 is probably small enough to consider backporting, but I'll worry about that when the patch exists.
Whiteboard: [fixed in 21 by 824864?][Reprioritize if we see ESR17 crashes] → [fixed in 21 by 824864]
Bill, in one of your zones patch, you just drop the same-compartment check on JS_GetStringCharsZAndLength, which is precisely the one we're trying to fix here.  Do you think it is probably safe to not care about the compartments on all branches?  If so, we can just won't fix this everywhere.

[1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=759585&attachment=714648
Flags: needinfo?(wmccloskey)
Okay, well, it is already wontfixed everywhere, but we could rest easy knowing that it isn't really a problem anywhere...
(In reply to Andrew McCreight [:mccr8] from comment #19)
> Bill, in one of your zones patch, you just drop the same-compartment check
> on JS_GetStringCharsZAndLength, which is precisely the one we're trying to
> fix here.  Do you think it is probably safe to not care about the
> compartments on all branches?  If so, we can just won't fix this everywhere.

Sure.
Flags: needinfo?(wmccloskey)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Whiteboard: [fixed in 21 by 824864] → [fixed in 21 by 824864][[probably bogus assertion]
Whiteboard: [fixed in 21 by 824864][[probably bogus assertion] → [fixed in 21 by 824864][probably bogus assertion]
Whiteboard: [fixed in 21 by 824864][probably bogus assertion] → [adv-main21-][fixed in 21 by 824864][probably bogus assertion]
Group: core-security
You need to log in before you can comment on or make changes to this bug.