Closed Bug 1471427 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: Javascript: WebAssembly, enhancement)

60 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: mozillabugs, Assigned: luke)

References

Details

(Keywords: csectype-bounds, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main63+])

Attachments

(2 files)

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.
Flags: needinfo?(mozillabugs)
Group: core-security → javascript-core-security
Flags: sec-bounty?
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.)
(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.
(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.
(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.
Attached patch fix.patchSplinter Review
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.
Attachment #8990269 - Flags: feedback?
Attachment #8990269 - Flags: feedback?(jvarga)
Attachment #8990269 - Flags: feedback?(bugmail)
Attachment #8990269 - Flags: feedback?
(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)
(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.
Flags: needinfo?(mozillabugs)
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.
Attachment #8990269 - Flags: feedback?(jvarga)
Attachment #8990269 - Flags: feedback?(bugmail)
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
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.
Attachment #8990838 - Flags: review?(jvarga)
Attachment #8990838 - Flags: feedback?(bbouvier)
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 on attachment 8990838 [details] [diff] [review]
Use fallible allocation to abort less

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

Looks good, thanks.
Attachment #8990838 - Flags: review?(jvarga) → review+
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(); }`?
Attachment #8990838 - Flags: feedback?(bbouvier) → feedback+
(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.
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.
Flags: needinfo?(bugmail)
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.
Flags: needinfo?(bugmail)
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.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Group: javascript-core-security → core-security-release
Assignee: bugmail → luke
Target Milestone: --- → mozilla63
Flags: sec-bounty? → sec-bounty+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.