1350440 - Latent (?) write beyond bounds in wasm::DecodeName()

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[q1]

wasm::DecodeName() (js\src\wasm\WasmBinaryFormat.cpp) could cause an integer overflow if it were possible (maybe it is?) to create an ArrayBuffer or typed array of >= 4 GB.

The bug is on line 155, which overflows if |numBytes|, which is read directly from an untrusted .wasm file, is 0xffffffff. Because lines 152-53 require the file to contain at least |numBytes| bytes of data (in this case, 0xffffffff bytes) following the name length value, the total file size must be > 0xffffffff bytes.

Currently, WebAssembly.instantiate() (and WebAssembly.compile()) -- which lead to this code -- require an ArrayBuffer or typed array argument. But there appears to be no way to create an ArrayBuffer or typed array > 0xffffffff bytes. Directly creating such things does not work. Using XMLHttpRequest to create them does not work. Creating a Blob that large works, but it doesn't work to read the blob into an ArrayBuffer using FileReader (the maximum size of an ArrayBuffer is limited to 2 GB by ArrayBufferObject::create()).

The might, however. be some other way to accomplish this task, and any limits preventing its achievement might be relaxed in the future, so the bug should be fixed.

144: UniqueChars
145: wasm::DecodeName(Decoder& d)
146: {
147:     uint32_t numBytes;
148:     if (!d.readVarU32(&numBytes))
149:         return nullptr;
150: 
151:     const uint8_t* bytes;
152:     if (!d.readBytes(numBytes, &bytes))
153:         return nullptr;
154: 
155:     UniqueChars name(js_pod_malloc<char>(numBytes + 1));
156:     if (!name)
157:         return nullptr;
158: 
159:     memcpy(name.get(), bytes, numBytes);
160:     name[numBytes] = '\0';
161: 
162:     return name;
163: }

This bug would be exploitable only on a 64-bit build.

Comment 1

[q1]

Of course: "The bug is on line 155, which overflows if |numBytes|, which is read directly from an untrusted .wasm file, is 0xffffffff." should be followed by "In that case (and assuming the file contains enough data, which allows control to get to line 155 in the first place), line 159 writes 0xffffffff bytes of untrusted data beyond |name|'s end.

Comment 2

[q1]

Attachment: bug_752_poc_5.htm

Attached is a POC that shows wasm::DecodeName() reading |numBytes| directly from an untrusted Uint8Array. Attach a debugger to FF 52.0 and put a BP on DecodeName(), then load the POC to observe the read. If it were possible to make a Uint8Array (or ArrayBuffer) long enough, DecodeName() would also read the data from the array, then copy it beyond the end of its internal buffer.

Comment 3

[Andrew McCreight [:mccr8]]

I'm not sure if this can be triggered, but I'll just mark it moderate to be cautious.

Comment 4

[Luke Wagner [:luke]]

I don't think this can be triggered either: wasm binaries can be at most 2gb and so the readBytes() would fail.

FWIW, this is a super-fragile property rely on and future rewrites of this code (in FF53+) explicitly check numBytes immediately: https://hg.mozilla.org/releases/mozilla-beta/file/tip/js/src/wasm/WasmValidate.cpp#l697.

Comment 5

[Hannes Verschore [:h4writer]]

Should this get closed?

Comment 6

[Luke Wagner [:luke]]

Given that it's more-properly fixed in FF53 and prevented by 2gb-max ArrayBuffers, I think so.  Please feel free to reopen if we've missed something in our analysis.

Comment 7

[Daniel Veditz [:dveditz]]

Not awarding a bounty given comment 6.