Closed Bug 1598778 Opened 4 years ago Closed 4 years ago

AddressSanitizer: heap-buffer-overflow [@ void ConsumeSpaces<unsigned char>] with READ of size 1

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 --- unaffected
firefox71 --- unaffected
firefox72 --- fixed

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 1 open bug, Regression)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 1469feeca395 (build with --enable-address-sanitizer, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

toString = function() {
    return [];
}
x = new ArrayBuffer;
y = function(j) {};
Object.defineProperty(x, "", {
    enumerable: true,
    get: y
})
y.__proto__ = [(function(){ undefined; })]
Number(this);

Backtrace:

==19886==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60300000e3cc at pc 0x5583e598b7e9 bp 0x7ffe50075910 sp 0x7ffe50075908
READ of size 1 at 0x60300000e3cc thread T0
    #0 0x5583e598b7e8 in void ConsumeSpaces<unsigned char>(unsigned char const*&, unsigned char const*) js/src/builtin/Object.cpp:156:10
    #1 0x5583e598b7e8 in bool ArgsAndBodySubstring<unsigned char>(mozilla::Range<unsigned char const>, unsigned long*, unsigned long*) js/src/builtin/Object.cpp:211
    #2 0x5583e598b7e8 in js::ObjectToSource(JSContext*, JS::Handle<JSObject*>)::$_3::operator()(JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, PropertyKind) const js/src/builtin/Object.cpp:352
    #3 0x5583e5987b23 in js::ObjectToSource(JSContext*, JS::Handle<JSObject*>) js/src/builtin/Object.cpp:429:14
    #4 0x5583e59b3f39 in obj_toSource(JSContext*, unsigned int, JS::Value*) js/src/builtin/Object.cpp:132:19
    #5 0x5583e5829efc in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) js/src/vm/Interpreter.cpp:456:13
/snip

For detailed crash information, see attachment.

Setting s-s as a start as ASan has spotted a heap buffer overflow.

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/02f8ab4ff124
user: Jan de Mooij
date: Wed Oct 23 11:23:50 2019 +0000
summary: Bug 1330776 - Stop null-terminating JS strings, remove JSFlatString. r=jwalden,sfink

Jan, is bug 1330776 a likely regressor?

Flags: needinfo?(jdemooij)
Regressed by: 1330776
template <typename CharT>
static void ConsumeSpaces(const CharT*& s, const CharT* e) {
  while (*s == ' ' && s < e) {
    s++;
  }
}

Almost certainly those conditions should be flipped, but I can't reproduce the original problem so am not absolutely confident that's the problem and obvious fix.

Yeah I was wondering too. I'll try to reproduce this tomorrow.

I'm pretty sure I audited all uses of the char-access APIs at least twice, resulting in many of the patches in bug 1586991. Maybe I missed latin1Range/twoByteRange, will look at the other callers too.

There was a similar issue in the caller, where it checks |*s != '('|.

Also changes the code to use RangedPtr so these issues become assertion failures
instead of requiring ASan.

ConsumeUntil was added to encapsulate the js_strchr_limit call.

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Priority: -- → P1
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: