1464477 - Crash [@ JSObject::getClass] with wasm

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The attached binary WebAssembly testcase crashes on mozilla-inbound revision cade55a8f3ac (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu). To reproduce, you can run the following code in the JS shell:

var data = os.file.readFile(file, 'binary');
new WebAssembly.Instance(new WebAssembly.Module(data.buffer));



Backtrace:

==7894==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x082b225b bp 0xffde2528 sp 0xffde2510 T0)
==7894==The signal is caused by a READ memory access.
==7894==Hint: address points to the zero page.
    #0 0x82b225a in JSObject::getClass() const js/src/vm/JSObject.h:114:16
    #1 0x82b225a in js::NativeObject::getReservedSlot(unsigned int) const js/src/vm/NativeObject.h:1081
    #2 0xb02028e in js::WasmMemoryObject::buffer() const js/src/wasm/WasmJS.cpp:1621:12
    #3 0xb02028e in js::WasmMemoryObject::isShared() const js/src/wasm/WasmJS.cpp:1644
    #4 0xb02028e in js::WasmMemoryObject::volatileMemoryLength() const js/src/wasm/WasmJS.cpp:1634
    #5 0xaedcb0f in js::wasm::Instance::memCopy(js::wasm::Instance*, unsigned int, unsigned int, unsigned int) js/src/wasm/WasmInstance.cpp:401:28
    #6 0xeaf310f0  (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV js/src/vm/JSObject.h:114:16 in JSObject::getClass() const
==7894==ABORTING



Signature-wise this looks very similar to bug 1461291, marking s-s.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Julian Seward [:jseward]]

Reproduced.  At a first glance, this appears to have happened because we
made it into Instance::memCopy(..) with instance->memory() returning nullptr,
viz, I'd guess, without a valid memory to do the memCopy in.  Feels like
a validation problem back down the line.  I'll investigate.

I imagine Instance::memFill(..) can be made to fail in the same way.

Comment 3

[Julian Seward [:jseward]]

Attachment: bug1464477-crash-JSObject-getClass-1.diff

Indeed, the testcase has no memories but uses BulkMem.Copy anyway.
So it seems like a verification error.  This patch rejects it; more
details in the commit message.  Lars, is this the correct way to fix
it?

I think we will need to update the proposal spec to explicitly reject
such cases.

This also shows the verification test cases for BulkMem.Copy/Fill to
be inadequate in a couple of ways.  I'll file a followup to fix that.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8981352 [details] [diff] [review]
bug1464477-crash-JSObject-getClass-1.diff

Review of attachment 8981352 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this seems right.  The other memory ops get this test for free from the reading of the address, but since the address here is just a raw integer (for now), it's right to duplicate the test in these opcodes.

Comment 5

[Julian Seward [:jseward]]

(In reply to Julian Seward [:jseward] from comment #3)
> This also shows the verification test cases for BulkMem.Copy/Fill to
> be inadequate in a couple of ways.  I'll file a followup to fix that.

Bug 1465011.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc0e4096849fcee3a72b41bbf9a40ab509f06be
https://hg.mozilla.org/mozilla-central/rev/9fc0e4096849

Comment 7

[Ryan VanderMeulen [:RyanVM]]

What's the severity of this bug and which other branches are affected?

Comment 8

[Julian Seward [:jseward]]

This is due to inadequate input validation for Wasm modules that use the
currently experimental "bulk memory" extension facility.  The only effect is
to cause a read from zero, viz, immediate and terminating segfault.  I can't
see that it would corrupt local state (the user's profile, etc).

The underlying flaw was introduced at this point
user:        Julian Seward <[e-mail]>
date:        Fri May 04 23:14:09 2018 +0200
summary:     Bug 1446930 - Wasm: basic (OOL) implementation of memory.fill and memory.copy.  r=lth.

Comment 9

[Julian Seward [:jseward]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> What's the severity of this bug and which other branches are affected?

To answer directly:

* affected branches: I can't see the comment 8 commit in the beta tree,
  so it's only trunk.

* severity: I studied https://wiki.mozilla.org/Security_Severity_Ratings
  but can't see anything pertaining directly to null pointer reads. 
  Nevertheless I'd say the severity is low.

Comment 10

[Christian Holler (:decoder)]

Opening bug based on comment 8 and comment 9.