1700690 - (CVE-2021-29945) WasmIonCompile multi-value return can clobber stack with f32 writes (was: Crash [@ ??] with WebAssembly on 32-bit)

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 3be60f42358a (build with --target=i686-pc-linux-gnu --enable-gczeal --enable-optimize --enable-debug --enable-fuzzing, run with --no-threads --fuzzing-safe --baseline-warmup-threshold=0 --disable-oom-functions test.js).

Backtrace:

==26528==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x00000010 (pc 0x41b4d933 bp 0xffd8cac8 sp 0xffd8cac8 T26528)
==26528==The signal is caused by a READ memory access.
==26528==Hint: address points to the zero page.
    #0 0x41b4d932  (<unknown module>)

This is essentially a crash on the heap with a near-null deref. 32-bit only, marking s-s until investigated.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

Yeah, read from 0x10($esi) where $esi==0.

Comment 3

[Lars T Hansen [:lth]]

Wasm code:

(module
  (type (;0;) (func (param i64) (result f32 f64)))
  (type (;1;) (func (param i32) (result i32 i32)))
  (import "fuzzFunc" "f1" (func (;0;) (type 0)))
  (func $main (type 1) (param i32) (result i32 i32)
    i64.const -1
    i64.const 41
    i64.div_s
    i64.const 39
    call 0
    i64.const 0
    i64.const 68
    i64.div_u
    unreachable)
  (export "main" (func $main)))

with imported function:

function(a, b, c, d) {
    return [b + c, d];
}

Related to multi-value return, so this bug has probably been with us for some time.

Comment 4

[Lars T Hansen [:lth]]

Ion-specific and easily reproducible, and has to do with multi-value return in the presence of other callouts to the runtime.

Simplified self-contained minimal-ish test case follows.

My config script:

export CC="clang -m32 -msse2 -mfpmath=sse"
export CXX="clang++ -m32 -msse2 -mfpmath=sse"
../configure --target=i686-pc-linux --enable-debug --disable-optimize --without-intl-api

Run as dist/bin/js test.js (no flags needed) or with --no-ion --no-baseline --no-blinterp --wasm-compiler=ion:

let buffer = wasmTextToBinary(`
(module
  (import "" "f" (func $f (result f32 f64)))
  (func $main
    i64.const 1
    i64.const 1
    i64.div_s
    call $f
    i64.const 1
    i64.const 1
    i64.div_s
    unreachable)
  (export "main" (func $main)))
`);
let module = new WebAssembly.Module(buffer);
let instance = new WebAssembly.Instance(module, {"": {"f":function() { return [0, 0] }}})
print(instance.exports.main());

If the result types of $f are the same (either f32 or f64), or if either division is removed, or are replaced by 32-bit divisions, we reach the unreachable. If we use the baseline compiler, we reach the unreachable.

64-bit division should be implemented by callouts on this platform and multi-value returns from JS to wasm must take the interpreter path, so there are lots of moving parts here.

Comment 5

[Lars T Hansen [:lth]]

The crash location is the third instruction in the stub for the second division:

   0x2418c930:	push   %ebp
   0x2418c931:	mov    %esp,%ebp
=> 0x2418c933:	mov    0x10(%esi),%ecx
   0x2418c936:	mov    0xdc(%ecx),%ecx
   0x2418c93c:	movl   $0x41,0x40(%ecx)
   0x2418c943:	or     $0x1,%ebp
   ...

$esi is the TLS register. Looking back up in the calling function, we find that it is read from the top of the frame, right above the area that is used for the multi-value return. Every indication is that the multi-value return scribbles over the saved TLS in the frame. In the frame (when no space has been allocated for outgoing args and no words have been pushed temporarily) the TLS is at sp+24. The return area address is computed as sp+20, leaving four bytes. The values stored in this area should be all but the last [sic] return value, ie, we should store a single f32 value. So we have enough space, but the question is, what does the code actually do?

Drilling down, we get to UnpackResults in WasmInstance.cpp, which calls ToWebAssemblyValue with a type of F32 and mustWrite64=true. Looking at the location, as a uint32_t array, the second element is definitely the tls pointer, and it will be clobbered.

So why is mustWrite64==true? This is because, up in UnpackResults, result_size==8. The reason a float is 8 bytes is because on x86, a float is pushed as 8 bytes (see comments in ABIResult in WasmStubs.h). Ergo the problem here is that the result pointer is incorrectly computed, it should make more space. But is the problem that the area set aside for results is too small, or is the pointer computation off?

It looks like the area size computation is off. It gets its slot sizes from MIRTypeToSize which will return 4 for float on x86. At the end of the computation in passStackResultAreaCallArg in WasmIonCompile.cpp, stackResultArea->byteSize() == 4, which is too small. It looks like this is specific to Ion; Baseline uses a different mechanism based on the ABIResultIter and its stackBytesConsumedSoFar() mechanism.

Comment 6

[Lars T Hansen [:lth]]

All recent versions are likely affected (on x86-only, barring further evidence). Not sure about ESR78 - the mustWrite64 logic post-dates that and may be part of the puzzle here.

Comment 7

[Lars T Hansen [:lth]]

Attachment: Bug 1700690 - Fix. r?jseward

Comment 8

[Lars T Hansen [:lth]]

The current patch is a very local fix that addresses the area size computation directly, it updates the MIR code to get the correct size via ABIResult. With this in place, the generated code looks reasonable and the test reaches the unreachable instruction as expected. This should really only have an effect on x86-32, and only for float32 multi-value returns; for all other platforms, all sizes should remain what they were. (The ABIResult push size for float32 on arm32 is 32 bits, and is unchanged.)

With this patch, all jit-tests continue to pass on x86 and arm32.

Comment 9

[Lars T Hansen [:lth]]

So how could this be abused? The version of ToWebAssemblyValue that writes an f32 will store the f32 in the first word and then if mustWrite64==true will store a zero value in the second (higher) word, thus always overwriting with zeroes.

On this platform, pointers are four bytes, so that overwrite will never partially clobber something, it will always overwrite an entire pointer if it overwrites anything. This could lead to a crash if the pointer is assumed to be non-null or a failing null check; no drama in either case. If the pointer is used as the base pointer for an access with a large-ish offset it could in principle be used to access memory (because the 32-bit heap is small). If the access is from user-controlled code (ie compiled wasm) the compromised pointer would have to be the heap pointer or a reftype (eg, we manage to clobber a struct* right above the return area and then access that struct with a very large offset). But very large offsets off of structs will result in some pointer chasing that will crash (to get to the out-of-line part of the object), and anyway the GC feature is behind a flag and only in nightly. Clobbering the heap pointer also seems hard, one would have to compromise the tls through some other means. That leaves the possibility that the engine makes direct use of the pointer with a large-ish offset. It's not completely obvious what that would be, we don't store pointers used in that way on the stack normally.

The overwrite could clobber part of an int64. That could cause an optimized-out integer range check to be bypassed in principle but I don't know if we have any of those on i64. Hard to know where to go with that too because i64 does not participate in memory computations.

In conclusion, we want to fix this but it's probably not terribly dramatic.

Comment 10

[Lars T Hansen [:lth]]

After being prompted by decoder, I dug around a little more to look at the wasm <-> wasm case, and though that has the same bug in that it allocates too little stack on the caller side, it doesn't provoke the same behavior because compiled wasm does not clobber the unused four bytes for the eight-byte f32 slot when it stores the f32 into the slot. Thus the crash is only provoked when we call out to JS so that ToWebAssemblyValue gets to process & store the value and overwrite the extra word.

Furthermore, though too little space is allocated, the offsets within the space are computed correctly, so the correct values are observed in the wasm <-> wasm case.

Comment 11

[Lars T Hansen [:lth]]

Attachment: Bug 1700690 - Add clarifying comment. r?jseward

To land once the bug has been opened up.

Depends on D109701

Comment 12

[Lars T Hansen [:lth]]

sec-moderate based on comment 9, on the assumption that it may be possible to use a spoofed null pointer as the base for an access with a large offset to access actual memory, but that it is not very plausible.

Comment 13

[Lars T Hansen [:lth]]

It was bug 1673589 that introduced the zeroing that causes the crash in this bug. That landed in FF85, so I will assume that FF85+ is affected for this problem. Unfortunately, that bug also landed a backport to ESR78, so ESR78 could be similarly affected.

However, the test case does not crash ESR78.

(Should it become necessary to fix ESR78, the patch on the present bug grafts cleanly onto ESR78 and builds.)

Comment 14

[Lars T Hansen [:lth]]

I tested Release and Beta shells, they are both vulnerable to the test case, and are both fixed by the patch (which applies cleanly via hg graft).

Comment 15

[Lars T Hansen [:lth]]

Comment on attachment 9211443 [details]
Bug 1700690 - Fix. r?jseward

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Probably tricky, if at all possible; see comment 9.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: FF85 forward
• If not all supported branches, which bug introduced the flaw?: "It's complicated." See comment 13.
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: It needs no additional testing.

Comment 16

[Tom Ritter [:tjr]]

Comment on attachment 9211443 [details]
Bug 1700690 - Fix. r?jseward

Approved to land on Nightly/Beta. We should figure out the situation on ESR: either it's safe to land the patch because it's not concerning stability-wise (whether or not it's exploitable), or we don't need to land it because ESR because it's not actually affected.

Comment 17

[Tom Ritter [:tjr]]

Gonna mark ESR 78 affected so it's still tracked.

Comment 18

[Lars T Hansen [:lth]]

FWIW I believe the patch should be safe to land on ESR78, it did apply cleanly with hg graft.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/aa0ebe849f8b798619b0663b30ac0caca2cf1c0f

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/aa0ebe849f8b

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9211443 [details]
Bug 1700690 - Fix. r?jseward

Approved for 78.10esr also.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e840029f72ef

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/548acfe955afeaedd326cc55a281542a57da589a

Comment 24

[Frederik Braun [:freddy]]

Attachment: advisory.txt

I believe this bug would be included in our roll-up announcement but the generator script is faling me. I'll write an advisory text for this after all, even if we might not end up using it..

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a7bbe440fe7
Add clarifying comment.  r=jseward DONTBUILD

Comment 26

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9a7bbe440fe7