1708124 - Crash [@ js::ErrorObject::setFromWasmTrap()]

Status: VERIFIED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20210426-6f8320a4798f (opt build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

See attachment.

Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000055c074e9a44c in js::ErrorObject::setFromWasmTrap() ()
#1  0x000055c0750ff0c3 in ReportError(JSContext*, unsigned int) ()
#2  0x000055c0750f2b34 in WasmHandleTrap() ()
#3  0x000011ba17332745 in ?? ()
#4  0x0000000000000000 in ?? ()
rax	0xfffb0b4401f2ad01	-1394988165190399
rbx	0x50b4401f2ada0	1419761601916320
rcx	0x57a0a1381a812400	6314223939908346880
rdx	0x7fb1a33fd868	140400924809320
rsi	0xfffb000000000000	-1407374883553280
rdi	0x50b4401f2ada0	1419761601916320
rbp	0x7fb1a33fd4b0	140400924808368
rsp	0x7fb1a33fd4a0	140400924808352
r8	0x0	0
r9	0x7fb1a3e53000	140400935645184
r10	0x7fb1a3e53000	140400935645184
r11	0x0	0
r12	0x7fb1a33fd718	140400924808984
r13	0x7fb1a33fd540	140400924808512
r14	0x7fb1a3855000	140400929361920
r15	0x7fb02326f000	140394480726016
rip	0x55c074e9a44c <js::ErrorObject::setFromWasmTrap()+12>
=> 0x55c074e9a44c <_ZN2js11ErrorObject15setFromWasmTrapEv+12>:	mov    (%rdi),%rax
   0x55c074e9a44f <_ZN2js11ErrorObject15setFromWasmTrapEv+15>:	movzbl 0x13(%rax),%eax

This bug has been around for quite a bit but I was never able to get a testcase until now. The attached testcase only reproduces on opt (non-debug) builds and is slightly intermittent. Marking s-s due to deref of a bogus memory address.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20210428100720-1ea87880589f.
The bug appears to have been introduced in the following build range:

Start: 11fddf9b0949346c7f21e56fdb82c282e4fa009a (20210407070707)
End: b00dca6a499518027381e5ae68926c0afa5ac7c4 (20210407071858)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=11fddf9b0949346c7f21e56fdb82c282e4fa009a&tochange=b00dca6a499518027381e5ae68926c0afa5ac7c4

Comment 3

[Lars T Hansen [:lth]]

That range does not seem terribly likely, there are no wasm changes or wasm-related compiler changes there, though in principle I guess anything's possible.

Comment 4

[Lars T Hansen [:lth]]

Offending code:

(module
       (memory 1)
       (func $store (param i32) (param i64)
         (i64.store
          (local.get 0)
          (local.get 1)
         )
       ) (export "store" (func 0))
       (func $load (param i32) (result i64)
        (i64.load
         (local.get 0)
        )
       ) (export "load" (func 1))
        (func (export "assert_0") (result i32)
         i32.const 65536
i64.const 0x1234567887654321
         call $store
i32.const 1))

The store in $store is OOB and triggers the signal handler. Things work OK in a single global; presumably they go south in the presence of other globals (tc is elaborate). Looking into it.

Comment 5

[Lars T Hansen [:lth]]

Attachment: test.js reduced

This is much smaller and reproduces the crash (or anyway a crash) reliably on my system. I'm sure GC is somehow involved here; trying to remove unused arguments or intermediate function calls that have no semantic meaning removes the crash as well.

Comment 6

[Daniel Veditz [:dveditz]]

If this is correctly caught by the Wasm trap is it a security bug?

Comment 7

[Lars T Hansen [:lth]]

(In reply to Daniel Veditz [:dveditz] from comment #6)

If this is correctly caught by the Wasm trap is it a security bug?

It's too early to tell, but given that the program crashes with a wild non-null this pointer, I'm slightly nervous.

Comment 8

[Lars T Hansen [:lth]]

Two notes about the smaller TC:

• it crashes without any flags at all in a release build, js tc.js
• in a debug build it will eventually (< 1min) abort with the expected assertion failure, exn.isObject() && exn.toObject().is<ErrorObject>(), at /home/lhansen/m-u/js/src/wasm/WasmBuiltins.cpp:595 , with --fuzzing-safe (alone); without flags, it signals OOM

With rr and a debug build things become a little easier. We're in WasmHandleTrap with Trap::OutOfBounds as expected. We grab the activation and then the context from the activation. Down in ReportError, we find that the exn value is not an object (which is why the assertion fails), it is a string, "out of memory". This string is being thrown by js::ReportOutOfMemory in response to an allocation failure in js. But that OOM failure is itself a result of trying to report a wasm failure:

#0  js::ReportOutOfMemory (cx=0x7f4b70df5000) at js/src/vm/JSContext.cpp:257
#1  0x0000563e3895fb00 in js::gc::GCRuntime::tryNewTenuredThing<JSObject, (js::AllowGC)1> (cx=0x7f4b70df5000, 
    kind=js::gc::AllocKind::OBJECT12, thingSize=120) at js/src/gc/Allocator.cpp:367
...
#21 0x0000563e38532a93 in JS_ReportErrorNumberUTF8 (cx=0x7f4b70df5000, 
    errorCallback=0x563e37f98e20 <js::GetErrorMessage(void*, unsigned int)>, userRef=0x0, errorNumber=356)
    at /home/lhansen/m-u/js/src/jsapi.cpp:4752
#22 0x0000563e391d1ff8 in ReportError (cx=0x7f4b70df5000, errorNumber=356) at js/src/wasm/WasmBuiltins.cpp:587
#23 0x0000563e391bc17e in WasmHandleTrap () at js/src/wasm/WasmBuiltins.cpp:642

I think the bug here is an assumption in the wasm code that when JS_ReportErrorNumberUTF8 is called by ReportError in WasmBuiltins.cpp, either there will be no pending exception or the exception will be an exception object. But OOM breaks those rules because in that case a static string is thrown. This code was recently rewritten as part of the exception handling patch,

# HG changeset patch
# User Asumu Takikawa <[e-mail]>
# Date 1612326572 0
#      Wed Feb 03 04:29:32 2021 +0000
# Node ID 0d79003d2bab8f54911d8bf7780307ab0d466895
# Parent  81bb9db5e1392947bb8cb07f9f9a585574e7c931
Bug 1335652 - wasm exceptions part 12: stack walking for exceptions r=rhunt

In this patch, there's a more sophisticated check in HasCatchableException, it will not blithely assume that the exn is an exn object. At a minimum we need the same check here.

Comment 9

[Lars T Hansen [:lth]]

Asumu, just checking that you saw this one.

Comment 10

[Asumu Takikawa]

Thanks for the needinfo ping, I had missed this bug discussion. I agree about the need for an additional guard here and will attach a patch in a little bit.

Comment 11

[Asumu Takikawa]

Attachment: Bug 1708124 - Fix OOM case in Wasm ReportError r=rhunt

Comment 12

[Asumu Takikawa]

Attachment: Bug 1708124 - Add test case for ReportError OOM r=rhunt

Depends on D114146

Comment 13

[Ryan Hunt [:rhunt]]

Comment on attachment 9219924 [details]
Bug 1708124 - Fix OOM case in Wasm ReportError r=rhunt

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily, the commit adds a check for if we OOM'ed while reporting a JS error. It's not clear where the OOM may lead to bad results from reading the commit. The exploiter would need to correctly trigger an OOM during a wasm trap.

If this happens, we'll incorrectly reinterpret the js::Value for the "out of memory" JS string as a JS object and call setFromWasmTrap() on it, which will write to a slot in the invalid JS object. The pointer that we're performing the invalid write with is not under user control, so I don't think an exploit is easy here.

• 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?: 87+
• If not all supported branches, which bug introduced the flaw?: Bug 1335652
• 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's unlikely to cause regressions we're adding OOM handling for a function that missed it.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 9219924 [details]
Bug 1708124 - Fix OOM case in Wasm ReportError r=rhunt

sec-approval=dveditz

Thank you for separating out the testcase -- always a good idea with a security bug for flexibility.

In this bug you technically didn't need sec-approval because it's rated moderate, but we certainly appreciate it when people ask if there's any doubt. See the guidelines at https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html

I added the in-testsuite? flag so you can later set that to "+" when the test lands. We should be able to uplift this to Beta because we've got about 4 weeks left in the cycle and the patch is small and straightforward. Because the regression has now made it to Release status in 88 please wait to land the test until after the fix has hit Release.

Comment 15

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

Fix OOM case in Wasm ReportError r=rhunt
https://hg.mozilla.org/integration/autoland/rev/caf157f846d62511e21f0fb8eef79eb3574f4160
https://hg.mozilla.org/mozilla-central/rev/caf157f846d6

Comment 16

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20210504214950-3799c70b5773.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 17

[Ryan Hunt [:rhunt]]

Comment on attachment 9219924 [details]
Bug 1708124 - Fix OOM case in Wasm ReportError r=rhunt

Beta/Release Uplift Approval Request

• User impact if declined: A precisely triggered OOM while trapping in WebAssembly may trigger a write to a static location causing a crash.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We add a check for an OOM so that we perform the correct kind of cast while examining an error value.
• String changes made/needed:

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9219924 [details]
Bug 1708124 - Fix OOM case in Wasm ReportError r=rhunt

Approved for 89 beta 10, thanks.

Comment 19

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/166053456c0eee736f6bff59e7cb98737f351766

Comment 20

[Ryan Hunt [:rhunt]]

Four weeks have passed since 89 was released on June 1. Will land the tests now.

Comment 21

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

Add test case for ReportError OOM r=rhunt
https://hg.mozilla.org/integration/autoland/rev/2b5326c54de7fdc641a1de8e7658fefd4c75fb3b
https://hg.mozilla.org/mozilla-central/rev/2b5326c54de7