1673589 - Crash [@ ??] with SIGTRAP with WebAssembly

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 46a0e993f8bb (build with --enable-optimize --enable-debug, run with --fuzzing-safe --wasm-compiler=baseline --no-threads):

for (var eptxcm = 0; eptxcm < 29; ++eptxcm) {
    let { exports } = new WebAssembly.Instance(
    new WebAssembly.Module(wasmTextToBinary(`
    (module
        (func (import "module" "fn") (param f64 i32) (result i32 f64))
        (func (export "f") (result i32)
            f64.const 4.2
            i32.const 7
            call 0
            drop
        )
    )
    `)), { "module": { fn(f32, i32) { return [2, 7.3]; } } });
    exports.f()
}

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Lars T Hansen [:lth]]

Oh groan, on arm64 hardware... Looking at the test case, and the crash info, I'm guessing this is a problem in stubs but it could be in the callImport machinery too. It would have to be in InterpExit because JitExit can't handle multiple return values from JS. SIGTRAP could be a MOZ_CRASH or an explicit breakpoint emitted to signify unreachable code. I think a MOZ_ASSERT normally turns into SIGSEGV, in contrast.

What's the specific hardware here, do you know?

Comment 4

[Christian Holler (:decoder)]

(In reply to Lars T Hansen [:lth] from comment #3)

What's the specific hardware here, do you know?

This is on an AWS Graviton2 CPU.

Comment 5

[Lars T Hansen [:lth]]

That a loop is required in the test may suggest that GC needs to be triggered.

Comment 6

[Lars T Hansen [:lth]]

Julian, are you able to track down the crash location for this one on your RPi? If the breakpoint is one of ours then remember that on arm64, the breakpoint can take a payload and it's possible to hack up the code to distinguish the various breakpoints. But it may also be sufficient to stare at a larger context around the crash site to see what the code is doing.

Comment 7

[Julian Seward [:jseward]]

I can reproduce it, yes. I'll investigate in the morning.

Comment 8

[Julian Seward [:jseward]]

Some observations:

• the test case in comment 2 involves a 29-iteration loop. I could cause the failure at 17 iterations (viz, for (var eptxcm = 0; eptxcm < 17; ++eptxcm) {, but not below. Is there some GC threshold involving 16 iterations of something?

• I'll add crash-place disassembly, and some dynamically preceding instructions, in the next comment. It crashes in a pretty tiny function (40-ish insns), and that crash is preceded by two levels of return to this function. That is, two frames return, we are "back" in the crashing function, and it then crashes.

• Immediately after return to the crashing function, x29 is compared against 0xBAD, and found to be different. I assume 0xBAD is a magic value of some kind.

• In the trace shown in the next comment, the entire trace is "first execution of these instructions". That is, they have not been executed before. That is not the case prior to the first block shown in the trace, though.

Comment 9

[Julian Seward [:jseward]]

Attachment: Disassembly of crashing function

Comment 10

[Christian Holler (:decoder)]

(In reply to Julian Seward [:jseward] from comment #8)

Some observations:

• the test case in comment 2 involves a 29-iteration loop. I could cause the failure at 17 iterations (viz, for (var eptxcm = 0; eptxcm < 17; ++eptxcm) {, but not below. Is there some GC threshold involving 16 iterations of something?

On the machine I use, 8 iterations seem to be enough already.

Comment 11

[Julian Seward [:jseward]]

Attachment: Path leading up to the crash

Comment 12

[Julian Seward [:jseward]]

Referring to the disassembly in comment 9, it appears that the branch instruction immediately prior to the breakpoint instruction, that is, 0x00000718a62303fc: b.ls 0x718a6230404 is based on undefined data. Tracing back a few instructions, that would imply that not all of the 64 bits of the value returned from the call at 0x00000718a62303d8: bl 0x718a6230100 are defined.

I notice also, at the return point of that call instruction is what looks a lot like a stack-alignment check.

Comment 13

[Lars T Hansen [:lth]]

We should remember that there may be bug in various patch sets that may not have been tested well on this platform - multi-value, obviously, but also Dmitry's patches. A possible triage point here is to back up to before Dmitry's patches landed.

Comment 14

[Lars T Hansen [:lth]]

Julian says it's cross-platform. Looks like maybe the baseline compiler mis-handles the i32 return.

Comment 15

[Lars T Hansen [:lth]]

This is obscure but fairly nasty. Not sure how exploitable it is though. During cross-module calls via C++ we may end up storing an i32 value in a 64-bit slot without updating the high bits appropriately; later we'll load 64 bits on the assumption that they will have been updated. The problem would have come in with the multi-value rewrite, more likely than not. The logic here is a little knotty. On x64 and arm64, we want to zero-extend the 32-bit value to 64 bits before storing. On mips64 (and probably on riscv, which we do not support), the correct behavior is to sign-extend the value. It's not clear to us yet what all the invariants are and what the correct behavior is on 32-bit systems either. An audit is in order. Uplifts will be required.

Comment 16

[Julian Seward [:jseward]]

Attachment: for_discussion.diff

An initial patch for discussion. Per analysis/discussion yesterday, this is
far more than actually needed to fix the specific case here -- that can be
fixed with a one-liner. This is more general:

• it makes all the ToWebAssemblyValue_ functions always write 64 bits.

• as a result, both uses of ToWebAssemblyValue in UnpackResults are made
  safe.

• the revised ToWebAssemblyValue_i32 is revised to an in-principle
  how-would-we-also- handle-MIPS, assuming MIPS sign-extends (to be
  confirmed). We can prune the MIPS specifics out of this for a the current
  patch.

• In UnpackResults, there are now assertions to verify that the stack result
  area slots are always 64-bits long and are packed together, with no holes in
  between.

Does this seem along the right lines? Or do we want something much simpler?

Comment 17

[Julian Seward [:jseward]]

Attachment: Bug 1673589 - Crash [@ ??] with SIGTRAP with WebAssembly. r=lth.

During cross-module calls via C++ we may end up storing an i32 value in a
64-bit slot without updating the high bits appropriately; later we'll load 64
bits on the assumption that they will have been updated. This particular test
case fails because it tries to nan-box such a value, and being a debug build,
asserts that the top 32 bits are zero. Which they aren't.

This is general fix, aimed at all targets. In WasmInstance.cpp:

• it makes all the ToWebAssemblyValue_ functions always write 64 bits.

• as a result, both uses of ToWebAssemblyValue in UnpackResults are made safe.

• the revised ToWebAssemblyValue_i32 is revised to an in-principle
  how-would-we-also-handle-MIPS, assuming MIPS sign-extends (to be
  confirmed). We can prune the MIPS specifics out of this for a the current
  patch.

• In UnpackResults, there are now assertions to verify that the stack result
  area slots are always 64-bits long and are packed together, with no holes in
  between.

Comment 18

[Julian Seward [:jseward]]

Comment on attachment 9185090 [details]
Bug 1673589 - Crash [@ ??] with SIGTRAP with WebAssembly. r=lth.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Hard to say. I suspect it would be difficult. The effect of the bug is that when a wasm-to-JS cross-module call returns, and that call returns multiple values, a returned 32-bit integer might be extended out to 64 bits with the upper 32 bits being uninitialised bits from stack or heap. And/or, that such a value is later presented for nan-boxing, but because its upper 32 bits are undefined rather than zero, it might be interpreted as some other, non-number, kind of JS value -- maybe a pointer, even. Whether any such pointer could be caused to point to any specific place .. I don't know.
• 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?: The bug was probably introduced into m-c prior to May 2020.
• If not all supported branches, which bug introduced the flaw?: 1401675, maybe
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should be easy and low risk.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. We have test cases, although they are not in the patch. Patch is small and is changes only the very small part of the wasm implementation which directly relates to this problem (cross-module multi-value returns to wasm).

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 9185090 [details]
Bug 1673589 - Crash [@ ??] with SIGTRAP with WebAssembly. r=lth.

sec-approved, setting flag but please request uplift.

Comment 20

[Lars T Hansen [:lth]]

Looks like the regressing bug landed before 78 branched, so arguably we also need ESR.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

I tried to land this patch but hit conflicts in Lando. Given where we are in the cycle, I think we should probably punt on this until the next cycle.

Comment 22

[Tom Ritter [:tjr]]

Sure, sounds good

Comment 23

[Julian Seward [:jseward]]

I've made a rebased version of the patch and tested it locally on x86-linux, x64-linux and aarch64-linux. Is it too late to land it this cycle?

Comment 24

[Tom Ritter [:tjr]]

(In reply to Julian Seward [:jseward] from comment #23)

I've made a rebased version of the patch and tested it locally on x86-linux, x64-linux and aarch64-linux. Is it too late to land it this cycle?

Yeah, we're in RC week, because this is internally found, it'd be preferable to wait until next cycle, unless this because public somehow or was a big crasher or something similarly urgent.

Comment 25

[Lars T Hansen [:lth]]

(In reply to Tom Ritter [:tjr] (ni? for response to sec-[advisories/bounties/ratings/cves]) from comment #24)

(In reply to Julian Seward [:jseward] from comment #23)

I've made a rebased version of the patch and tested it locally on x86-linux, x64-linux and aarch64-linux. Is it too late to land it this cycle?

Yeah, we're in RC week, because this is internally found, it'd be preferable to wait until next cycle, unless this because public somehow or was a big crasher or something similarly urgent.

That's too bad. Julian, you'll want to sync up with Ryan on this because I have a patch from him that rewrites ToWebAssemblyValue and ToJSValue...

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Please attach a rebased patch for ESR78 and request approval on it when you get a chance.

Comment 27

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

https://hg.mozilla.org/integration/autoland/rev/ecf4bb7d1522c6d26f3b1eadbab20bb082ac28ab
https://hg.mozilla.org/mozilla-central/rev/ecf4bb7d1522

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9185090 [details]
Bug 1673589 - Crash [@ ??] with SIGTRAP with WebAssembly. r=lth.

Approved for 84.0b2.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[BugBot [:suhaib / :marco]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 31

[Julian Seward [:jseward]]

Attachment: Bug 1673589 - Crash [@ ??] with SIGTRAP with WebAssembly. r=lth.

Comment 32

[Julian Seward [:jseward]]

Comment 31 is a proposed patch for ESR78. It looks OK on try.

Comment 33

[Julian Seward [:jseward]]

Comment on attachment 9190969 [details]
Bug 1673589 - Crash [@ ??] with SIGTRAP with WebAssembly. r=lth.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is sec-high.
• User impact if declined: There's some possibility of an attacker being able cause memory corruption and possibly misinterpretation of the (runtime) type of Javascript values.
• Fix Landed on Version: 85
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low because it's for a currently obscure (maybe even experimental?) feature of wasm, viz, returning of multiple values by a wasm-to-JS call. And because, from the patch, it's fairly easy to argue that we correctly write the bits of memory that we missed before, without writing anywhere we shouldn't.
• String or UUID changes made by this patch: none

Comment 34

[Julien Cristau [:jcristau]]

Comment on attachment 9190969 [details]
Bug 1673589 - Crash [@ ??] with SIGTRAP with WebAssembly. r=lth.

approved for 78.6esr

Comment 35

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/96a892d40be2

Comment 36

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Bug appears to be fixed on mozilla-central 20210404214218-1ec31daa1ae0 but BugMon was unable to reproduce using mozilla-central 20201027044126-46a0e993f8bb.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 37

[BugBot [:suhaib / :marco]]

:jseward, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 38

[Marco Castelluccio [:marco]]

Sorry, bug in the bot.