The following testcase crashes on mozilla-central revision 8dbe89935366 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --without-intl-api --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --target=i686-pc-linux-gnu, run with --fuzzing-safe --ion-offthread-compile=off min.js): assertEq(newExternalString("abc"), 0 + (this) << (this) <= 1 < (this) < (this)); Backtrace: ==17324==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xf5e160b6 at pc 0x0994c228 bp 0xffc1a068 sp 0xffc1a05c READ of size 2 at 0xf5e160b6 thread T0 #0 0x994c227 in char* js::QuoteString<char16_t>(js::Sprinter*, char16_t const*, unsigned int, char16_t) js/src/vm/Printer.cpp:311:17 #1 0x994c227 in js::QuoteString(js::Sprinter*, JSString*, char16_t) js/src/vm/Printer.cpp:374 #2 0x994c4d8 in js::QuoteString(js::ExclusiveContext*, JSString*, char16_t) js/src/vm/Printer.cpp:383:19 #3 0x934c238 in js::StringToSource(JSContext*, JSString*) js/src/jsstr.cpp:3181:12 #4 0x934c238 in js::ValueToSource(JSContext*, JS::Handle<JS::Value>) js/src/jsstr.cpp:3149 #5 0x90a32d4 in JS_ValueToSource(JSContext*, JS::Handle<JS::Value>) js/src/jsapi.cpp:381:12 #6 0x81eee22 in ToSource(JSContext*, JS::MutableHandle<JS::Value>, JSAutoByteString*) js/src/shell/js.cpp:2223:21 #7 0x81eee22 in AssertEq(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:2253 #8 0x8434c9b in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:239:15 [...] #24 0x80ae104 in _start (/home/ubuntu/mozilla-central/js/src/dist/bin/js+0x80ae104) 0xf5e160b6 is located 0 bytes to the right of 6-byte region [0xf5e160b0,0xf5e160b6) allocated by thread T0 here: #0 0x8173ff4 in __interceptor_malloc asan/asan_malloc_linux.cc:52 #1 0x8e99f7f in js_malloc(unsigned int) dist/include/js/Utility.h:229:12 #2 0x8e99f7f in char16_t* js_pod_malloc<char16_t>(unsigned int) dist/include/js/Utility.h:420 #3 0x8e99f7f in NewExternalString(JSContext*, unsigned int, JS::Value*) js/src/builtin/TestingFunctions.cpp:1270 #4 0x8434c9b in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:239:15 [...] #18 0x81cce75 in main js/src/shell/js.cpp:7960 #19 0xf734a636 in __libc_start_main (/lib32/libc.so.6+0x18636) SUMMARY: AddressSanitizer: heap-buffer-overflow js/src/vm/Printer.cpp:311:17 in char* js::QuoteString<char16_t>(js::Sprinter*, char16_t const*, unsigned int, char16_t) Shadow bytes around the buggy address: 0x3ebc2c00: fa fa fa fa fa fa fa fa fa fa 00 00 fa fa fd fa =>0x3ebc2c10: fa fa 00 04 fa fafa fa fa 00 00 fa fa fd fa 0x3ebc2c20: fa fa 00 04 fa fa 07 fa fa fa fd fd fa fa fd fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd This is probably shell only (I suspect some interaction between assertEq native and the new testing function), but marking s-s until it has been checked.
Created attachment 8831645 [details] [diff] [review] Patch (In reply to Christian Holler (:decoder) from comment #0) > This is probably shell only (I suspect some interaction between assertEq > native and the new testing function), but marking s-s until it has been > checked. It's actually a pre-existing bug in QuoteString, exposed by newExternalString \o/. QuoteString does this: c = *++t; if (t == end) break; That's fine when the string is null-terminated, but if it isn't, we read 1 character past the end of the string. We don't *use* this character anywhere so this isn't exploitable, but it could definitely cause crashes. I rewrote this function to use mozilla::Range, so we get assertion failures in debug builds.
What's the sec rating for this bug and how far back does it go?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #2) > What's the sec rating for this bug and how far back does it go? Not a security bug. We can read 2 bytes out of bounds, but we don't use these bytes anywhere, so the worst that could happen is a crash. Thinking about it more, this likely can't even happen in the browser because AFAIK external strings it creates are still part of a larger DOM string that's null-terminated. Let's backport this because the fix is pretty simple and safe, but it's not super urgent.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/b572e7a74e7f Fix QuoteString to not read a character out of bounds. r=nbp
Please request Aurora/Beta approval on this when you get a chance.
Actually, I think it's fine and safer to let this ride the trains. As described in comment 3 this can't happen in the browser (Boris confirmed this last week on IRC) and even if it could it's not s-s.