I came across this stack report: https://crash-stats.mozilla.com/report/index/c8be7cc5-f067-4ec6-9eec-34bac2130112 I'm not sure what the real problem is here, but the immediate problem appears to be that js_ValueToSource turns a value into an object, then runs code on it without entering the compartment. 0 js::CompartmentChecker::fail js/src/jscntxtinlines.h:205 1 js::InvokeKernel js/src/jsinterp.cpp:391 2 js::Invoke js/src/jsinterp.cpp:439 3 js_ValueToSource js/src/jsstr.cpp:3537 4 js::DecompileValueGenerator js/src/jsopcode.cpp:6260 5 js_ReportValueErrorFlags js/src/jscntxt.cpp:1014 6 js::ReportIsNotFunction js/src/jsinterp.cpp:271 7 js::InvokeKernel js/src/jsinterp.cpp:370 8 js::Interpret js/src/jsinterp.cpp:2368 9 js::RunScript js/src/jsinterp.cpp:340 10 UncachedInlineCall js/src/methodjit/InvokeHelpers.cpp:372 11 js::mjit::stubs::UncachedCallHelper js/src/methodjit/InvokeHelpers.cpp:460 12 js::mjit::CallCompiler::update js/src/methodjit/MonoIC.cpp:1236 13 js::mjit::ic::Call js/src/methodjit/MonoIC.cpp:1317 14 js::mjit::EnterMethodJIT js/src/methodjit/MethodJIT.cpp:1041 15 js::RunScript js/src/jsinterp.cpp:345

I'm not too famaliar with these types of bugs. Is this a fix?

Comment on attachment 701925 [details] [diff] [review] enter compartment Review of attachment 701925 [details] [diff] [review]: ----------------------------------------------------------------- That looks okay to me, but you should get a review from somebody who is more familiar with this part of the code.

This seems wrong to me. Ideally, we would have an assertion at the top of js_ValueToSource to ensure that v is in the cx->compartment compartment. I suspect that that assertion would fire instead, although maybe not. However, we shouldn't need to enter any compartments in this code, since it should all be same-compartment. Maybe we should add that assertion and see what happens to these crashes.

Still happening (?) https://crash-stats.mozilla.com/report/index/185620e6-57b0-4bad-b90d-016042130130

That's a different stack. It doesn't have js_ValueToSource or js::ReportIsNotFunction anywhere in it. I've only seen this crash a single time, so it may make sense to just mark this bug incomplete, given that it seems like garbage floating in from elsewhere.

Easy to reopen if the assertion ever fails.

This happened again. The stack is similar, though the error was triggered inside JS_CloneFunctionObject instead. I don't know if this needs to be sec-critical given that it only seems to happen every few months, but it would be nice to understand why this is happening. As you might expect, the assertion at the top of the function fires immediately, instead of later at the invoke. Does RootedValue not check that the value is in the right compartment? This error code could also be changed to just always do a runtime abort when it detects compartment problems, or bail or or something. https://crash-stats.mozilla.com/report/index/ba3df980-8e68-4083-930b-4ddbd2130401

I think the problem here is that JS_CloneFunctionObject can handle functions from different comparments. Most of the function handles it correctly, but in the error case that the argument is a function, it fails to enter the compartment before invoking ReportIsNotFunction.

Comment on attachment 732620 [details] [diff] [review] enter compartment before calling ReportNotFunction Review of attachment 732620 [details] [diff] [review]: ----------------------------------------------------------------- Weird. Can you leave the comment?

Thanks for the quick review. I added a comment. https://hg.mozilla.org/integration/mozilla-inbound/rev/63453515a870

Btw, if this ValueToSource assertion reoccurs, I think a new bug should be filed, since it's likely a different underlying issue.

Security bug fixes that are not introduced by a known regressor need sec-approval before landing https://wiki.mozilla.org/Security/Bug_Approval_Process We are a little more than a week away from shipping Fx21 and also need to know if this vulnerability affects the ESR 17 branch. Please get beta landing approval ASAP.

Comment on attachment 732620 [details] [diff] [review] enter compartment before calling ReportNotFunction [Approval Request Comment] Bug caused by (feature/regressing bug #): NA User impact if declined: This bug seems to have only caused 1 or 2 crashes. That said, being a compartment mistach, it is a theoretical security hole. Testing completed (on m-c, etc.): on m-c for a long time Risk to taking this patch (and alternatives if risky): NA String or IDL/UUID changes made by this patch: NA

Comment on attachment 732620 [details] [diff] [review] enter compartment before calling ReportNotFunction .

(In reply to :Benjamin Peterson from comment #17) > Comment on attachment 732620 [details] [diff] [review] > enter compartment before calling ReportNotFunction > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): NA > User impact if declined: This bug seems to have only caused 1 or 2 crashes. > That said, being a compartment mistach, it is a theoretical security hole. > Testing completed (on m-c, etc.): on m-c for a long time > Risk to taking this patch (and alternatives if risky): NA The patch looks safe and simple but it will be very helpful if you can please call out the risk explicitly, given we are in our last beta and very close to release and do not want to induce scope for any new regressions at this point in the 21 cycle?Thanks. > String or IDL/UUID changes made by this patch: NA

(In reply to bhavana bajaj [:bajaj] from comment #19) > The patch looks safe and simple but it will be very helpful if you can > please call out the risk explicitly, given we are in our last beta and very > close to release and do not want to induce scope for any new regressions at > this point in the 21 cycle?Thanks. I think it's about as safe a patch as you can get. It only has effect in an uncommon error case.

(In reply to :Benjamin Peterson from comment #20) > (In reply to bhavana bajaj [:bajaj] from comment #19) > > The patch looks safe and simple but it will be very helpful if you can > > please call out the risk explicitly, given we are in our last beta and very > > close to release and do not want to induce scope for any new regressions at > > this point in the 21 cycle?Thanks. > > I think it's about as safe a patch as you can get. It only has effect in an > uncommon error case. I discussed this offline with :dveditz and the only reason I was considering this for final beta was due to comment #16.But based on my recent conversation I am only going to approve this for aurora at this time as we typically do not tend to take patches in our final beta if not necessary.In addition to this being a very old bug as esr 17 is affected,it is our understanding that the bug comments or the code that is being checked-in do not paint a bull's eye in the security problem which otherwise could be a strong driver to take into beta . Please feel free to renominate if there is a disagreement here and make sure to nominate such bugs for sec-approval before landing.Thanks!

I think that's a good assesment.

We've got a fix at this point, probably don't need the reduced test case right now as a result.

Comment on attachment 732620 [details] [diff] [review] enter compartment before calling ReportNotFunction [Security approval request comment] I don't have any comment except to say this is at best a theoretical security hole.

(In reply to :Benjamin Peterson from comment #24) > Comment on attachment 732620 [details] [diff] [review] > enter compartment before calling ReportNotFunction > > [Security approval request comment] > I don't have any comment except to say this is at best a theoretical > security hole. Can you please fill out the form for sec-approval? Otherwise, this won't be approved.

Never mind. I didn't read up. *sigh* I see this went in without going through the process.

This will get esr17 landing approval after we ship 17.0.6 (so it will be in 17.0.7)

Comment on attachment 732620 [details] [diff] [review] enter compartment before calling ReportNotFunction Approving for B2G as well since this should apply cleanly.

https://hg.mozilla.org/releases/mozilla-esr17/rev/19c948221485 https://hg.mozilla.org/releases/mozilla-b2g18/rev/ec791aec2023

Unless a test case appears, QA is going to assume unverifiable.