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

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: anba, Assigned: arai)

Tracking

({regression})

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48 fixed, firefox49 fixed, firefox-esr45 unaffected)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
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);
---
Group: javascript-core-security
Assignee

Comment 2

3 years ago
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)
Assignee: nobody → arai.unmht
Attachment #8746300 - Flags: review?(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.
Attachment #8746300 - Flags: review?(till) → review+
Assignee

Comment 4

3 years ago
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.
Attachment #8746300 - Flags: sec-approval?
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.
Attachment #8746300 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/95a15364952f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee

Comment 8

3 years ago
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
Attachment #8746300 - Flags: approval-mozilla-aurora?
Group: javascript-core-security → core-security-release
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.
Attachment #8746300 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1272523
Group: core-security-release
Keywords: regression
You need to log in before you can comment on or make changes to this bug.