Closed Bug 879723 Opened 11 years ago Closed 11 years ago

Crash [@ check] or Opt-Crash [@ js::ToNumberSlow] with __proto__ and Proxy

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 + verified
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: decoder, Assigned: shu)

References

Details

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

Crash Data

Attachments

(3 files)

The following testcase crashes on mozilla-central revision 8f9ba85eb61c (no options required):


try {
  this.__defineGetter__('x',(Function('for(z=0;z<6;z++)(x)')))
  var p = Proxy.create({});
  Object.prototype.__proto__ = p;
  for (var a in x) {}
} catch(exc2) {}
try {
  new x(0X78 );
} catch(exc3) {}
var z = "";
function f() {}
f(x.Float64Array.length, 1)
I marked this s-s because we recently had a security bug with exactly the same signature where ints and strings were confused with each other.
Crash Signature: [@ check] or Opt-Crash [@ js::ToNumberSlow] → [@ check] [@ js::ToNumberSlow]
Whiteboard: [jsbugmon:update,bisect]
Keywords: sec-high
Crash Signature: [@ check] [@ js::ToNumberSlow] → [@ check] [@ js::ToNumberSlow]
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:
changeset:   http://hg.mozilla.org/mozilla-central/rev/ed26fdbe8444
user:        Shu-yu Guo
date:        Sat May 04 20:53:21 2013 -0700
summary:     Bug 646597 - Make functions made by the Function constructor compile-and-go. Most of patch was originally written by jorendorff. (r=luke)

This iteration took 332.794 seconds to run.
Crash Signature: [@ check] [@ js::ToNumberSlow] → [@ check] [@ js::ToNumberSlow]
Needinfo from Shu based on comment 3 :)
Flags: needinfo?(shu)
Flags: needinfo?(shu)
Attached patch fix for auroraSplinter Review
What's going on is that:

1) We're specializing the setgname for z in the Function constructor'd function to not emit the store for the Value type tag. In the meantime, the __proto__ assignment causes the global object's prototype's TypeObject to be marked unknownProperties.

2) After we specialize the setgname, the getgname ends up propagating the inherited types for z, causing the HeapTypeSet we specialized on in 1) to be unknown.

3) The try-catch block finishes. We now set z="", whereas it used to always be 0. This would have caused a recompilation usually, but 2) means that the HeapTypeSet is already unknown, so the string type is considered already in the type set, so we don't invalidate

4) We re-enter Ion code for the Function constructor'd function and store 0 into the z slot on the global without storing the type tag.

5) Crash sometime later when we try to use the incorrectly tagged value.
Assignee: general → shu
Attachment #760096 - Flags: review?(bhackett1024)
Attachment #760096 - Flags: review?(bhackett1024) → review+
Attached patch fix for tipSplinter Review
[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not obvious, but doable.

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

Which older supported branches are affected by this flaw? 23.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Use the patch above, the method got renamed on tip.

How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #760232 - Flags: sec-approval?
Attachment #760232 - Flags: review+
Attachment #760096 - Attachment description: fix → fix for aurora
Comment on attachment 760232 [details] [diff] [review]
fix for tip

sec-approval+ for trunk. Please check this in.
Attachment #760232 - Flags: sec-approval? → sec-approval+
Comment on attachment 760096 [details] [diff] [review]
fix for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bisection says 646597, but ultimately 804676
User impact if declined: Unsafe reinterpret casts in Ion due to specializing on wrong type
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch:
Attachment #760096 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7ecbdd658637
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Status: RESOLVED → VERIFIED
Crash Signature: [@ check] [@ js::ToNumberSlow] → [@ check] [@ js::ToNumberSlow]
JSBugMon: This bug has been automatically verified fixed.
Crash Signature: [@ check] [@ js::ToNumberSlow] → [@ check] [@ js::ToNumberSlow]
Attachment #760096 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: