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

VERIFIED FIXED in Firefox 25

Status

()

--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: sunfish)

Tracking

(Blocks: 1 bug, {crash, sec-high, testcase})

Trunk
mozilla26
x86_64
Linux
crash, sec-high, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox24 unaffected, firefox25+ fixed, firefox26+ fixed, firefox-esr17 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected)

Details

(Whiteboard: [jsbugmon:update,ignore], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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


function test() {
  LastIndexOf("hello");  
  LastIndexOf("hello");
  eval("");
  function LastIndexOf(s) {
     x = Math.min(Math.max(Math.pow(-1, 0.5), 0), s.length);
     0 <= x;
  }
} test();
(Reporter)

Comment 1

5 years ago
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]
(Reporter)

Comment 2

5 years ago
Created attachment 790222 [details]
[crash-signature] Machine-readable crash signature
(Reporter)

Updated

5 years ago
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 3

5 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/f2f06acd92b5
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.
(Reporter)

Updated

5 years ago
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
(Reporter)

Comment 4

5 years ago
Needinfo from Dan based on comment 3.
Flags: needinfo?(sunfish)
status-firefox24: --- → unaffected
status-firefox25: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox26: --- → +
(Assignee)

Comment 5

5 years ago
Created attachment 790334 [details] [diff] [review]
ported.patch

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.
(Assignee)

Comment 6

5 years ago
Created attachment 790385 [details] [diff] [review]
fix.patch

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]
fix.patch

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.

[1] https://wiki.mozilla.org/Security/Bug_Approval_Process

::: 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+
(Assignee)

Comment 8

5 years ago
Created attachment 790421 [details] [diff] [review]
fix.patch

[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.
status-firefox26: --- → affected
tracking-firefox25: --- → ?
(Reporter)

Comment 10

5 years ago
Assuming sec-high based on comment 8.
Keywords: sec-high
Comment on attachment 790421 [details] [diff] [review]
fix.patch

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+
tracking-firefox25: ? → +
(Assignee)

Comment 12

5 years ago
Should I check in the testcase along with the fix, or should it be separate?
(Assignee)

Updated

5 years ago
Flags: needinfo?(abillings)
If this only affects Aurora and Trunk, go ahead and check in the testcase.
Flags: needinfo?(abillings)
(Reporter)

Updated

5 years ago
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
(Reporter)

Comment 15

5 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision aada0f74faf9).
https://hg.mozilla.org/mozilla-central/rev/3375b2da844e
Status: NEW → RESOLVED
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
Last Resolved: 5 years ago
status-firefox26: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
https://hg.mozilla.org/releases/mozilla-aurora/rev/96c4b6a68a48
status-firefox25: affected → fixed
Flags: in-testsuite+
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
Crash Signature: [@ IsObjectValueInCompartment] [@ GetValueType] → [@ IsObjectValueInCompartment] [@ GetValueType]
(Reporter)

Comment 18

5 years ago
JSBugMon: This bug has been automatically verified fixed.
(Assignee)

Comment 19

5 years ago
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]
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → unaffected
Group: core-security
You need to log in before you can comment on or make changes to this bug.