1137624 - MArrayJoin misbehaves when array elements override toString

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

x = []
Array.prototype.unshift.call(x, x)
f = function(j) {
    if (j) {
        schedulegc()
    } else {
        this.eval("print(0)");
        s = x.join("")
    }
};
toString = function() {
    z = 0
    return function() {
        ++z
        f(z == 3)
    }
}
()
s = x.join()
print("FOO: " + s.replace(this, ""))
relazifyFunctions()
Array.prototype.push.call(x, this)
Array.prototype.sort.call(x, function(a, x, a) {
    x | 7
})

$ ./js-dbg-64-dm-nsprBuild-darwin-0a8b3b67715a --fuzzing-safe --no-threads --ion-eager testcase.js
0
FOO:
0
0

$ ./js-dbg-64-dm-nsprBuild-darwin-0a8b3b67715a --fuzzing-safe --no-threads --baseline-eager testcase.js
0
FOO:
0

Tested this on m-c rev 0a8b3b67715a.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh ~/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build -R ~/trees/mozilla-central" -r 0a8b3b67715a

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/eb6e90404b76
parent:      b86864fd9d60
user:        Jan de Mooij
date:        Sat Jan 17 12:54:03 2015 +0100
summary:     Bug 1116760 - Add a shell function to test function relazification. r=till

Jan, bug 1116760 probably exposed this issue. What do you think?

Comment 1

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:

Here's a simpler testcase:

x = ["a", {toString() { s = x.join(""); }}];
x.join("");
x.join("");
print("end");

This prints "end" without any flags, doesn't print anything with --ion-eager --no-threads. The problem is in jit::ArrayJoin:

    if (detector.foundCycle())
        return nullptr;

This is wrong. Also, why does jit::ArrayJoin mostly duplicate what's in js::ArrayJoin?

Furthermore, there's another problem with MArrayJoin, it's not marked as effectful but it can invoke toString. We fail this testcase with --no-threads:

function f() {
    var x = 0;
    var a = [{toString: () => x++}];
    for (var i=0; i<10000; i++)
	a.join("");
    assertEq(x, 10000);
}
f();

Nicolas can you fix this?

Comment 2

[Jan de Mooij [:jandem]]

Just realized part 2 of comment 1 is bad: toString can neuter array buffers for instance to trick JIT code into UAF or bounds check bugs.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Shell test

This test shows it's possible to access arbitrary memory.

$ js --no-threads test.js
test.js:27:4 Error: Assertion failed: got -842150451, expected (void 0)

Comment 4

[Lawrence Mandel [:lmandel] (use needinfo)]

sec-crit - tracking across the board.

Bug 1116760 that is referenced in comment 0 was fixed in 38 but 36+ are marked as affected. Does this affect ESR31?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #4)
> Bug 1116760 that is referenced in comment 0 was fixed in 38 but 36+ are
> marked as affected. Does this affect ESR31?

Comment 0 is wrong, this is a regression from bug 977966 so 34+.

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment 5 means ESR31 is unaffected.

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> This is wrong. Also, why does jit::ArrayJoin mostly duplicate what's in
> js::ArrayJoin?

We were not able to reuse most of the code, because of the spec-order in which the arguments are decoded, which is unfortunately observable in JavaScript, and would be different.

And I forgot to consider other cases than Array of strings, which was the focus of the initial optimization.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Disable Array.join optimization.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8574740 [details] [diff] [review]
Disable Array.join optimization.

Review of attachment 8574740 [details] [diff] [review]:
-----------------------------------------------------------------

Please file a follow-up bug (or "leave-open" this bug) to remove/fix inlineArrayJoin, MArrayJoin etc.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Remove ArrayJoin code duplication, and use a correct alias set.

This fix the issue synthezised in comment 1.

This patch should land on nightly once the previous patch made it to all
affected branches.

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: Remove ArrayJoin code duplication, and use a correct alias set.

Delta:
 - Fix the test case to actually fail without this modification, and
   without printing anything.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8574740 [details] [diff] [review]
Disable Array.join optimization.

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

I don't know, but apparently Jan managed to do so in comment 3.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

This does not highlight toString, but do highlight Array.join.

> Which older supported branches are affected by this flaw?

34+

> If not all supported branches, which bug introduced the flaw?

Bug 977966

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

These 2 line patch should apply to all branches.

> How likely is this patch to cause regressions; how much testing does it need?

This patch is unlikely to cause any regression.

The only reason for Bug 977966 is to provide the ground work before Bug 1112537.  So, we should not even expect any performance regressions.

Comment 13

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

Assigning to Nicolas since he has some patches up.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8575292 [details] [diff] [review]
Remove ArrayJoin code duplication, and use a correct alias set.

Review of attachment 8575292 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsarray.cpp
@@ +1001,5 @@
>      return true;
>  }
>  
>  template <bool Locale>
> +bool

Nit: static bool

@@ +1096,5 @@
>  
>      // Step 11
>      JSString *str = sb.finishString();
>      if (!str)
>          return nullptr;

return false;

Comment 15

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk.

We should get this on Aurora and Beta.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

We need to get this in this week, ideally (and Aurora and Beta patches as well).

Comment 17

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f266c100d831

I am removing the checkin-needed, as we should not land the second patch before backporting the first patch on all branches.

(In reply to Al Billings [:abillings] from comment #16)
> We need to get this in this week, ideally (and Aurora and Beta patches as
> well).

The first patch should apply cleanly on all branches.
I will test and upload new one if this is not the case.

Comment 18

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8574740 [details] [diff] [review]
Disable Array.join optimization.

Approval Request Comment (see comment 12)
[Feature/regressing bug #]:
Bug 977966

[User impact if declined]:
see comment 3.

[Describe test coverage new/current, TreeHerder]:
landed on inbound.

[Risks and why]:
Low, it disable an optimization which is not yet heavily used.

[String/UUID change made/needed]:
N/A

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f266c100d831

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc647f26739b
https://hg.mozilla.org/releases/mozilla-beta/rev/968fa2b32612

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/968fa2b32612
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/1cbcf829623f

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/1cbcf829623f

Comment 23

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/631fef632ea8 (gecko 46)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/631fef632ea8