Stream: git-wasmtime

Topic: wasmtime / PR #6028 Validate faulting addresses are valid...

Wasmtime GitHub notifications bot (Mar 15 2023 at 20:58):

alexcrichton opened PR #6028 from validate-segfault to main:

This commit adds a defense-in-depth measure to Wasmtime which is intended to mitigate the impact of CVEs such as GHSA-ff4p-7xrq-q5r8. Currently Wasmtime will catch SIGSEGV signals for WebAssembly code so long as the instruction which faulted is an allow-listed instruction (aka has a trap code listed for it). With the recent security issue, however, the problem was that a wasm guest could exploit a compiler bug to access memory outside of its sandbox. If the access was successful there's no real way to detect that, but if the access was unsuccessful then Wasmtime would happily swallow the SIGSEGV and report a nominal trap. To embedders, this might look like nothing is going awry.

The new strategy implemented here in this commit is to attempt to be more robust towards these sorts of failures. When a SIGSEGV is raised the faulting pc is recorded but additionally the address of the inaccessible location is also record. After the WebAssembly stack is unwound and control returns to Wasmtime which has access to a Store Wasmtime will now use this inaccessible faulting address to translate it to a wasm address. This process should be guaranteed to succeed as WebAssembly should only be able to access a well-defined region of memory for all linear memories in a Store.

If no linear memory in a Store could contain the faulting address, then Wasmtime now prints a scary message and aborts the process. The purpose of this is to catch these sorts of bugs, make them very loud errors, and hopefully mitigate impact. This would continue to not mitigate the impact of a guest successfully loading data outside of its sandbox, but if a guest was doing a sort of probing strategy trying to find valid addresses then any invalid access would turn into a process crash which would immediately be noticed by embedders.

While I was here I went ahead and additionally took a stab at #3120. Traps due to SIGSEGV will now report the size of linear memory and the address that was being accessed in addition to the bland "access out of bounds" error. While this is still somewhat bland in the context of a high level source language it's hopefully at least a little bit more actionable for some. I'll note though that this isn't a guaranteed contextual message since only the default configuration for Wasmtime generates SIGSEGV on out-of-bounds memory accesses. Dynamically bounds-checked configurations, for example, don't do this.

Testing-wise I unfortunately am not aware of a great way to test this. The closet equivalent would be something like an unsafe method Config::allow_wasm_sandbox_escape. In lieu of adding tests, though, I can confirm that during development the crashing messages works just fine as it took awhile on macOS to figure out where the faulting address was recorded in the exception information which meant I had lots of instances of recording an address of a trap not accessible from wasm.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Mar 15 2023 at 22:54):

alexcrichton updated PR #6028 from validate-segfault to main.

Wasmtime GitHub notifications bot (Mar 16 2023 at 00:00):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 00:00):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 00:00):

cfallin created PR review comment:

it's pre-existing terminology but "byte size" to me reads somewhat ambiguously (the size of a byte vs the size in bytes); "The size of memory in bytes" maybe?

Wasmtime GitHub notifications bot (Mar 16 2023 at 00:00):

cfallin created PR review comment:

Can we link the Gecko source (e.g. on searchfox) for reference?

Is repr(packed) equivalent to align=4 here because the current struct offset is already 4-aligned? If so, could we say this?

Wasmtime GitHub notifications bot (Mar 16 2023 at 00:00):

cfallin created PR review comment:

Outdated comment, I think (there is no addr parameter)?

Wasmtime GitHub notifications bot (Mar 16 2023 at 00:00):

cfallin created PR review comment:

s/case/cause/

Wasmtime GitHub notifications bot (Mar 16 2023 at 00:00):

cfallin created PR review comment:

I would call this faulting_addr maybe (otherwise it could be e.g. a fault code or something else). Likewise below on set_jit_trap

Wasmtime GitHub notifications bot (Mar 16 2023 at 00:00):

cfallin created PR review comment:

I suppose we aren't worried about linear-time behavior here because this is only within a store, not across all live instances in the engine; and way may have some number of instances (due to a component instantiation perhaps) but not an extremely large number. Can we note that here? And, are there circumstances in the future where this may be a problem?

Wasmtime GitHub notifications bot (Mar 16 2023 at 14:41):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 14:41):

alexcrichton created PR review comment:

Yeah that's the rough idea, that the total number of linear memories and instances is likely quite small per-store. I'm also sort of banking on everything being of the mindset "well it's ok if traps are a bit slower". If this becomes an issue though I think we can push more maps outward into the store to more efficiently (probably log(n) style) calculate which instance/memory a linear memory address belongs to.

Wasmtime GitHub notifications bot (Mar 16 2023 at 15:48):

fitzgen created PR review comment:

The only traps that are semi-hot are wasi exits, which don't have faulting addresses and so should never enter this function anyways, right?

Wasmtime GitHub notifications bot (Mar 16 2023 at 15:48):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 15:49):

fitzgen created PR review comment:

Maybe also suggest people reach out to us and link the security reporting/disclosure document?

Wasmtime GitHub notifications bot (Mar 16 2023 at 15:49):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 15:53):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 18:38):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 18:38):

alexcrichton created PR review comment:

repr(packed) reduces alignment to 1 I think, but it turns out that Rust supports repr(packed(4)) which exactly matches what C does so I've switched to that instead of the packed newtype wrapper.

Wasmtime GitHub notifications bot (Mar 16 2023 at 18:40):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 18:40):

alexcrichton created PR review comment:

Right yeah wasm exits won't execute this. I don't know of any where the loop would be hot other than synthetic "instantiate 10000 things and then repeatedly trap", but I'll definitely bolster the comments here.

Wasmtime GitHub notifications bot (Mar 16 2023 at 18:42):

alexcrichton updated PR #6028 from validate-segfault to main.

Wasmtime GitHub notifications bot (Mar 16 2023 at 18:42):

alexcrichton has enabled auto merge for PR #6028.

Wasmtime GitHub notifications bot (Mar 16 2023 at 19:00):

alexcrichton updated PR #6028 from validate-segfault to main.

Wasmtime GitHub notifications bot (Mar 16 2023 at 20:27):

alexcrichton updated PR #6028 from validate-segfault to main.

Wasmtime GitHub notifications bot (Mar 16 2023 at 20:27):

alexcrichton has enabled auto merge for PR #6028.

Wasmtime GitHub notifications bot (Mar 17 2023 at 14:32):

alexcrichton updated PR #6028 from validate-segfault to main.

Wasmtime GitHub notifications bot (Mar 17 2023 at 14:32):

alexcrichton has enabled auto merge for PR #6028.

Wasmtime GitHub notifications bot (Mar 17 2023 at 15:37):

alexcrichton merged PR #6028.

Last updated: Oct 23 2024 at 20:03 UTC