Closed Bug 1137624 Opened 5 years ago Closed 5 years ago

MArrayJoin misbehaves when array elements override toString

Categories

(Core :: JavaScript Engine: JIT, defect, major)

x86_64
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- wontfix
firefox37 + fixed
firefox38 + fixed
firefox39 + fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: gkw, Assigned: nbp)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, sec-critical, testcase, Whiteboard: [adv-main37+])

Attachments

(3 files, 1 obsolete file)

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?
Flags: needinfo?(till)
Flags: needinfo?(jdemooij)
[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?
Blocks: 977966
No longer blocks: 1116760
Flags: needinfo?(till)
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(jdemooij)
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.
Group: core-security
Keywords: sec-critical
Summary: Differential Testing: Different output message involving relazifyFunctions → MArrayJoin misbehaves when array elements override toString
Attached file 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)
Attachment #8570453 - Attachment mime type: text/x-c → text/javascript
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?
Flags: needinfo?(jdemooij)
(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+.
Flags: needinfo?(jdemooij)
(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.
Attachment #8574740 - Flags: review?(jdemooij)
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.
Attachment #8574740 - Flags: review?(jdemooij) → review+
This fix the issue synthezised in comment 1.

This patch should land on nightly once the previous patch made it to all
affected branches.
Attachment #8575278 - Flags: review?(jdemooij)
Delta:
 - Fix the test case to actually fail without this modification, and
   without printing anything.
Attachment #8575278 - Attachment is obsolete: true
Attachment #8575278 - Flags: review?(jdemooij)
Flags: needinfo?(nicolas.b.pierron)
Attachment #8575292 - Flags: review?(jdemooij)
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.
Attachment #8574740 - Flags: sec-approval?
Assigning to Nicolas since he has some patches up.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
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;
Attachment #8575292 - Flags: review?(jdemooij) → review+
sec-approval+ for trunk.

We should get this on Aurora and Beta.
Attachment #8574740 - Flags: sec-approval? → sec-approval+
We need to get this in this week, ideally (and Aurora and Beta patches as well).
Flags: needinfo?(nicolas.b.pierron)
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.
Flags: needinfo?(nicolas.b.pierron)
Keywords: checkin-needed
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
Attachment #8574740 - Flags: approval-mozilla-beta?
Attachment #8574740 - Flags: approval-mozilla-aurora?
Attachment #8574740 - Flags: approval-mozilla-beta?
Attachment #8574740 - Flags: approval-mozilla-beta+
Attachment #8574740 - Flags: approval-mozilla-aurora?
Attachment #8574740 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/f266c100d831
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Whiteboard: [adv-main37+]
Group: core-security → core-security-release
Flags: needinfo?(nicolas.b.pierron)
Blocks: 1112537
https://hg.mozilla.org/integration/mozilla-inbound/rev/631fef632ea8 (gecko 46)
Flags: needinfo?(nicolas.b.pierron) → in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.