1268138 - Unsafe use of String.prototype.split in Intl.js code

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[André Bargull [:anba]]

mozilla-central revision ab0044bfa1df


Test case 1:
---
String.prototype[Symbol.split] = function(s, limit) { return [""]; };
Intl.DateTimeFormat("de", {}).format()
---

fails with:
---
Self-hosted JavaScript assertion info: "/home/andre/git/mozilla-central/js/src/builtin/Intl.js:114: recombination produced an invalid language tag"
Assertion failure: false, at /home/andre/git/mozilla-central/js/src/vm/SelfHosting.cpp:388

Thread 1 "mozjs-debug" received signal SIGSEGV, Segmentation fault.
0x0000000000d66b6f in intrinsic_AssertionFailed (cx=0x7ffff691a000, argc=1, vp=0x7ffff40eb5f8) at /home/andre/git/mozilla-central/js/src/vm/SelfHosting.cpp:388
388	    MOZ_ASSERT(false);
---



Test case 2:
---
String.prototype[Symbol.split] = function(s, limit) { return ["x-foo"]; };
Intl.DateTimeFormat("de", {}).format()
---

fails with:
---
Self-hosted JavaScript assertion info: "/home/andre/git/mozilla-central/js/src/builtin/Intl.js:596: the computed default locale must be canonical"
Assertion failure: false, at /home/andre/git/mozilla-central/js/src/vm/SelfHosting.cpp:388

Thread 1 "mozjs-debug" received signal SIGSEGV, Segmentation fault.
0x0000000000d66b6f in intrinsic_AssertionFailed (cx=0x7ffff691a000, argc=1, vp=0x7ffff40eb718) at /home/andre/git/mozilla-central/js/src/vm/SelfHosting.cpp:388
388	    MOZ_ASSERT(false);
---



Test case 3:
---
String.prototype[Symbol.split] = function(s, limit) { return ["de-u-co"]; };
Intl.DateTimeFormat("de", {}).format()
---

fails with:
---
Self-hosted JavaScript assertion info: "/home/andre/git/mozilla-central/js/src/builtin/Intl.js:688: non-canonical BestAvailableLocale locale"
Assertion failure: false, at /home/andre/git/mozilla-central/js/src/vm/SelfHosting.cpp:388

Thread 1 "mozjs-debug" received signal SIGSEGV, Segmentation fault.
0x0000000000d66b6f in intrinsic_AssertionFailed (cx=0x7ffff691a000, argc=1, vp=0x7ffff40eb678) at /home/andre/git/mozilla-central/js/src/vm/SelfHosting.cpp:388
388	    MOZ_ASSERT(false);
---

Comment 1

[André Bargull [:anba]]

Relevant spec change: https://github.com/tc39/ecma402/pull/70/commits/7cb955c1f6bf00c4e218c6b87ee247a0e320187e

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Call StringSplitString directly for internal use.

Changed String_split to StringSplitString, that is not affected by String.prototype[Symbol.split] change.
it receives 2 strings, the target string and the separator.  in all cases, separator is a string.
about the target string, I applied ToString just to be sure.  (might be redundant)

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8746300 [details] [diff] [review]
Call StringSplitString directly for internal use.

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

Bah. Anba, thanks for reporting. Arai, thanks for fixing.

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8746300 [details] [diff] [review]
Call StringSplitString directly for internal use.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
I don't think this issue is exploitable, but asking sec-approval just in case, as this is not nightly-only.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The testcase describes how to hit the issue, but it just results in wrong date format string.

> Which older supported branches are affected by this flaw?
mozilla-aurora (mozilla48)

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

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not yet, but whole patches related to bug 887016 will be backed out from aurora in bug 1265307.

> How likely is this patch to cause regressions; how much testing does it need?
Not likely.  This removes unnecessary code path that was added by bug 887016.

Comment 5

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

Comment on attachment 8746300 [details] [diff] [review]
Call StringSplitString directly for internal use.

Clearing sec-approval. 

Just check this into trunk and then nominate for Aurora so we don't ship this problem anywhere.

Comment 6

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95a15364952f0f6ea4ba1bcdfb4b07b41de10f0b
Bug 1268138 - Call StringSplitString directly for internal use. r=till

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/95a15364952f

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8746300 [details] [diff] [review]
Call StringSplitString directly for internal use.

same patch is applicable to mozilla-aurora

Approval Request Comment
> [Feature/regressing bug #]
bug 887016

> [User impact if declined]
Wrong behavior in JavaScript Intl API

> [Describe test coverage new/current, TreeHerder]
Tested in mozilla-central automated test.

> [Risks and why]
Low.  Removed unneccesary code path added by bug 887016.

> [String/UUID change made/needed]
None

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8746300 [details] [diff] [review]
Call StringSplitString directly for internal use.

OK to uplift to aurora without sec approval; this is part of a backout for bug 1258920.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/22166e1f14d2