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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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();

Comment 1

[Christian Holler (:decoder)]

Crashes look like null-derefs but the original crash had GC frames on the stack, so I'm marking this s-s until triaged.

Comment 2

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 3

[Christian Holler (:decoder)]

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.

Comment 4

[Christian Holler (:decoder)]

Needinfo from Dan based on comment 3.

Comment 5

[Dan Gohman [:sunfish]]

Attachment: 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.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: 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.

Comment 7

[Nicolas B. Pierron [:nbp]]

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.

Comment 8

[Dan Gohman [:sunfish]]

Attachment: 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.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 10

[Christian Holler (:decoder)]

Assuming sec-high based on comment 8.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 12

[Dan Gohman [:sunfish]]

Should I check in the testcase along with the fix, or should it be separate?

Comment 13

[Al Billings [:abillings - ex-MoCo]]

If this only affects Aurora and Trunk, go ahead and check in the testcase.

Comment 14

[Dan Gohman [:sunfish]]

Checked into mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3375b2da844e

Comment 15

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision aada0f74faf9).

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3375b2da844e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/96c4b6a68a48

Comment 18

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Dan Gohman [:sunfish]]

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.