If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

AddressSanitizer: heap-buffer-overflow [@ char* js::QuoteString<char16_t>] with READ of size 2 with newExternalString

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla54
x86
Linux
crash, jsbugmon, regression, testcase
Points:
---

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54+ fixed)

Details

(Whiteboard: [jsbugmon:update,bisect])

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
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

8 months ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 1

8 months ago
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8831645 - Flags: review?(nicolas.b.pierron)
Attachment #8831645 - Flags: review?(nicolas.b.pierron) → review+
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

8 months 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
status-firefox52: ? → affected
status-firefox53: ? → affected
status-firefox-esr45: ? → unaffected
tracking-firefox-esr45: ? → ---

Comment 4

8 months ago
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

8 months ago
Flags: needinfo?(jdemooij)

Comment 5

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b572e7a74e7f
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 7

8 months 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.
status-firefox52: affected → wontfix
status-firefox53: affected → wontfix
tracking-firefox52: ? → ---
tracking-firefox53: ? → ---
Flags: needinfo?(jdemooij)
tracking-firefox54: ? → +
You need to log in before you can comment on or make changes to this bug.