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

VERIFIED FIXED in Firefox -esr52

Status

()

defect
--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Assigned: arai)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla55
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr5254+ fixed, firefox53 wontfix, firefox54+ verified, firefox55+ verified)

Details

(Whiteboard: [jsbugmon:update][adv-main54+][adv-esr52.2+])

Attachments

(2 attachments)

Reporter

Description

2 years ago
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.

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

2 years ago
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
Arai-san, is bug 887016 or bug 1165052 a likely regressor?
Flags: needinfo?(arai.unmht)
[Tracking Requested - why for this release]:
Assignee

Comment 4

2 years ago
(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.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Flags: needinfo?(arai.unmht)
Assignee

Updated

2 years ago
Blocks: 887016
Assignee

Comment 5

2 years ago
Changed String_replace in Intl.js to StringReplaceString, that is the fast path for the case everything is string, used internally in String_replace.
Attachment #8873102 - Flags: review?(till)
Assignee

Comment 6

2 years ago
and the testcase.
maybe this shouldn't be landed at the same time as the Part 1.
Attachment #8873104 - Flags: review?(till)
Sounds like a type confusion, so I'll mark it sec-high.
Keywords: sec-high
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 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
Attachment #8873102 - Flags: review?(till) → review+
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.
Attachment #8873104 - Flags: review?(till) → review+
Assignee

Comment 11

2 years ago
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.
Attachment #8873102 - Flags: sec-approval?
Reporter

Comment 12

2 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(till)
Reporter

Comment 14

2 years ago
Putting a needinfo on Dan and Al, maybe they can help figuring out the right time and way to land this :)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
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.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(abillings)
(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.
(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.
Reporter

Comment 18

2 years ago
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.
Keywords: sec-highsec-critical
I would lean towards taking this today so we can get it into today's 54 rc build.
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.
Flags: needinfo?(arai.unmht)
Attachment #8873102 - Flags: sec-approval? → sec-approval+
Yes, exactly what Liz said. I'll take it in ESR52 today as well.
Flags: needinfo?(rkothari)
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
Flags: needinfo?(lhenry)
Flags: needinfo?(dveditz)
Flags: needinfo?(arai.unmht)
Attachment #8873102 - Flags: approval-mozilla-esr52?
Attachment #8873102 - Flags: approval-mozilla-beta?
Comment on attachment 8873102 [details] [diff] [review]
Part 1: Use StringReplaceString instead of String_replace in Intl.

Sec-crit, Beta54+, ESR52+
Attachment #8873102 - Flags: approval-mozilla-esr52?
Attachment #8873102 - Flags: approval-mozilla-esr52+
Attachment #8873102 - Flags: approval-mozilla-beta?
Attachment #8873102 - Flags: approval-mozilla-beta+
I see Beta just merged into Release branch. Do we need to take this directly on Release as well as Beta?
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main54+][adv-esr52.2+]
https://hg.mozilla.org/mozilla-central/rev/4d5328d66663
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 years ago
Status: RESOLVED → VERIFIED

Comment 28

2 years ago
JSBugMon: This bug has been automatically verified fixed.
Reporter

Comment 29

2 years ago
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.
Flags: needinfo?(jschwartzentruber)
Flags: needinfo?(gary)
Group: javascript-core-security → core-security-release

Comment 30

2 years ago
JSBugMon: This bug has been automatically verified fixed on Fx54
I'm not familiar with jsfunfuzz coverage.
Flags: needinfo?(jschwartzentruber)
Punting to Jason as per the All-Hands f2f discussion.
Flags: needinfo?(gary) → needinfo?(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.
Flags: needinfo?(jkratzer)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.