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)

x86
Linux
defect
Not set
critical

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)

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.
Jon, this looks like some kind of use-after-sweep. Could you take a look at this? Thanks.
Flags: needinfo?(jcoppeard)
tracking since this is sec-critical. If this affects other versions please mark them affected.
Flags: needinfo?(jcoppeard)
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)
(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.
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)
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 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+
Attachment #8708430 - Flags: review?(jcoppeard) → review+
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?
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.
Tracking because it is a sec critical. Too late for 44 but happy to take a patch for 45.
(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.
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]
Attachment #8708426 - Flags: sec-approval? → sec-approval+
Whiteboard: [jsbugmon:update,bisect][checkin on 2/9] → [checkin on 2/9] [jsbugmon:update]
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)
(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.
Blocks: 1162986
No longer blocks: 1214126
Flags: needinfo?(jdemooij)
Jan, we will need uplift requests here too, thanks!
Flags: needinfo?(jdemooij)
Keywords: checkin-needed
Whiteboard: [checkin on 2/9] [jsbugmon:update] → [jsbugmon:update]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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 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+
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main45+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: