Closed
Bug 1137624
Opened 10 years ago
Closed 10 years ago
MArrayJoin misbehaves when array elements override toString
Categories
(Core :: JavaScript Engine: JIT, defect)
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
Details
(Keywords: regression, sec-critical, testcase, Whiteboard: [adv-main37+])
Attachments
(3 files, 1 obsolete file)
554 bytes,
text/javascript
|
Details | |
1.07 KB,
patch
|
jandem
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
10.65 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•10 years ago
|
||
[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?
status-firefox36:
--- → affected
status-firefox37:
--- → affected
status-firefox38:
--- → affected
tracking-firefox39:
--- → ?
Flags: needinfo?(till)
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(jdemooij)
Comment 2•10 years ago
|
||
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.
Updated•10 years ago
|
Summary: Differential Testing: Different output message involving relazifyFunctions → MArrayJoin misbehaves when array elements override toString
Comment 3•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8570453 -
Attachment mime type: text/x-c → text/javascript
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
(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)
Comment 6•10 years ago
|
||
Comment 5 means ESR31 is unaffected.
status-firefox-esr31:
--- → unaffected
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Updated•10 years ago
|
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → affected
status-b2g-v2.1S:
--- → affected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8574740 -
Flags: review?(jdemooij)
Comment 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(nicolas.b.pierron)
Attachment #8575292 -
Flags: review?(jdemooij)
Assignee | ||
Comment 12•10 years ago
|
||
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?
![]() |
Reporter | |
Comment 13•10 years ago
|
||
Assigning to Nicolas since he has some patches up.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Comment 14•10 years ago
|
||
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+
Comment 15•10 years ago
|
||
sec-approval+ for trunk. We should get this on Aurora and Beta.
Updated•10 years ago
|
Attachment #8574740 -
Flags: sec-approval? → sec-approval+
Comment 16•10 years ago
|
||
We need to get this in this week, ideally (and Aurora and Beta patches as well).
Flags: needinfo?(nicolas.b.pierron)
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 17•10 years ago
|
||
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
Assignee | ||
Comment 18•10 years ago
|
||
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?
Updated•10 years ago
|
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 20•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc647f26739b https://hg.mozilla.org/releases/mozilla-beta/rev/968fa2b32612
Comment 21•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/968fa2b32612 https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/1cbcf829623f
Updated•10 years ago
|
Whiteboard: [adv-main37+]
Updated•9 years ago
|
Group: core-security → core-security-release
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/631fef632ea8 (gecko 46)
Flags: needinfo?(nicolas.b.pierron) → in-testsuite+
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•