1368732 - Assertion failure: args[1].isString(), at js/src/builtin/RegExp.cpp:1116 with Intl

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision ebad93e11770 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

Object.prototype[Symbol.replace] = Array;
Intl.DateTimeFormat("arSA-u-ca-islamic-civilnu-latn").format;



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000f78cf0 in js::regexp_exec_no_statics (cx=0x7ffff6924000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/RegExp.cpp:1116
#0  0x0000000000f78cf0 in js::regexp_exec_no_statics (cx=0x7ffff6924000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/RegExp.cpp:1116
#1  0x000000000053e1df in js::CallJSNative (cx=cx@entry=0x7ffff6924000, native=0xf78b80 <js::regexp_exec_no_statics(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:293
#2  0x0000000000532f83 in js::InternalCallOrConstruct (cx=0x7ffff6924000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:470
#3  0x0000000000525940 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:521
#4  Interpret (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:3028
#5  0x0000000000532b32 in js::RunScript (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:410
#6  0x00000000005330b7 in js::InternalCallOrConstruct (cx=0x7ffff6924000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:488
#7  0x0000000000525940 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:521
[...]
#27 0x0000000000525940 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:521
#28 Interpret (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:3028
#29 0x0000000000532b32 in js::RunScript (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:410
#30 0x00000000005330b7 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6924000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:488
#31 0x0000000000533398 in InternalCall (cx=cx@entry=0x7ffff6924000, args=...) at js/src/vm/Interpreter.cpp:515
#32 0x00000000005334cd in js::Call (cx=cx@entry=0x7ffff6924000, fval=..., fval@entry=..., thisv=..., thisv@entry=..., args=..., rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:534
#33 0x000000000053366c in js::CallGetter (cx=cx@entry=0x7ffff6924000, thisv=thisv@entry=..., getter=getter@entry=..., rval=rval@entry=...) at js/src/vm/Interpreter.cpp:649
#34 0x0000000000b86fa9 in CallGetter (vp=..., shape=..., receiver=..., obj=..., cx=0x7ffff6924000) at js/src/vm/NativeObject.cpp:1912
#35 GetExistingProperty<(js::AllowGC)1> (cx=0x7ffff6924000, receiver=..., obj=..., shape=..., vp=...) at js/src/vm/NativeObject.cpp:1960
#36 0x0000000000b87cdf in NativeGetPropertyInline<(js::AllowGC)1> (cx=0x7ffff6924000, obj=..., receiver=..., id=..., nameLookup=nameLookup@entry=NotNameLookup, vp=...) at js/src/vm/NativeObject.cpp:2191
#37 0x0000000000b884c0 in js::NativeGetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.cpp:2225
#38 0x0000000000536834 in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.h:1527
#39 0x0000000000521f69 in js::GetProperty (vp=..., name=<optimized out>, receiver=..., obj=..., cx=0x7ffff6924000) at js/src/jsobj.h:846
#40 js::GetProperty (cx=0x7ffff6924000, v=..., name=..., vp=...) at js/src/vm/Interpreter.cpp:4402
#41 0x00000000005264f2 in GetPropertyOperation (vp=..., lval=..., pc=<optimized out>, script=..., fp=<optimized out>, cx=<optimized out>) at js/src/vm/Interpreter.cpp:193
#42 Interpret (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:2743
#43 0x0000000000532b32 in js::RunScript (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:410
[...]
#52 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8464
rax	0x0	0
rbx	0x7ffff425b5c8	140737289500104
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffff8ff0	140737488326640
rsp	0x7fffffff8f80	140737488326528
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff6924000	140737330167808
r13	0x7fffffff9010	140737488326672
r14	0x7ffff425b5d0	140737289500112
r15	0x7ffff425b5d8	140737289500120
rip	0xf78cf0 <js::regexp_exec_no_statics(JSContext*, unsigned int, JS::Value*)+368>
=> 0xf78cf0 <js::regexp_exec_no_statics(JSContext*, unsigned int, JS::Value*)+368>:	movl   $0x0,0x0
   0xf78cfb <js::regexp_exec_no_statics(JSContext*, unsigned int, JS::Value*)+379>:	ud2    


Marking this one s-s because it could be a type confusion in the native implementation of this part of Intl.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160407030451" and the hash "9f6afb62fe80ee168a90577957d1c53ad9fe8ecd".
The "bad" changeset has the timestamp "20160407034945" and the hash "4d0f975a23119a61a6c8e5856125de2db5713c49".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9f6afb62fe80ee168a90577957d1c53ad9fe8ecd&tochange=4d0f975a23119a61a6c8e5856125de2db5713c49

Comment 2

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

Arai-san, is bug 887016 or bug 1165052 a likely regressor?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]:

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> Arai-san, is bug 887016 or bug 1165052 a likely regressor?

yes

https://dxr.mozilla.org/mozilla-central/rev/7b8937970f9ca85db88cb2496f2112175fd847c8/js/src/builtin/Intl.js#113
>     while ((extensions = regexp_exec_no_statics(unicodeLocaleExtensionSequenceRE, left)) !== null) {
>         left = callFunction(String_replace, left, extensions[0], "");
>         unicodeLocaleExtensionSequenceRE.lastIndex = 0;
>     }

There, internal function shouldn't call String_replace.
it should use StringReplaceString or similar instead.

I'll try fixing it.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Use StringReplaceString instead of String_replace in Intl.

Changed String_replace in Intl.js to StringReplaceString, that is the fast path for the case everything is string, used internally in String_replace.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add testcase.

and the testcase.
maybe this shouldn't be landed at the same time as the Part 1.

Comment 7

[Andrew McCreight [:mccr8]]

Sounds like a type confusion, so I'll mark it sec-high.

Comment 8

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

We could still take a patch for 54 to land in the RC build, if you think it is worth the last minute risk. Otherwise we may want to wontfix for 54.

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 8873102 [details] [diff] [review]
Part 1: Use StringReplaceString instead of String_replace in Intl.

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

Sorry for the delay here :(

r=me

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 8873104 [details] [diff] [review]
Part 2: Add testcase.

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

Yeah, let's land this a bit later, definitely not for uplifts.

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8873102 [details] [diff] [review]
Part 1: Use StringReplaceString instead of String_replace in Intl.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not so easily, since the testcase is in different patch, and the change itself doesn't directly imply the flaw.

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

> Which older supported branches are affected by this flaw?
Firefox 53

> If not all supported branches, which bug introduced the flaw?
Bug 887016 (Firefox 48)

> Do you have backports for the affected branches? If not, how different,
> hard to create, and risky will they be?
not yet, but should be easily backported.

> How likely is this patch to cause regressions; how much testing does it need?
existing automated tests should catch most regression.
the testcase in Part 2 shouldn't be landed at the same time tho, it passes locally.

Comment 12

[Christian Holler (:decoder)]

I'm not sure about both the request to not uplift, as well as the assessment how obvious the vulnerability is.

First of all, it would be helpful to know the actual impact here. What can go wrong if you call String_replace with a wrong type? If this leads to critical things happening, then this should be straightforward to exploit. Till, can you maybe comment on this, or should I investigate this further?

Furthermore, for someone who knows a bit about JS vulnerabilities, the patch is in my opinion obvious. To me, this looks like a type confusion in a JS binding and this type of vulnerability has been highlighted multiple times recently.

So overall that means landing this without fixing it in earlier vulnerable releases might be very risky. We should at double check before proceeding here.

Comment 13

[Till Schneidereit [:till]]

After talking to decoder a bit, I agree that this seems worse than I originally assumed. It should be fairly easy to figure out that Object.prototype[Symbol.replace] is the weak link upon following String_replace, after which it's trivial to pass in arbitrary values to ExecuteRegExp.

OTOH, if we could someone mask this as a perf improvement, which it technically also is, it might be much harder to find. Unfortunately it'd probably be hard to explain why we'd uplift this perf improvement to release or ESR.

Perhaps we could land it as a perf improvement in release and not uplift it? We have lots of small perf patches landing right now, including in Intl code, so that'd not be noteworthy.

Comment 14

[Christian Holler (:decoder)]

Putting a needinfo on Dan and Al, maybe they can help figuring out the right time and way to land this :)

Comment 15

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

It is too late for 54. We're literally shipping in just over a week and making release candidates in the next day or too. We have no betas left. Unless this is the most trivial fix in the world, we can't take it. I do see that it is only one line, so you could make an argument for taking it on 55 (trunk) and backporting it to 54 (beta) for the release candidate.

I'd suggest that we land this in the first week of July, mid-way through the next release cycle, so we can get beta coverage and uplift.

I'm ni? Ritu and Liz.

Comment 16

[Till Schneidereit [:till]]

(In reply to Al Billings [:abillings] from comment #15)
> It is too late for 54. We're literally shipping in just over a week and
> making release candidates in the next day or too. We have no betas left.
> Unless this is the most trivial fix in the world, we can't take it. I do see
> that it is only one line, so you could make an argument for taking it on 55
> (trunk) and backporting it to 54 (beta) for the release candidate.

The patch really is about as trivial as it gets. Based on just that, I would argue for taking it for 54. However, see my comment above about discoverability of the issue. If we do backport this, we can be sure it'll be found, whereas if we land it as a perf improvement on trunk, it'll look entirely innocuous.

I guess it boils down to how likely we deem it that this has been or will be found independently.

Comment 17

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #11)
> Comment on attachment 8873102 [details] [diff] [review]
> Part 1: Use StringReplaceString instead of String_replace in Intl.
> 
> [Security approval request comment]
> > Which older supported branches are affected by this flaw?
> Firefox 53

To clarify, ESR 52 is affected - the introducing patch landed for 48.

Comment 18

[Christian Holler (:decoder)]

Also upping severity to critical based on comment 13. It is very likely that you can exploit this type confusion by passing a suitable object, plus it is easy and reliable to reach.

Comment 19

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

I would lean towards taking this today so we can get it into today's 54 rc build.

Comment 20

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

Comment on attachment 8873102 [details] [diff] [review]
Part 1: Use StringReplaceString instead of String_replace in Intl.

sec-approval+ for trunk.
We need beta and ESR52 branch patches made ASAP and nominated so they can get in today.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Yes, exactly what Liz said. I'll take it in ESR52 today as well.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8873102 [details] [diff] [review]
Part 1: Use StringReplaceString instead of String_replace in Intl.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d5328d66663b380382913a36caf877c65258540

Confirmed that it grafts cleanly to Beta and ESR52.

Approval Request Comment
[Feature/Bug causing the regression]: bug 887016
[User impact if declined]: sec-crit
[Is this code covered by automated tests?]: yes, though the testcase from this bug can't be landed until this is on all supported releases
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: none needed, we'll land the attached test when the time comes
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: existing test coverage will find any issues
[String changes made/needed]: none

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8873102 [details] [diff] [review]
Part 1: Use StringReplaceString instead of String_replace in Intl.

Sec-crit, Beta54+, ESR52+

Comment 24

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

I see Beta just merged into Release branch. Do we need to take this directly on Release as well as Beta?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Yes

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/97767088522e
https://hg.mozilla.org/releases/mozilla-release/rev/97767088522e
https://hg.mozilla.org/releases/mozilla-esr52/rev/4ec931d4bf29

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/4d5328d66663

Comment 28

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 29

[Christian Holler (:decoder)]

It took us quite a while to find this bug, so Al asked me to check why that is the case. For LangFuzz, the Symbol object and its properties were not listed as known (builtin) identifiers and therefore it was a little harder to produce the test in comment 0, plus there was maybe no suitable test in the codebase.

Symbol is an ES6 feature, does jsfunfuzz use it for fuzzing? A full list of all redefinable symbols is here: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol#Well-known_symbols

It especially makes sense to combine this with calls to self-hosted JS APIs like the Intl code. Not sure if jsfunfuzz can actually call those.

Comment 30

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx54

Comment 31

[Jesse Schwartzentruber (:truber)]

I'm not familiar with jsfunfuzz coverage.

Comment 32

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

Punting to Jason as per the All-Hands f2f discussion.

Comment 33

[Jason Kratzer [:jkratzer]]

It doesn't appear that jsfunfuzz has any direct coverage for symbol objects.  I'll look into this later this week to see what would be required in order to implement it.