Closed Bug 905166 Opened 9 years ago Closed 9 years ago

Crash [@ IsObjectValueInCompartment] or Opt-Crash [@ GetValueType]


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox24 --- unaffected
firefox25 + fixed
firefox26 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected


(Reporter: decoder, Assigned: sunfish)


(Keywords: crash, sec-high, testcase, Whiteboard: [jsbugmon:update,ignore])

Crash Data


(3 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision 3c61cc01a3b1 (run with --fuzzing-safe --ion-eager):

function test() {
  function LastIndexOf(s) {
     x = Math.min(Math.max(Math.pow(-1, 0.5), 0), s.length);
     0 <= x;
} test();
Crashes look like null-derefs but the original crash had GC frames on the stack, so I'm marking this s-s until triaged.
Crash Signature: [@ IsObjectValueInCompartment] or Opt-Crash [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
Whiteboard: [jsbugmon:update,bisect]
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
user:        Dan Gohman
date:        Thu Aug 01 13:39:28 2013 -0700
summary:     Bug 898468 - IonMonkey: Optimize min/max for the case where RangeAnalysis can prove that there are no NaNs. r=evilpies

This iteration took 350.052 seconds to run.
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
Needinfo from Dan based on comment 3.
Flags: needinfo?(sunfish)
Attached patch ported.patchSplinter Review
I will investigate shortly.

In case it is helpful, here is a forward-ported of the offending patch to trunk (as there have been several changes to the tree in between). With this patch reverse-applied on trunk, I don't see the crash.
Attached patch fix.patch (obsolete) — Splinter Review
The real bug is in Range::min in RangeAnalysis.cpp, introduced in 199bda69b2d9 for bug 889451. It was not computing the correct range for a Math.min call which could have a NaN operand.
Assignee: general → sunfish
Attachment #790385 - Flags: review?(nicolas.b.pierron)
Flags: needinfo?(sunfish)
Comment on attachment 790385 [details] [diff] [review]

Review of attachment 790385 [details] [diff] [review]:

Is that your first security issue on Bugzilla?
Before landing, you need to ask for sec-approval, and follow the following link[1] or what the release driver (such as :abillings) are telling you.


::: js/src/jit-test/tests/ion/bug905166.js
@@ +1,2 @@
> +function test() {
> +  LastIndexOf("hello");  

nit: remove trailing white-space.
Attachment #790385 - Flags: review?(nicolas.b.pierron) → review+
Attached patch fix.patchSplinter Review
[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The bug allows an attacker to construct a NaN value with arbitrary bits in its mantissa set. If I understand correctly how SpiderMonkey uses NaN-encoded values, this might allow an attacker who somehow has knowledge of the virtual address of security-relevant data to construct what would appear to be a reference to a boxed value at that address, which I assume could be used to modify data at or around that address. I can't comment on how easy that would be though.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The included testcase demonstrates a way to build a NaN value with arbitrary bits in its mantissa set.

Which older supported branches are affected by this flaw?

Any branch/merge from mozilla-central as of 199bda69b2d9 (circa Wed, 03 Jul 2013) would contain the flaw.

If not all supported branches, which bug introduced the flaw?

The actual bug was introduced in mozilla-central 199bda69b2d9. It's unknown if the flaw is observable without the additional changes introduced in mozilla-central f2f06acd92b5 (which is the revision turned up by the bisection).

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I don't have backports, but it would be very easy to create them for any branch or revision which needs it.

How likely is this patch to cause regressions; how much testing does it need?

It's unlikely to cause regressions. I don't expect it to need anything more than what would ordinarily be run for a JavaScript JIT change.
Attachment #790385 - Attachment is obsolete: true
Attachment #790421 - Flags: sec-approval?
We need a security rating on this issue before it can go in.

Also, as a security bug, tests shouldn't go in at the same time unless it is trunk only or we can check tests into all affected branches. If this affected release, texts would be split out and checked out a period of time after we ship a release with this fix. 

Based on dates, I'm guessing that this affects trunk and aurora? If that's the case, we might be able to check this in (with test) if we approve it for Aurora at the same time. I'm inclined to do this.
Assuming sec-high based on comment 8.
Keywords: sec-high
Comment on attachment 790421 [details] [diff] [review]

Ok. Sec-approval+ for trunk. Once it is in there and everything is green, please check it into aurora as well.
Attachment #790421 - Flags: sec-approval?
Attachment #790421 - Flags: sec-approval+
Attachment #790421 - Flags: approval-mozilla-aurora+
Should I check in the testcase along with the fix, or should it be separate?
Flags: needinfo?(abillings)
If this only affects Aurora and Trunk, go ahead and check in the testcase.
Flags: needinfo?(abillings)
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision aada0f74faf9).
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
JSBugMon: This bug has been automatically verified fixed.
FYI, I've now added a debugging option for verifying range analysis results, and I can confirm that it catches this bug. See bug 894813 for details.
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
Group: core-security
You need to log in before you can comment on or make changes to this bug.