Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic in wasmtime::component::Component::from_binary with a truncated WASM component file #8322

Closed
maxbrunsfeld opened this issue Apr 10, 2024 · 4 comments
Labels
bug Incorrect behavior in the current implementation that needs fixing

Comments

@maxbrunsfeld
Copy link
Contributor

Test Case

Wasm file: truncated.wasm.zip

Steps to Reproduce

const TRUNCATED_WASM: &[u8] = include_bytes!("truncated.wasm");

async fn test() {
    let mut config = wasmtime::Config::new();
    config.wasm_component_model(true);
    config.async_support(true);

    let engine = wasmtime::Engine::new(&config).unwrap();

    wasmtime::component::Component::from_binary(&engine, TRUNCATED_WASM).await;
}

Expected Results

The Component::from_binary call should return an Err value.

Actual Results

Thread "main" panicked with "range end index 267037 out of range for slice of length 253952" at ~.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmtime-environ-19.0.0/src/component/translate.rs:537:46

   5: core::panicking::panic_fmt
             at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/panicking.rs:72:14
   6: core::slice::index::slice_end_index_len_fail_rt
             at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/slice/index.rs:76:5
      core::slice::index::slice_end_index_len_fail
             at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/slice/index.rs:68:9
   7: <core::ops::range::Range<usize> as core::slice::index::SliceIndex<[T]>>::index
             at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/slice/index.rs:394:13
      core::slice::index::<impl core::ops::index::Index<I> for [T]>::index
             at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/slice/index.rs:18:9
      wasmtime_environ::component::translate::Translator::translate_payload
             at /home/luke/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmtime-environ-19.0.0/src/component/translate.rs:537:46
   8: wasmtime_environ::component::translate::Translator::translate
             at /home/luke/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmtime-environ-19.0.0/src/component/translate.rs:329:19
   9: wasmtime::compile::build_component_artifacts
             at /home/luke/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmtime-19.0.0/src/compile.rs:127:9
      wasmtime::runtime::component::component::Component::from_binary
             at /home/luke/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmtime-19.0.0/src/runtime/component/component.rs:234:41

Versions and Environment

Wasmtime version or commit: 19.0.0
Operating system: macOS
Architecture: aarch64

Extra Info

  • We produced this binary by taking a valid wasm component, and then truncating it a certain offset.
  • We can work around this bug for now by validating the components with wasmparser before attempting to instantiate them.
@maxbrunsfeld maxbrunsfeld added the bug Incorrect behavior in the current implementation that needs fixing label Apr 10, 2024
@fitzgen
Copy link
Member

fitzgen commented Apr 10, 2024

Thanks!

FWIW, also reproduces with

cargo run -- compile testcase.wasm

@fitzgen
Copy link
Member

fitzgen commented Apr 10, 2024

Fix in #8323

@maxbrunsfeld do you want us to cut a point release with this fix?

fitzgen added a commit to fitzgen/wasm-tools that referenced this issue Apr 10, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 10, 2024
* Handle out-of-bounds component sections

Fixes #8322

* Add a test that trancated component binaries don't cause panics
fitzgen added a commit to fitzgen/wasmtime that referenced this issue Apr 10, 2024
* Handle out-of-bounds component sections

Fixes bytecodealliance#8322

* Add a test that trancated component binaries don't cause panics
@maxbrunsfeld
Copy link
Contributor Author

That'd be great! Thanks @fitzgen.

@fitzgen
Copy link
Member

fitzgen commented Apr 10, 2024

Backport in #8325

alexcrichton pushed a commit that referenced this issue Apr 11, 2024
* Handle out-of-bounds component sections (#8323)

* Handle out-of-bounds component sections

Fixes #8322

* Add a test that trancated component binaries don't cause panics

* Add release notes
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this issue Apr 11, 2024
* Handle out-of-bounds component sections

Fixes bytecodealliance#8322

* Add a test that trancated component binaries don't cause panics
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this issue Apr 11, 2024
* Handle out-of-bounds component sections

Fixes bytecodealliance#8322

* Add a test that trancated component binaries don't cause panics
alexcrichton added a commit that referenced this issue Apr 11, 2024
* cranelift: Include clobbers and outgoing args in stack limit (#8301)

When we compute the amount of space that we need in a stack frame for
the stack limit check, we were only counting spill-slots and explicit
stack-slots. However, we need to account for all uses of the stack which
occur before the next stack limit check. That includes clobbers and any
stack arguments we want to pass to callees.

The maximum amount that we could have missed by is essentially bounded
by the number of arguments which could be passed to a function. In
Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in
`wasmparser::limits`, which is set to 1,000, and the largest arguments
are 16-byte vectors, so this could undercount by about 16kB.

This is not a security issue according to Wasmtime's security policy
(https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html)
because it's the embedder's responsibility to ensure that the stack
where Wasmtime is running has enough extra space on top of the
configured `max_wasm_stack` size, and getting within 16kB of the host
stack size is too small to be safe even with this fixed.

However, this was definitely not the intended behavior when stack limit
checks or stack probes are enabled, and anyone with non-default
configurations or non-Wasmtime uses of Cranelift should evaluate whether
this bug impacts your use case.

(For reference: When Wasmtime is used in async mode or on Linux, the
default stack size is 1.5MB larger than the default WebAssembly stack
limit, so such configurations are typically safe regardless. On the
other hand, on macOS the default non-async stack size for threads other
than the main thread is the same size as the default for
`max_wasm_stack`, so that is too small with or without this bug fix.)

* fix: bindgen trappable_errors using unversion/versioned packages (#8305)

Signed-off-by: Brian H <brian.hardock@fermyon.com>

* Cranelift: Do not dedupe/GVN bitcasts from reference values (#8317)

* Cranelift: Do not dedupe/GVN bitcasts from reference values

Deduping bitcasts to integers from references can make the references no long
longer live across safepoints, and instead only the bitcasted integer results
would be. Because the reference is no longer live after the safepoint, the
safepoint's stack map would not have an entry for the reference, which could
result in the collector reclaiming an object too early, which is basically a
use-after-free bug. Luckily, we sandbox the GC heap now, so such UAF bugs aren't
memory unsafe, but they could potentially result in denial of service
attacks. Either way, we don't want those bugs!

On the other hand, it is technically fine to dedupe bitcasts *to* reference
types. Doing so extends, rather than shortens, the live range of the GC
reference. This potentially adds it to more stack maps than it otherwise would
have been in, which means it might unnecessarily survive a GC it otherwise
wouldn't have. But that is fine. Shrinking live ranges of GC references, and
removing them from stack maps they otherwise should have been in, is the
problematic transformation.

* Add additional logging and debug asserts for GC stuff

* Handle out-of-bounds component sections (#8323)

* Handle out-of-bounds component sections

Fixes #8322

* Add a test that trancated component binaries don't cause panics

---------

Signed-off-by: Brian H <brian.hardock@fermyon.com>
Co-authored-by: Jamey Sharp <jsharp@fastly.com>
Co-authored-by: Brian <brian.hardock@fermyon.com>
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this issue Apr 11, 2024
* Handle out-of-bounds component sections

Fixes bytecodealliance#8322

* Add a test that trancated component binaries don't cause panics
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this issue Apr 11, 2024
* Handle out-of-bounds component sections

Fixes bytecodealliance#8322

* Add a test that trancated component binaries don't cause panics
alexcrichton pushed a commit to alexcrichton/wasmtime that referenced this issue Apr 11, 2024
* Handle out-of-bounds component sections

Fixes bytecodealliance#8322

* Add a test that trancated component binaries don't cause panics
alexcrichton added a commit that referenced this issue Apr 11, 2024
* Handle out-of-bounds component sections

Fixes #8322

* Add a test that trancated component binaries don't cause panics

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
alexcrichton added a commit that referenced this issue Apr 11, 2024
* Handle out-of-bounds component sections

Fixes #8322

* Add a test that trancated component binaries don't cause panics

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
github-merge-queue bot pushed a commit to bytecodealliance/wasm-tools that referenced this issue Apr 11, 2024
…1492)

* Ensure that we have the full length of component and module sections

Fixes the root cause of bytecodealliance/wasmtime#8322

* Extend validation fuzzer to check that payloads are in bounds

* Don't check module/component section ranges; rename to `unchecked_range`; document

* rustfmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing
Projects
None yet
Development

No branches or pull requests

2 participants