Closed
Bug 1334590
Opened 7 years ago
Closed 7 years ago
AddressSanitizer: heap-buffer-overflow [@ char* js::QuoteString<char16_t>] with READ of size 2 with newExternalString
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: decoder, Assigned: jandem)
References
(Blocks 1 open bug)
Details
(4 keywords, Whiteboard: [jsbugmon:update,bisect])
Attachments
(1 file)
3.41 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
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 fa[06]fa 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.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 1•7 years ago
|
||
(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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8831645 -
Flags: review?(nicolas.b.pierron)
Updated•7 years ago
|
Attachment #8831645 -
Flags: review?(nicolas.b.pierron) → review+
Comment 2•7 years ago
|
||
What's the sec rating for this bug and how far back does it go?
status-firefox51:
--- → wontfix
status-firefox52:
--- → ?
status-firefox53:
--- → ?
status-firefox-esr45:
--- → ?
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
tracking-firefox-esr45:
--- → ?
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 3•7 years ago
|
||
(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.
Group: javascript-core-security
tracking-firefox-esr45:
? → ---
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b572e7a74e7f Fix QuoteString to not read a character out of bounds. r=nbp
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jdemooij)
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b572e7a74e7f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 6•7 years ago
|
||
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 7•7 years ago
|
||
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.
Updated•7 years ago
|
Updated•4 years ago
|
Blocks: asan-maintenance
You need to log in
before you can comment on or make changes to this bug.
Description
•