1471427 - Latent (?) write beyond bounds in WasmCompiledModuleStream::onCompilationComplete()

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[mozillabugs]

WasmCompiledModuleStream::onCompilationComplete() (dom\indexedDB\IDBObjectStore.cpp) might be able to write beyond bounds. The bug is that line 459 computes the |size_t| |compiledSize| (a 64 bit quantity on 64-bit builds) but line 462 implicitly truncates |compiledSize| to 32 bits and uses that smaller quantity to reserve memory. Lines 464-65, however, then write the untruncated number of bytes into that memory:

442:   void
443:   onCompilationComplete() override
444:   {
445:     if (!IsOnOwningThread()) {
446:       MOZ_ALWAYS_SUCCEEDS(mOwningThread->Dispatch(NewCancelableRunnableMethod(
447:         "WasmCompiledModuleStream::onCompilationComplete",
448:         this,
449:         &WasmCompiledModuleStream::onCompilationComplete)));
450:       return;
451:     }
452: 
453:     if (NS_FAILED(mStatus) || !mCallback) {
454:       return;
455:     }
456: 
457:     MOZ_ASSERT(mModule);
458: 
459:     size_t compiledSize = mModule->compiledSerializedSize();
460: 
461:     nsCString compiled;
462:     compiled.SetLength(compiledSize);
463: 
464:     mModule->compiledSerialize(
465:       reinterpret_cast<uint8_t*>(compiled.BeginWriting()), compiledSize);
466: 
467:     MOZ_ALWAYS_SUCCEEDS(NS_NewCStringInputStream(getter_AddRefs(mStream),
468:                                                  compiled));
469: 
470:     mModule = nullptr;
471: 
472:     CallCallback();
473:   }

This bug appears to be latent, because DecodePreamble() in WasmValidate.cpp fails if the wasm module's file size is > |MaxModuleBytes| (currently 1 GB, and there's no security warning on the definition). However, the serialized stream include a variety of metadata that's implied by the wasm module and its environment, but which is not present in the wasm module itself. Therefore, it might be possible to compute a |compileSize| that's >= 4GB. I will attempt to do so.

If this write can occur, the data written is largely under the attacker's control.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Wow, that's a great find. I think a stopgap fix will be to return early in this function if the size_t is > UINT32_MAX. It might be a bigger issue on 32-bits architectures (x86 and ARM), where the size_t is a 32-bits integer, if I remember correctly. (I'll work on this fix tomorrow.)

Comment 2

[mozillabugs]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Wow, that's a great find. I think a stopgap fix will be to return early in
> this function if the size_t is > UINT32_MAX. It might be a bigger issue on
> 32-bits architectures (x86 and ARM), where the size_t is a 32-bits integer,
> if I remember correctly....

In that case, an overflow could occur inside compiledSerializedSize(), with the same result: underallocation and a write beyond bounds, so probably that function should use something like CheckedInt<>, too.

BTW, I am working on a POC, and currently have |compiledSize| up to 0x1992xxxx bytes with just 64M of WASM S-code, no significant exports, and no significant metadata.

Comment 3

[mozillabugs]

(In reply to mozillabugs from comment #2)
> In that case, an overflow could occur inside compiledSerializedSize(), with
> the same result: underallocation and a write beyond bounds, so probably that
> function should use something like CheckedInt<>, too.

Ack! This is incorrect. On 32 bit builds, you have a 32-bit address space, so you can't practically allocate enough stuff to cause a |size_t| (which is also 32 bits) to overflow.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to mozillabugs from comment #3)
> (In reply to mozillabugs from comment #2)
> > In that case, an overflow could occur inside compiledSerializedSize(), with
> > the same result: underallocation and a write beyond bounds, so probably that
> > function should use something like CheckedInt<>, too.
> 
> Ack! This is incorrect. On 32 bit builds, you have a 32-bit address space,
> so you can't practically allocate enough stuff to cause a |size_t| (which is
> also 32 bits) to overflow.

You're right; that's one thing less to worry about, thanks for noticing.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

asuth/janv: I don't often hack on Gecko (Spidermonkey hacker here), but it seems that this would be a sufficient stopgap fix, until we get something better (in particular, since this is a streaming API, maybe the bytes could be streamed in chunks of 4 GB or less). This compiles as intended.

The code is ifdef'd on 64 bits platform to avoid a silly tautological comparison warning on 32 bits, where size_t is uint32_t, so size_t(x) > UINT32_MAX would be always false.

I've used out of memory as the reported error, because we tend to do this in Spidermonkey when something gets so out of range that it wouldn't fit in other data structures. But this might not be the most appropriate, and I'm happy to use any other error you think would be a better fit here.

Also not sure whether the callback needs to be called here or not, in this case.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

(see comment 5 for details; I blame Bugzilla for silently not setting the fieldback? fields in comment 5 because they were not cc'd on the bug)

Comment 7

[mozillabugs]

(In reply to mozillabugs from comment #2)
> BTW, I am working on a POC, and currently have |compiledSize| up to
> 0x1992xxxx bytes with just 64M of WASM S-code, no significant exports, and
> no significant metadata.

It looks like this bug probably is latent. https://bugzilla.mozilla.org/show_bug.cgi?id=1444668 limited the maximum size of compiled WASM code to 1 GB (|MaxCodeBytesPerProcess|)in 64-bit builds. Other metadata (like the path to the .wasm file and its associated .js file) add to this, but I'm pretty sure that the limits on these strings are ~2^29 bytes each (js::MaxStringLength * sizeof (wchar)), so the maximum paths length is 3x this (the metadata contains 3 paths). This gives a probable maximum |compiledSize| of ~0x80000000 + *0x20000000*3) = ~0xe0000000, which would be safe.

Probably it would be good to update the comment in \js\src\jit\ProcessExecutableMemory.h, which currently says:

   // Limit on the number of bytes of executable memory to prevent JIT spraying
   // attacks.

to something more emphatic, like:
 
   // Limit on the number of bytes of executable memory to prevent JIT spraying
   // attacks, including those based upon overflows or truncation. Please
   // change these values only after thoroughly evaluating potential attacks.
   // See, e.g., https://bugzilla.mozilla.org/show_bug.cgi?id=1444668 and
   // https://bugzilla.mozilla.org/show_bug.cgi?id=1471427 for some examples
   // of these attacks.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8990269 [details] [diff] [review]
fix.patch

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

Restating my understanding of the problem:
- For all our nsTString classes, the `size_type` is uint32_t per https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/xpcom/string/nsTStringRepr.h#121.
- We pass in a size_t to the uint32_t-taking `SetLength(size_type aNewLength);` which truncates.  Oddly, this doesn't seem to result in a compiler warning for me for a DEBUG build with my build settings, which is concerning...

Patch-wise:
- I believe we do need to call the callback when we take this branch.
- I think the best mechanism to use for dealing with this type of integer overflow issue is mfbt's CheckedInt type.

I've prepared a revised patch that I believe accomplishes this, and will put up for counter-review in a sec.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Use fallible allocation to abort less

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8990838 [details] [diff] [review]
Use fallible allocation to abort less

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

Note that this changes us from using infallible nsCString::SetLength() to fallible.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Shoot, I didn't mean to take ownership; bzexport did that.  Feel free to re-claim and drive the landing if you're so inclined, :bbouvier ;)

Comment 12

[Jan Varga [:janv]]

Comment on attachment 8990838 [details] [diff] [review]
Use fallible allocation to abort less

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

Looks good, thanks.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8990838 [details] [diff] [review]
Use fallible allocation to abort less

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

Thanks for taking it over, and happy you did so; I do not have much knowledge about this particular code. One suggestion and one question below.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +462,5 @@
>      nsCString compiled;
> +
> +    // Constraints on WASM size elsewhere in the codebase should avoid the
> +    // following from happening, but we need to check that:
> +    // - We're not overflowing the string's numeric type (size_type is currently

nit: the string size's numeric type

(I'd remove the text in parenthesis, which is a blackbox detail and would need an update, would this change in the future)

@@ +471,5 @@
> +    if (!cstrSize.isValid() ||
> +        !compiled.SetLength(cstrSize.value(), fallible)) {
> +      mStatus = NS_ERROR_OUT_OF_MEMORY;
> +      mModule = nullptr;
> +      CallCallback();

Just a question to check my understanding: a few lines above, there's a check `if (NS_FAILED(mStatus)` that doesn't call the callback; as this above check is defined, there might not even be a callback. But should the above check be split as `if (!mCallback) { return; }` and then `if (NS_FAILED(mStatus)) { CallCallback(); }`?

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> @@ +471,5 @@
> > +    if (!cstrSize.isValid() ||
> > +        !compiled.SetLength(cstrSize.value(), fallible)) {
> > +      mStatus = NS_ERROR_OUT_OF_MEMORY;
> > +      mModule = nullptr;
> > +      CallCallback();
> 
> Just a question to check my understanding: a few lines above, there's a
> check `if (NS_FAILED(mStatus)` that doesn't call the callback; as this above
> check is defined, there might not even be a callback. But should the above
> check be split as `if (!mCallback) { return; }` and then `if
> (NS_FAILED(mStatus)) { CallCallback(); }`?

The callback is invoked when mStatus transitions to an NS_FAILED state from NS_OK (or we reach the end of onCompilationComplete and still are NS_OK).  The mStatus check earlier in onCompilationComplete is necessary because the following series of events can happen:
- AsyncWait() is invoked, mStatus is still NS_OK, mModule->notifywWhenCompilationComplete(this) invoked which will eventually result in onCompilationComplete() being invoked
- The consumer of the stream invokes Close()/CloseWithStatus() which will set mStatus and invoke the callback and clear it.  (This will also proactively drop the mModule reference, but it appears the Tier2GeneratorTaskImpl acquires its own RefPtr to the Module, so the Module will stay alive until the notification fires.)
- onCompilationComplete is now called and the object is in a state where it doesn't actually want to do anything further with the module, but instead early return.

And so the reason we invoke the callback in the new code is that we're making the transition from NS_OK to NS_FAILED.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Makes sense. Just to be very explicit: asuth, since you've written the patch and know better about this code, I'm happy to let you take ownership and drive landing here.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Somewhat duplicating what I just said on https://bugzilla.mozilla.org/show_bug.cgi?id=1473863#c7, I think the fix for bug 1469395 may moot this and the analysis in comment 7 makes me believe the bug is not properly exploitable due to the 1gig cap, and so perhaps we should just let that removal code land and resolve these bugs as fixed when it does.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

I confirm that bug 1469395 has mooted this and we don't need a fix anymore because the original code has been deleted. Not sure what's the right resolved status is, in this sort of situation; let's say fixed by the code removal as a first attempt.