Closed Bug 830389 Opened 7 years ago Closed 7 years ago

compartment mismatch in js_ValueToSource inside js::ReportIsNotFunction

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox21 + wontfix
firefox22 + fixed
firefox23 + fixed
firefox-esr17 22+ fixed
b2g18 22+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected
b2g-v1.1hd --- fixed

People

(Reporter: mccr8, Assigned: Benjamin)

References

Details

(Keywords: crash, sec-critical, Whiteboard: [qa-][adv-main22+][adv-esr1707+])

Attachments

(2 files, 1 obsolete file)

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
Attached patch enter compartment (obsolete) — Splinter Review
I'm not too famaliar with these types of bugs. Is this a fix?
Attachment #701925 - Flags: review?(continuation)
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.
Attachment #701925 - Flags: review?(continuation) → feedback+
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.
Attachment #703587 - Flags: review?(wmccloskey)
Attachment #703587 - Flags: review?(wmccloskey) → review+
Whiteboard: [leave open]
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
Whiteboard: [leave open]
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
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
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.
Attachment #701925 - Attachment is obsolete: true
Attachment #732620 - Flags: review?(wmccloskey)
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?
Attachment #732620 - Flags: review?(wmccloskey) → review+
Thanks for the quick review. I added a comment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/63453515a870
https://hg.mozilla.org/mozilla-central/rev/63453515a870
Assignee: general → benjamin
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.
tracking-b2g18: --- → ?
Flags: needinfo?(benjamin)
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
Attachment #732620 - Flags: approval-mozilla-beta?
Attachment #732620 - Flags: approval-mozilla-aurora?
Flags: needinfo?(benjamin)
Comment on attachment 732620 [details] [diff] [review]
enter compartment before calling ReportNotFunction


.
Attachment #732620 - Flags: approval-mozilla-esr17?
(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!
Attachment #732620 - Flags: approval-mozilla-beta?
Attachment #732620 - Flags: approval-mozilla-beta-
Attachment #732620 - Flags: approval-mozilla-aurora?
Attachment #732620 - Flags: approval-mozilla-aurora+
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.
Keywords: testcase-wanted
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.
Attachment #732620 - Flags: sec-approval?
(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.
Attachment #732620 - Flags: sec-approval?
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.
Attachment #732620 - Flags: approval-mozilla-esr17?
Attachment #732620 - Flags: approval-mozilla-esr17+
Attachment #732620 - Flags: approval-mozilla-b2g18+
Unless a test case appears, QA is going to assume unverifiable.
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-main22+][adv-esr1707+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.