Crash [@ ObjectType] or Crash [@ CanInlineSetPropTypeCheck] with use-after-free

VERIFIED FIXED in Firefox 45

Status

()

defect
--
critical
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla47
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

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

Details

(Whiteboard: [jsbugmon:update][adv-main45+], crash signature)

Attachments

(2 attachments)

Reporter

Description

3 years ago
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)
Assignee

Comment 4

3 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

3 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

3 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 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+
Assignee

Comment 8

3 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?
Assignee

Comment 9

3 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.
Tracking because it is a sec critical.
Too late for 44 but happy to take a patch for 45.
Assignee

Comment 11

3 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.
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+

Updated

3 years ago
Whiteboard: [jsbugmon:update,bisect][checkin on 2/9] → [checkin on 2/9] [jsbugmon:update]

Comment 13

3 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

3 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.
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]
https://hg.mozilla.org/mozilla-central/rev/b6613e975a08
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Updated

3 years ago
Status: RESOLVED → VERIFIED

Comment 19

3 years ago
JSBugMon: This bug has been automatically verified fixed.
Assignee

Comment 20

3 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 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.