1524755 - AddressSanitizer: Crash [@ bool InflateUTF8ToUTF16] or Assertion failure: mRangeStart <= mPtr, at dist/include/mozilla/RangedPtr.h:52

Status: VERIFIED FIXED

(Core :: MFBT, defect)

Description

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

The following testcase crashes on mozilla-central revision 024bef408a88 (build with AddressSanitizer on and --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion w134-out.wrapper w134-out.wasm):

The testcase unfortunately is several GB in size so just attaching the stack trace in the hopes that it would suffice.

Backtrace:

AddressSanitizer:DEADLYSIGNAL

==21059==ERROR: AddressSanitizer: SEGV on unknown address 0x7f591259c800 (pc 0x56174bdb0084 bp 0x7ffca75b68d0 sp 0x7ffca75b67c0 T0)
==21059==The signal is caused by a READ memory access.
#0 0x56174bdb0083 in bool InflateUTF8ToUTF16<(OnUTF8Error)2, InflateUTF8StringHelper<(OnUTF8Error)2, JS::TwoByteCharsZ, JS::UTF8Chars>(JSContext*, JS::UTF8Chars, unsigned long*)::{lambda(char16_t)#1}, JS::UTF8Chars>(JSContext*, JS::UTF8Chars, InflateUTF8StringHelper<(OnUTF8Error)2, JS::TwoByteCharsZ, JS::UTF8Chars>(JSContext*, JS::UTF8Chars, unsigned long*)::{lambda(char16_t)#1}) js/src/vm/CharacterEncoding.cpp:292:27
#1 0x56174bdb0083 in JS::TwoByteCharsZ InflateUTF8StringHelper<(OnUTF8Error)2, JS::TwoByteCharsZ, JS::UTF8Chars>(JSContext*, JS::UTF8Chars, unsigned long*) js/src/vm/CharacterEncoding.cpp:436
#2 0x56174bdf1401 in CompileUtf8(JSContext*, JS::ReadOnlyCompileOptions const&, char const*, unsigned long, JS::MutableHandle<JSScript*>) js/src/vm/CompilationAndEvaluation.cpp:78:7
#3 0x56174bdf2137 in JS::CompileUtf8File(JSContext*, JS::ReadOnlyCompileOptions const&, _IO_FILE*, JS::MutableHandle<JSScript*>) js/src/vm/CompilationAndEvaluation.cpp:135:10
#4 0x56174ba6389d in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool) js/src/shell/js.cpp:881:12
/snip

For detailed crash information, see attachment.

Setting s-s because this involves ASan.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

Not sure if this is wasm-specific or if it's related to bug 1523440? :arai/:lth, any thoughts?

Comment 3

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

Attachment: Debug shell stack trace

Tested with 64-bit debug shell compiled with --enable-more-deterministic on m-c rev 024bef408a88

Comment 4

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

I've tested and I think the patch in bug 1523440 comment 4 does not fix at least the assert.

Comment 5

[Lars T Hansen [:lth]]

What's wasm-specific here? Nothing, from what I can tell.

Comment 6

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

Attachment: seed.tar.bz2

Let's try these steps to reproduce:

1) Extract the seed using: tar xvjf seed.tar.bz2
2) Download binaryen v64: wget https://github.com/WebAssembly/binaryen/releases/download/version_64/binaryen-version_64-x86_64-linux.tar.gz
3) Generate the wasm and wrapper files: binaryen-version_64/wasm-opt w134-out.txt-binaryen-v64-seed --translate-to-fuzz --disable-simd --output out.wasm --emit-js-wrapper=wrapper.js
4) Compile: AR=ar sh ./configure --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests --disable-cranelift
5) Run using the js shell: ./js --fuzzing-safe --no-threads --no-baseline --no-ion wrapper.js out.wasm

Due to the large size, step 3 took 1 - 1.5 hours to run while step 5 takes ~20s. SSDs and a fast machine is recommended.

I could then get it to assert.

Comment 7

[Tooru Fujisawa [:arai]]

unfortunately, step 3 doesn't finish on my linux VM (has 8GB RAM, but OOM and killed)
if you know the content of memory between src.mStart.mPtr and src.mEnd.mPtr, that will help.
(I could setup another linux box tho, I don't have vacant SSD right now)

Comment 8

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

Attachment: test.tar.xz

I dug more and realised only the wrapper.js was needed, so I compressed it using tar cvJf and attached it here.

Will try and reduce more but not sure Lithium is being as effective as normal cases.

Comment 9

[Tooru Fujisawa [:arai]]

Thanks! reproduced locally.

and turns out that it's integer overflow (not specific to WASM).

https://searchfox.org/mozilla-central/rev/69d3c6c61dca9a41f14797cd9924733289238d1a/mfbt/RangedPtr.h#214-217

template <typename T>
class RangedPtr {
...
  T& operator[](int aIndex) const {
    MOZ_ASSERT(size_t(aIndex > 0 ? aIndex : -aIndex) <= size_t(-1) / sizeof(T));
    return *create(mPtr + aIndex);
  }

https://searchfox.org/mozilla-central/rev/69d3c6c61dca9a41f14797cd9924733289238d1a/js/src/vm/CharacterEncoding.cpp#291-292

template <OnUTF8Error ErrorAction, typename OutputFn, class InputCharsT>
static bool InflateUTF8ToUTF16(JSContext* cx, const InputCharsT src,
                               OutputFn dst) {
  size_t srclen = src.length();
  for (uint32_t i = 0; i < srclen; i++) {
    uint32_t v = uint32_t(src[i]);

where i = aIndex = 0x80000000.

Waldo, can I have your opinion how to fix this?
can we have unsigned int variant of RangedPtr::operator[] ?

Comment 10

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

Thanks Arai-san!

I checked that it goes back to prior to m-c rev bcacb5692ad9 and hence prior to Jan 2015.

Comment 11

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 12

[Jeff Walden [:Waldo]]

I don't think you need a separate variant. Just make this function take ptrdiff_t.

(Now technically this would still be a problem if the array being ranged over occupied more than half of memory and elements were byte-sized, but. ptrdiff_t is what the standard uses for this, and it's really the best we can do.)

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Use ptrdiff_t instead in RangedPtr.

Thanks!
replaced int with ptrdiff_t.
confirmed it doesn't crash with the testcase above, but just hit SyntaxError.

../tmp/wrapper.js:50331649:0 SyntaxError: missing } after catch block
../tmp/wrapper.js:50331648:12 note: { opened at line 50331648, column 12

This patch doesn't contain testcase, given it needs to be 2+ GB and takes 10+ minutes on debug build :P

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 9041423 [details] [diff] [review]
Use ptrdiff_t instead in RangedPtr.

Security Approval Request

How easily could an exploit be constructed based on the patch?

Technically easy, but not so much in practice.
this requires 2GB+ source code to be downloaded and parsed before hitting it, that should take much time.

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

Yes

Which older supported branches are affected by this flaw?

all

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

Bug 662001

Do you have backports for the affected branches?

Yes

If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

less likely. It doesn't change the behavior for correct case, and fixes the wrong case (that overflow happened).
This patch doesn't contain testcase given it requires 2GB+ file, but manually testing it with the files in this bug should be sufficient.

Comment 15

[Tooru Fujisawa [:arai]]

to be clear, the same code had existed before bug 662001, but moved to mfbt at that point.

Comment 16

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

sec-approval+ for trunk. We'll want beta and ESR60 patches made and nominated as well.

Comment 17

[Tooru Fujisawa [:arai]]

Comment on attachment 9041423 [details] [diff] [review]
Use ptrdiff_t instead in RangedPtr.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 662001

User impact if declined

Possible out-of-bound access in JS parsing, and memory data leak from JavaScript error message.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

No

Needs manual test from QE?

Yes

If yes, steps to reproduce

1. Download and extract debug build of JS shell
2. Download and extract file in comment #8
3. Move files from step 1 and 2 in the same directory
4. Run ./js --fuzzing-safe --no-threads --no-baseline --no-ion wrapper.js
5. Wait for several minutes
6. Confirm it throws SyntaxError, instead of crash

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

It doesn't change the behavior for correct case, and fixes the wrong case (that overflow happened).

String changes made/needed

None

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

it's sec-high

User impact if declined

Possible out-of-bound access in JS parsing, and memory data leak from JavaScript error message.

Fix Landed on Version

not yet, will be 67

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

It doesn't change the behavior for correct case, and fixes the wrong case (that overflow happened).

String or UUID changes made by this patch

None

Comment 18

[Tooru Fujisawa [:arai]]

The same patch applies to beta and esr60 branches

Comment 19

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5732522a3540af8ad2dbfcb156346b6e44ef2233
Bug 1524755 - Use ptrdiff_t instead in RangedPtr. r=jwalden

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5732522a3540

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9041423 [details] [diff] [review]
Use ptrdiff_t instead in RangedPtr.

[Triage Comment]
Fixes a sec-high, approved for 66.0b7.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2b77d6958f31

Comment 23

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Hello,

Managed to reproduce this issue with this build from Treeherder.

Verified fixed with this build from Treeherder and on the latest FF 66.0b7 build on Ubuntu 16.04 x64.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9041423 [details] [diff] [review]
Use ptrdiff_t instead in RangedPtr.

Simple sec fix already verified by QA on Nightly & Beta. Approved for 60.6esr.

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/eb3fb35e30ca

Comment 26

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Confirming this issue as verified fixed with 60.6.0esr(20190313091946) on Ubuntu 16.04x64.