1518440 - AddressSanitizer: heap-buffer-overflow [@ mozilla::RangedPtr<unsigned char> InfallibleQuote<unsigned char, unsigned char>] with WRITE of size 1 or Assertion failure: mPtr <= mRangeEnd, at mozilla/RangedPtr.h:53

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 0dcf945b3871 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --without-intl-api --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --enable-fuzzing --target=i686-pc-linux-gnu, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

try {
prettyPrinted = function prettyPrinted(value) {
return JSON.stringify(value);
}
var y = class SingletonArrayBuffer extends constructor {}
^ ArrayBuffer++
x = x = "";
for(var i = 0; i < 10000; ++i)
x += x + y + x + y + x + "";
} catch (exc) {}
function foo(s) {
return s + x;
}
var outer = {};
var boom = foo(outer);
prettyPrinted(boom);

Backtrace:

==6626==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xea527600 at pc 0x56c36e41 bp 0xfffbfa48 sp 0xfffbfa3c
WRITE of size 1 at 0xea527600 thread T0
#0 0x56c36e40 in mozilla::RangedPtr<unsigned char> InfallibleQuote<unsigned char, unsigned char>(mozilla::RangedPtr<unsigned char const>, mozilla::RangedPtr<unsigned char const>, mozilla::RangedPtr<unsigned char>) js/src/builtin/JSON.cpp:90:19
#1 0x56c36e40 in bool Quote<unsigned char, mozilla::Vector<unsigned char, 64u, js::TempAllocPolicy> >(mozilla::Vector<unsigned char, 64u, js::TempAllocPolicy>&, JSLinearString*) js/src/builtin/JSON.cpp:157
#2 0x56c36e40 in Quote(JSContext*, js::StringBuffer&, JSString*) js/src/builtin/JSON.cpp:181
#3 0x56c1d331 in Str(JSContext*, JS::Value const&, (anonymous namespace)::StringifyContext*) js/src/builtin/JSON.cpp:642:12
#4 0x56c1714a in js::Stringify(JSContext*, JS::MutableHandle<JS::Value>, JSObject*, JS::Value const&, js::StringBuffer&, js::StringifyBehavior) js/src/builtin/JSON.cpp:866:10
#5 0x56c26a9e in json_stringify(JSContext*, unsigned int, JS::Value*) js/src/builtin/JSON.cpp:1065:8
#6 0x56ae7737 in CallJSNative(JSContext*, bool ()(JSContext, unsigned int, JS::Value*), JS::CallArgs const&) js/src/vm/Interpreter.cpp:443:13
#7 0x56ae7737 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:535
#8 0x56aeb2cf in InternalCall(JSContext*, js::AnyInvokeArgs const&) js/src/vm/Interpreter.cpp:590:10
#9 0x56ab3354 in js::CallFromStack(JSContext*, JS::CallArgs const&) js/src/vm/Interpreter.cpp:594:10
#10 0x56ab3354 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3320
#11 0x56a905b2 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:423:10
#12 0x56ae7ce2 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:563:13
#13 0x56aeb2cf in InternalCall(JSContext*, js::AnyInvokeArgs const&) js/src/vm/Interpreter.cpp:590:10
#14 0x56aeb0db in js::CallFromStack(JSContext*, JS::CallArgs const&) js/src/vm/Interpreter.cpp:594:10
#15 0x5831b8e3 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:3910:10
#16 0xea90422c (<unknown module>)

0xea527600 is located 0 bytes to the right of 128-byte region [0xea527580,0xea527600)
allocated by thread T0 here:
#0 0x568e1744 in __interceptor_malloc (/mnt/LangFuzz/work/builds/opt32asan/dist/bin/js+0x360744)
#1 0x569dcfeb in SystemMalloc::malloc(unsigned int) memory/build/malloc_decls.h:37:1
#2 0x569dcfeb in DummyArenaAllocator<SystemMalloc>::moz_arena_malloc(unsigned int, unsigned int) memory/build/malloc_decls.h:37
#3 0x569dcfeb in moz_arena_malloc memory/build/malloc_decls.h:115
#4 0x569c1fb7 in js_arena_malloc(unsigned int, unsigned int) dist/include/js/Utility.h:353:10
#5 0x569c1fb7 in unsigned char* js_pod_arena_malloc<unsigned char>(unsigned int, unsigned int) dist/include/js/Utility.h:535
#6 0x569c1fb7 in unsigned char* js_pod_malloc<unsigned char>(unsigned int) dist/include/js/Utility.h:540
#7 0x569c1fb7 in unsigned char* js::AllocPolicyBase::maybe_pod_malloc<unsigned char>(unsigned int) dist/include/js/AllocPolicy.h:31
#8 0x569c1fb7 in unsigned char* js::TempAllocPolicy::pod_malloc<unsigned char>(unsigned int) dist/include/js/AllocPolicy.h:102
#9 0x569c1fb7 in mozilla::Vector<unsigned char, 64u, js::TempAllocPolicy>::convertToHeapStorage(unsigned int) dist/include/mozilla/Vector.h:915
#10 0x569c1fb7 in mozilla::Vector<unsigned char, 64u, js::TempAllocPolicy>::growStorageBy(unsigned int) dist/include/mozilla/Vector.h:1003
#11 0x56c36348 in mozilla::Vector<unsigned char, 64u, js::TempAllocPolicy>::growByUninitialized(unsigned int) dist/include/mozilla/Vector.h:1113:9
#12 0x56c36348 in bool Quote<unsigned char, mozilla::Vector<unsigned char, 64u, js::TempAllocPolicy> >(mozilla::Vector<unsigned char, 64u, js::TempAllocPolicy>&, JSLinearString*) js/src/builtin/JSON.cpp:147
#13 0x56c36348 in Quote(JSContext*, js::StringBuffer&, JSString*) js/src/builtin/JSON.cpp:181
#14 0x56c1d331 in Str(JSContext*, JS::Value const&, (anonymous namespace)::StringifyContext*) js/src/builtin/JSON.cpp:642:12
#15 0x56c1714a in js::Stringify(JSContext*, JS::MutableHandle<JS::Value>, JSObject*, JS::Value const&, js::StringBuffer&, js::StringifyBehavior) js/src/builtin/JSON.cpp:866:10
#16 0x56c26a9e in json_stringify(JSContext*, unsigned int, JS::Value*) js/src/builtin/JSON.cpp:1065:8
#17 0x56ae7737 in CallJSNative(JSContext*, bool ()(JSContext, unsigned int, JS::Value*), JS::CallArgs const&) js/src/vm/Interpreter.cpp:443:13
#18 0x56ae7737 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:535
#19 0x56aeb2cf in InternalCall(JSContext*, js::AnyInvokeArgs const&) js/src/vm/Interpreter.cpp:590:10
#20 0x56ab3354 in js::CallFromStack(JSContext*, JS::CallArgs const&) js/src/vm/Interpreter.cpp:594:10
#21 0x56ab3354 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3320
#22 0x56a905b2 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:423:10
#23 0x56ae7ce2 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:563:13
#24 0x56aeb2cf in InternalCall(JSContext*, js::AnyInvokeArgs const&) js/src/vm/Interpreter.cpp:590:10
#25 0x56aeb0db in js::CallFromStack(JSContext*, JS::CallArgs const&) js/src/vm/Interpreter.cpp:594:10
#26 0x5831b8e3 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:3910:10
#27 0xea90422c (<unknown module>)
#28 0xf4f1ff5f (<unknown module>)
#29 0xea9006aa (<unknown module>)
#30 0x586f6745 in EnterBaseline(JSContext*, js::jit::EnterJitData&) js/src/jit/BaselineJIT.cpp:124:5
#31 0x586f6745 in js::jit::EnterBaselineAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) js/src/jit/BaselineJIT.cpp:202
#32 0x56ad0415 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2250:24
#33 0x56a905b2 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:423:10
#34 0x56af03b1 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:782:13
#35 0x56af0e30 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:815:10
#36 0x56e3b342 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) js/src/vm/CompilationAndEvaluation.cpp:438:10
#37 0x56e3b829 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) js/src/vm/CompilationAndEvaluation.cpp:471:10
#38 0x569bfead in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool) js/src/shell/js.cpp:893:10
#39 0x569bdd29 in Process(JSContext*, char const*, bool, FileKind) js/src/shell/js.cpp
#40 0x5694bdc5 in ProcessArgs(JSContext*, js::cli::OptionParser*) js/src/shell/js.cpp:10083:10
#41 0x5694bdc5 in Shell(JSContext*, js::cli::OptionParser*, char**) js/src/shell/js.cpp:10536
#42 0x5694bdc5 in main js/src/shell/js.cpp:11128
#43 0xf73a7636 in __libc_start_main (/lib32/libc.so.6+0x18636)

SUMMARY: AddressSanitizer: heap-buffer-overflow js/src/builtin/JSON.cpp:90:19 in mozilla::RangedPtr<unsigned char> InfallibleQuote<unsigned char, unsigned char>(mozilla::RangedPtr<unsigned char const>, mozilla::RangedPtr<unsigned char const>, mozilla::RangedPtr<unsigned char>)
Shadow bytes around the buggy address:
0x3d4a4e70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x3d4a4e80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x3d4a4e90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x3d4a4ea0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x3d4a4eb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x3d4a4ec0:[fa]fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x3d4a4ed0: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
0x3d4a4ee0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x3d4a4ef0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x3d4a4f00: fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa fa
0x3d4a4f10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Heap left redzone: fa
Freed heap region: fd
==6626==ABORTING

This bug reproduces on 32-bit only (might be an integer overflow).

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/cafc30a40d9f
user:        Jan de Mooij
date:        Mon Nov 26 23:28:47 2018 +0000
summary:     Bug 1509542 part 3 - Increase JSString max length from |2**28 - 1| to |2**30 - 2|. r=jwalden

This iteration took 556.580 seconds to run.

Comment 2

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

Jan, is bug 1509542 a likely regressor?

Comment 3

[Jan de Mooij [:jandem]]

Minimal testcase:

  var s = "\0".repeat(715827897);
  JSON.stringify(["a", s]);

Bug 1343005 optimized the JSON Quote function but didn't add any overflow checks or assertions, so when we bumped the max string length this became an issue on 32-bit :/

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1518440 - Use CheckedInt in JSON Quote function. r?jwalden!

Comment 5

[Jan de Mooij [:jandem]]

NI for the question on the Phabricator review :)

Comment 6

[Jan de Mooij [:jandem]]

Discussed on IRC :)

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 9036535 [details]
Bug 1518440 - Use CheckedInt in JSON Quote function. r?jwalden!

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Patch makes it pretty clear what the problem is sadly.

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?: beta and central

If not all supported branches, which bug introduced the flaw?: 1509542 exposed it but code was questionable before

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: Easy, low risk.

How likely is this patch to cause regressions; how much testing does it need?: Unlikely, fix is simple.

Comment 8

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

Comment on attachment 9036535 [details]
Bug 1518440 - Use CheckedInt in JSON Quote function. r?jwalden!

sec-approval+ for trunk. Please nominate a beta patch as well.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 9036535 [details]
Bug 1518440 - Use CheckedInt in JSON Quote function. r?jwalden!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: 1509542 exposed it but code was questionable before

User impact if declined: Security issues on 32-bit

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Simple fix, verified locally.

String changes made/needed: None

Comment 10

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/a27f8c7557d4

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/a27f8c7557d4

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9036535 [details]
Bug 1518440 - Use CheckedInt in JSON Quote function. r?jwalden!

[Triage Comment]
Fixes a new sec-high caused by the max string size increase shipping in 65. Approved for 65.0b12.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d850ab97f26b

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.