Closed
Bug 879723
Opened 12 years ago
Closed 11 years ago
Crash [@ check] or Opt-Crash [@ js::ToNumberSlow] with __proto__ and Proxy
Categories
(Core :: JavaScript Engine, defect)
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)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bhackett1024
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
shu
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
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]
Reporter | ||
Updated•12 years ago
|
Crash Signature: [@ check]
[@ js::ToNumberSlow] → [@ check]
[@ js::ToNumberSlow]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter | ||
Comment 3•12 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/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.
Reporter | ||
Updated•11 years ago
|
Crash Signature: [@ check]
[@ js::ToNumberSlow] → [@ check]
[@ js::ToNumberSlow]
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(shu)
Assignee | ||
Comment 5•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #760096 -
Flags: review?(bhackett1024) → review+
Updated•11 years ago
|
Blocks: 646597
status-b2g18:
--- → unaffected
status-firefox22:
--- → unaffected
status-firefox23:
--- → affected
status-firefox24:
--- → affected
status-firefox-esr17:
--- → unaffected
tracking-firefox23:
--- → ?
tracking-firefox24:
--- → ?
Assignee | ||
Comment 6•11 years ago
|
||
[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+
Assignee | ||
Updated•11 years ago
|
Attachment #760096 -
Attachment description: fix → fix for aurora
Comment 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
Assignee | ||
Comment 9•11 years ago
|
||
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?
Comment 10•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Reporter | ||
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Crash Signature: [@ check]
[@ js::ToNumberSlow] → [@ check]
[@ js::ToNumberSlow]
Reporter | ||
Comment 11•11 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•11 years ago
|
Crash Signature: [@ check]
[@ js::ToNumberSlow] → [@ check]
[@ js::ToNumberSlow]
Updated•11 years ago
|
Attachment #760096 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 12•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main23-]
Reporter | ||
Updated•11 years ago
|
status-b2g18:
unaffected → ---
status-firefox24:
verified → ---
status-firefox-esr17:
unaffected → ---
tracking-firefox24:
+ → ---
Updated•11 years ago
|
status-b2g18:
--- → unaffected
status-firefox24:
--- → verified
status-firefox-esr17:
--- → unaffected
tracking-firefox24:
--- → +
Updated•11 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•