Closed
Bug 1238935
Opened 9 years ago
Closed 9 years ago
Crash [@ ObjectType] or Crash [@ CanInlineSetPropTypeCheck] with use-after-free
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla47
| Tracking | Status | |
|---|---|---|
| firefox42 | --- | wontfix |
| firefox43 | --- | wontfix |
| firefox44 | - | wontfix |
| firefox45 | + | fixed |
| firefox46 | + | fixed |
| firefox47 | + | verified |
| firefox-esr38 | --- | unaffected |
| firefox-esr45 | --- | fixed |
| b2g-v2.0 | --- | unaffected |
| b2g-v2.0M | --- | unaffected |
| b2g-v2.1 | --- | unaffected |
| b2g-v2.1S | --- | unaffected |
| b2g-v2.2 | --- | unaffected |
| b2g-v2.5 | --- | affected |
| b2g-v2.2r | --- | unaffected |
| b2g-master | --- | affected |
People
(Reporter: decoder, Assigned: jandem)
References
Details
(5 keywords, Whiteboard: [jsbugmon:update][adv-main45+])
Crash Data
Attachments
(2 files)
|
3.48 KB,
patch
|
jonco
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
|
2.37 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 6020a4cb41a7 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --disable-debug, run with --fuzzing-safe --no-threads --disable-oom-functions --baseline-eager --ion-eager):
setJitCompilerOption("ion.warmup.trigger", 20)
Native = function(k) {
d = k.initialize;
j = function(n, l, o) n.prototype[l] = o;
d.implement = function(m) {
for (n in m)
j(this, n, m[n]);
}
};
Native({ initialize: Function });
function $mixin(e) {
for (d = 1;;) {
var b = arguments[d];
for (c in b)
e[String];
e[c] = $unlink();
}
}
function $type(a) {
return typeof a;
}
function $unlink() {
if ($type(c) == "object")
b = {};
return b;
}
Function.implement({
extend(a) {
for (var b in a)
this[b] = a[b];
return this;
}
})
function Class(b) {
a = function() {
Object.reset(this);
}.extend(this);
a.implement(b);
return a;
}
Object.reset = function(a, c) {
if (c == null)
for (e in a)
Object.reset(a, e);
switch ($type([])) {
case "object":
d = function() {};
d.prototype = a[c];
var b = new d;
a[c] = b;
}
}
new Native({ initialize: Class });
Class.implement({
implement(a, d) {
if ($type(a) == "object")
for (e in a)
this.implement(e, a[e]);
f = Class.Mutators[a];
if (f)
f.call(this, d);
c = this.prototype;
switch ($type(d)) {
case "object":
var b = c[a];
if ($type(b) == "object")
$mixin(b, d);
c[a] = $unlink();
}
}
})
Class.Mutators = {
Extends(a) {
this.prototype = new a;
}
}
new Class({
Extends: new Class({ options: {} }),
options: { postVar : "" }
})
Backtrace:
Program received signal SIGSEGV, Segmentation fault.
ObjectType (obj=0xf66003b0) at js/src/vm/TypeInference-inl.h:146
#0 ObjectType (obj=0xf66003b0) at js/src/vm/TypeInference-inl.h:146
#1 GetValueType (val=...) at js/src/vm/TypeInference-inl.h:171
#2 CanInlineSetPropTypeCheck (obj=obj@entry=0xf654c310, id=..., checkTypeset=checkTypeset@entry=0xffffa980, val=...) at js/src/jit/IonCaches.cpp:3179
#3 0x08225e9f in IsPropertySetInlineable (checkTypeset=0xffffa980, needsTypeBarrier=true, pshape=..., id=..., obj=0xf654c310, val=...) at js/src/jit/IonCaches.cpp:3226
#4 CanAttachNativeSetProp (checkTypeset=0xffffa980, shape=..., holder=..., needsTypeBarrier=true, id=..., obj=..., cx=0xf7a72040, val=...) at js/src/jit/IonCaches.cpp:3308
#5 js::jit::SetPropertyIC::tryAttachNative (this=this@entry=0xf7a9c8c0, cx=cx@entry=0xf7a72040, outerScript=outerScript@entry=..., ion=ion@entry=0xf7a9c800, obj=obj@entry=..., id=id@entry=..., emitted=emitted@entry=0xffffaa70, tryNativeAddSlot=tryNativeAddSlot@entry=0xffffaa80) at js/src/jit/IonCaches.cpp:3526
#6 0x0822ca0a in js::jit::SetPropertyIC::tryAttachStub (this=this@entry=0xf7a9c8c0, cx=cx@entry=0xf7a72040, outerScript=outerScript@entry=..., ion=ion@entry=0xf7a9c800, obj=obj@entry=..., idval=idval@entry=..., value=value@entry=..., id=id@entry=..., emitted=emitted@entry=0xffffaa70, tryNativeAddSlot=tryNativeAddSlot@entry=0xffffaa80) at js/src/jit/IonCaches.cpp:3594
#7 0x0822cca3 in js::jit::SetPropertyIC::update (cx=0xf7a72040, outerScript=..., cacheIndex=0, obj=..., idval=..., value=...) at js/src/jit/IonCaches.cpp:3691
#8 0xf7fcb0f5 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
eax 0x2b2b2b2b 724249387
ebx 0x947c3dc 155698140
ecx 0xf65495e0 -162228768
edx 0xf64aabd0 -162878512
esi 0xf64aabf0 -162878480
edi 0xf654c310 -162217200
ebp 0xf6549000 4132737024
esp 0xffffa920 4294945056
eip 0x821222d <CanInlineSetPropTypeCheck(JSObject*, jsid, bool*)+589>
=> 0x821222d <CanInlineSetPropTypeCheck(JSObject*, jsid, bool*)+589>: testb $0x2,0xc(%eax)
0x8212231 <CanInlineSetPropTypeCheck(JSObject*, jsid, bool*)+593>: mov %eax,0xc(%esp)
The testcase crashes with pattern 0x2b2b2b2b indicating a use-after-free. Marking sec-critical until shown otherwise.
Comment 1•9 years ago
|
||
Jon, this looks like some kind of use-after-sweep. Could you take a look at this? Thanks.
Flags: needinfo?(jcoppeard)
Updated•9 years ago
|
tracking-firefox46:
--- → +
Comment 2•9 years ago
|
||
tracking since this is sec-critical. If this affects other versions please mark them affected.
Updated•9 years ago
|
Flags: needinfo?(jcoppeard)
Comment 3•9 years ago
|
||
We're ending up with a nursery object pointer in SetPropertyIC::value_. This is a ConstantOrRegister which contains an unbarriered Value. I guess this is never meant to contain nursery pointers but I'm not sure how this works in the JIT.
There's a comment in LIRGenerator::visitPostWriteBarrier that says constant nursery objects are lowered to a register, but that doesn't seem to help in this case. LIRGenerator::visitSetPropertyCache ends up creating a constant LAllocation for this value by calling LIRGeneratorShared::useBoxOrTypedOrConstant.
jandem do you know how this is supposed to work?
Flags: needinfo?(jdemooij)
| Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #3)
> We're ending up with a nursery object pointer in SetPropertyIC::value_.
> This is a ConstantOrRegister which contains an unbarriered Value. I guess
> this is never meant to contain nursery pointers but I'm not sure how this
> works in the JIT.
Ugh, good catch. I was wondering why we didn't find this sooner, but it takes some effort to get a nursery JSObject pointer in Ion.
I'll write a patch. Long shot but maybe this will fix some of those crashes on Treeherder.
| Assignee | ||
Comment 5•9 years ago
|
||
This ensures we pass nursery pointers in registers, so we don't store these pointers in SetPropertyIC.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8708426 -
Flags: review?(jcoppeard)
| Assignee | ||
Comment 6•9 years ago
|
||
This adds an assert to check we don't pass nursery pointers to LIR instructions for ICs. The assert fails without the previous patch.
Attachment #8708430 -
Flags: review?(jcoppeard)
Comment 7•9 years ago
|
||
Comment on attachment 8708426 [details] [diff] [review]
Part 1 - Don't pass nursery pointers to SetPropertyIC
Review of attachment 8708426 [details] [diff] [review]:
-----------------------------------------------------------------
Great, thanks for taking this!
Attachment #8708426 -
Flags: review?(jcoppeard) → review+
Updated•9 years ago
|
Attachment #8708430 -
Flags: review?(jcoppeard) → review+
| Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8708426 [details] [diff] [review]
Part 1 - Don't pass nursery pointers to SetPropertyIC
[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
Not easily. It's pretty hard to get a nursery pointer baked into Ion code and then make it crash here.
* 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?
Firefox 41+.
* If not all supported branches, which bug introduced the flaw?
Bug 1162986.
* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be trivial.
* How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #8708426 -
Flags: sec-approval?
Updated•9 years ago
|
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-v2.2r:
--- → unaffected
status-b2g-v2.5:
--- → affected
status-b2g-master:
--- → affected
status-firefox42:
--- → wontfix
status-firefox43:
--- → wontfix
status-firefox44:
--- → affected
status-firefox45:
--- → affected
status-firefox-esr38:
--- → unaffected
status-firefox-esr45:
--- → affected
tracking-firefox44:
--- → ?
tracking-firefox45:
--- → ?
Flags: in-testsuite?
| Assignee | ||
Comment 9•9 years ago
|
||
It's possible this will fix some intermittent GC-related crashes like bug 1236316 and bug 1239554. The logs indicate we're accessing swept nursery memory (0x2b2b2b2b) there as well.
Comment 10•9 years ago
|
||
Tracking because it is a sec critical.
Too late for 44 but happy to take a patch for 45.
| Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #9)
> It's possible this will fix some intermittent GC-related crashes like bug
> 1236316 and bug 1239554. The logs indicate we're accessing swept nursery
> memory (0x2b2b2b2b) there as well.
Nevermind, these crashes are entirely unrelated; fixing them in bug 1236316.
Comment 12•9 years ago
|
||
Sec-approval+ for checkin on Feb 9 or later. We can't take this now without exposing ourselves.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][checkin on 2/9]
Updated•9 years ago
|
Attachment #8708426 -
Flags: sec-approval? → sec-approval+
Updated•9 years ago
|
Whiteboard: [jsbugmon:update,bisect][checkin on 2/9] → [checkin on 2/9] [jsbugmon:update]
Comment 13•9 years ago
|
||
JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===
The "good" changeset has the timestamp "20151026030436" and the hash "51036998fbc8f650a2e8d6800b53ed58ab7887ad".
The "bad" changeset has the timestamp "20151026031135" and the hash "55a2293db8217d785808f1aeffde63f55cc11956".
Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=51036998fbc8f650a2e8d6800b53ed58ab7887ad&tochange=55a2293db8217d785808f1aeffde63f55cc11956
Jan, is bug 1214126 a likely regressor?
Blocks: 1214126
Flags: needinfo?(jdemooij)
| Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #14)
> Jan, is bug 1214126 a likely regressor?
Likely for this particular testcase, but the underlying bug is older. Goes back to bug 1162986 I think.
Updated•9 years ago
|
status-firefox47:
--- → affected
tracking-firefox47:
--- → +
Comment 16•9 years ago
|
||
Jan, we will need uplift requests here too, thanks!
Flags: needinfo?(jdemooij)
Keywords: checkin-needed
Whiteboard: [checkin on 2/9] [jsbugmon:update] → [jsbugmon:update]
Comment 17•9 years ago
|
||
Keywords: checkin-needed
Comment 18•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Comment 19•9 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
| Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8708426 [details] [diff] [review]
Part 1 - Don't pass nursery pointers to SetPropertyIC
Approval Request Comment
[Feature/regressing bug #]: Bug 1162986.
[User impact if declined]: Crashes, security bugs.
[Describe test coverage new/current, TreeHerder]: We have many tests exercising this code path.
[Risks and why]: Low risk; just disables a small optimization if we see a nursery-allocated object (which is uncommon).
[String/UUID change made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8708426 -
Flags: approval-mozilla-beta?
Attachment #8708426 -
Flags: approval-mozilla-aurora?
Comment 21•9 years ago
|
||
Comment on attachment 8708426 [details] [diff] [review]
Part 1 - Don't pass nursery pointers to SetPropertyIC
Fix a sec critical issue, taking it.
Should be in 45 beta 5.
Attachment #8708426 -
Flags: approval-mozilla-beta?
Attachment #8708426 -
Flags: approval-mozilla-beta+
Attachment #8708426 -
Flags: approval-mozilla-aurora?
Attachment #8708426 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Group: javascript-core-security → core-security-release
Updated•9 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main45+]
Updated•9 years ago
|
Group: core-security-release
Updated•8 years ago
|
Keywords: csectype-uaf
You need to log in
before you can comment on or make changes to this bug.
Description
•