From c132ce70a95e8f7a873682bcc3cc22aa603ca9ad Mon Sep 17 00:00:00 2001 From: Marcin S Date: Thu, 19 Jan 2023 21:01:29 +0100 Subject: [PATCH 1/4] Fix potential huge allocation as a result of `validate_block` output --- Cargo.lock | 2 ++ client/executor/Cargo.toml | 1 + client/executor/runtime-test/Cargo.toml | 1 + client/executor/runtime-test/src/lib.rs | 14 ++++++++ client/executor/src/integration_tests/mod.rs | 37 +++++++++++++++++++- client/executor/wasmtime/src/runtime.rs | 17 +++++++-- 6 files changed, 69 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dbb046ccd86af..348f23d736d37 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8270,6 +8270,7 @@ name = "sc-executor" version = "0.10.0-dev" dependencies = [ "array-bytes", + "assert_matches", "criterion", "env_logger", "lru", @@ -8842,6 +8843,7 @@ dependencies = [ "sp-core", "sp-io", "sp-runtime", + "sp-runtime-interface", "sp-std", "substrate-wasm-builder", ] diff --git a/client/executor/Cargo.toml b/client/executor/Cargo.toml index 4604b976b4248..8bedcd3edcff3 100644 --- a/client/executor/Cargo.toml +++ b/client/executor/Cargo.toml @@ -35,6 +35,7 @@ sp-wasm-interface = { version = "7.0.0", path = "../../primitives/wasm-interface [dev-dependencies] array-bytes = "4.1" +assert_matches = "1.3.0" wat = "1.0" sc-runtime-test = { version = "2.0.0", path = "runtime-test" } substrate-test-runtime = { version = "2.0.0", path = "../../test-utils/runtime" } diff --git a/client/executor/runtime-test/Cargo.toml b/client/executor/runtime-test/Cargo.toml index e99f3caa9447e..07626c2343361 100644 --- a/client/executor/runtime-test/Cargo.toml +++ b/client/executor/runtime-test/Cargo.toml @@ -16,6 +16,7 @@ targets = ["x86_64-unknown-linux-gnu"] sp-core = { version = "7.0.0", default-features = false, path = "../../../primitives/core" } sp-io = { version = "7.0.0", default-features = false, features = ["improved_panic_error_reporting"], path = "../../../primitives/io" } sp-runtime = { version = "7.0.0", default-features = false, path = "../../../primitives/runtime" } +sp-runtime-interface = { version = "7.0.0", default-features = false, path = "../../../primitives/runtime-interface" } sp-std = { version = "5.0.0", default-features = false, path = "../../../primitives/std" } [build-dependencies] diff --git a/client/executor/runtime-test/src/lib.rs b/client/executor/runtime-test/src/lib.rs index fc98d1909d00d..ef9f46074ce74 100644 --- a/client/executor/runtime-test/src/lib.rs +++ b/client/executor/runtime-test/src/lib.rs @@ -30,6 +30,8 @@ use sp_runtime::{ traits::{BlakeTwo256, Hash}, }; +use sp_runtime_interface::pack_ptr_and_len; + extern "C" { #[allow(dead_code)] fn missing_external(); @@ -337,3 +339,15 @@ sp_core::wasm_export_functions! { return 1234; } } + +// Returns a huge len. It should result in an error, and not an allocation. +#[no_mangle] +pub extern "C" fn test_return_huge_len(_params: *const u8, _len: usize) -> u64 { + pack_ptr_and_len(0, u32::MAX) +} + +// Returns an output that overflows the u32 range. It should result in an error. +#[no_mangle] +pub extern "C" fn test_return_overflow(_params: *const u8, _len: usize) -> u64 { + pack_ptr_and_len(u32::MAX, 1) +} diff --git a/client/executor/src/integration_tests/mod.rs b/client/executor/src/integration_tests/mod.rs index 25b999f115363..1d6ccf455ee9f 100644 --- a/client/executor/src/integration_tests/mod.rs +++ b/client/executor/src/integration_tests/mod.rs @@ -20,7 +20,7 @@ mod linux; use codec::{Decode, Encode}; -use sc_executor_common::{error::Error, runtime_blob::RuntimeBlob, wasm_runtime::WasmModule}; +use sc_executor_common::{error::{Error, WasmError}, runtime_blob::RuntimeBlob, wasm_runtime::WasmModule}; use sc_runtime_test::wasm_binary_unwrap; use sp_core::{ blake2_128, blake2_256, ed25519, map, @@ -33,6 +33,7 @@ use sp_runtime::traits::BlakeTwo256; use sp_state_machine::TestExternalities as CoreTestExternalities; use sp_trie::{LayoutV1 as Layout, TrieConfiguration}; use std::sync::Arc; +use assert_matches::assert_matches; use tracing_subscriber::layer::SubscriberExt; use crate::WasmExecutionMethod; @@ -781,3 +782,37 @@ fn return_value(wasm_method: WasmExecutionMethod) { (1234u64).encode() ); } + +test_wasm_execution!(return_huge_len); +fn return_huge_len(wasm_method: WasmExecutionMethod) { + let mut ext = TestExternalities::default(); + let mut ext = ext.ext(); + + match call_in_wasm("test_return_huge_len", &[], wasm_method, &mut ext).unwrap_err() { + Error::Runtime => { + assert_matches!(wasm_method, WasmExecutionMethod::Interpreted); + } + Error::RuntimeConstruction(WasmError::Other(error)) => { + assert_matches!(wasm_method, WasmExecutionMethod::Compiled{..}); + assert_eq!(error, "output exceeds bounds of wasm memory"); + }, + error => panic!("unexpected error: {:?}", error), + } +} + +test_wasm_execution!(return_overflow); +fn return_overflow(wasm_method: WasmExecutionMethod) { + let mut ext = TestExternalities::default(); + let mut ext = ext.ext(); + + match call_in_wasm("test_return_overflow", &[], wasm_method, &mut ext).unwrap_err() { + Error::Runtime => { + assert_matches!(wasm_method, WasmExecutionMethod::Interpreted); + } + Error::RuntimeConstruction(WasmError::Other(error)) => { + assert_matches!(wasm_method, WasmExecutionMethod::Compiled{..}); + assert_eq!(error, "output exceeds bounds of wasm memory"); + }, + error => panic!("unexpected error: {:?}", error), + } +} diff --git a/client/executor/wasmtime/src/runtime.rs b/client/executor/wasmtime/src/runtime.rs index b124fd627dc69..3280ee5ee2fea 100644 --- a/client/executor/wasmtime/src/runtime.rs +++ b/client/executor/wasmtime/src/runtime.rs @@ -30,6 +30,7 @@ use sc_executor_common::{ runtime_blob::{ self, DataSegmentsSnapshot, ExposedMutableGlobalsSet, GlobalsSnapshot, RuntimeBlob, }, + util::checked_range, wasm_runtime::{InvokeMethod, WasmInstance, WasmModule}, }; use sp_runtime_interface::unpack_ptr_and_len; @@ -41,7 +42,7 @@ use std::{ Arc, }, }; -use wasmtime::{Engine, Memory, StoreLimits, Table}; +use wasmtime::{Engine, Memory, StoreLimits, Table, AsContext}; pub(crate) struct StoreData { /// The limits we apply to the store. We need to store it here to return a reference to this @@ -793,7 +794,19 @@ fn extract_output_data( output_ptr: u32, output_len: u32, ) -> Result> { + let ctx = instance.store(); + + // Do a length check before allocating. The returned output should not be bigger than the + // available WASM memory. Otherwise, a malicious parachain can trigger a large allocation, + // potentially causing memory exhaustion. + // + // Get the size of the WASM memory in bytes. + let memory_size = ctx.as_context().data().memory().data_size(ctx); + if let None = checked_range(output_ptr as usize, output_len as usize, memory_size) { + return Err(WasmError::Other("output exceeds bounds of wasm memory".into()))? + } let mut output = vec![0; output_len as usize]; - util::read_memory_into(instance.store(), Pointer::new(output_ptr), &mut output)?; + + util::read_memory_into(ctx, Pointer::new(output_ptr), &mut output)?; Ok(output) } From 1df1de4cacfbc71479b31a117ce2aeca892684c3 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Fri, 20 Jan 2023 10:01:14 +0100 Subject: [PATCH 2/4] Address review comments; add more tests --- client/executor/runtime-test/src/lib.rs | 31 ++++++++++++- client/executor/src/integration_tests/mod.rs | 46 +++++++++++++++++--- client/executor/wasmtime/src/runtime.rs | 4 +- 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/client/executor/runtime-test/src/lib.rs b/client/executor/runtime-test/src/lib.rs index ef9f46074ce74..c3a37ed57fb1f 100644 --- a/client/executor/runtime-test/src/lib.rs +++ b/client/executor/runtime-test/src/lib.rs @@ -29,7 +29,7 @@ use sp_runtime::{ print, traits::{BlakeTwo256, Hash}, }; - +#[cfg(not(feature = "std"))] use sp_runtime_interface::pack_ptr_and_len; extern "C" { @@ -40,6 +40,10 @@ extern "C" { fn yet_another_missing_external(); } +#[cfg(not(feature = "std"))] +/// The size of a WASM page in bytes. +const WASM_PAGE_SIZE: usize = 65536; + #[cfg(not(feature = "std"))] /// Mutable static variables should be always observed to have /// the initialized value at the start of a runtime call. @@ -94,7 +98,7 @@ sp_core::wasm_export_functions! { let heap_ptr = heap_base as usize; // Find the next wasm page boundary. - let heap_ptr = round_up_to(heap_ptr, 65536); + let heap_ptr = round_up_to(heap_ptr, WASM_PAGE_SIZE); // Make it an actual pointer let heap_ptr = heap_ptr as *mut u8; @@ -342,12 +346,35 @@ sp_core::wasm_export_functions! { // Returns a huge len. It should result in an error, and not an allocation. #[no_mangle] +#[cfg(not(feature = "std"))] pub extern "C" fn test_return_huge_len(_params: *const u8, _len: usize) -> u64 { pack_ptr_and_len(0, u32::MAX) } +// Returns an offset right at the edge of the wasm memory boundary. With length 0, it should +// succeed. +#[no_mangle] +#[cfg(not(feature = "std"))] +pub extern "C" fn test_return_max_memory_offset(_params: *const u8, _len: usize) -> u64 { + // This should use `core::arch::wasm` instead of `core::arch::wasm32`, + // but `core::arch::wasm` depends on `#![feature(simd_wasm64)]` on current nightly. + // See https://github.com/Craig-Macomber/lol_alloc/issues/1 + pack_ptr_and_len((core::arch::wasm32::memory_size(0) * WASM_PAGE_SIZE) as u32, 0) +} + +// Returns an offset right at the edge of the wasm memory boundary. With length 1, it should fail. +#[no_mangle] +#[cfg(not(feature = "std"))] +pub extern "C" fn test_return_max_memory_offset_plus_one(_params: *const u8, _len: usize) -> u64 { + // This should use `core::arch::wasm` instead of `core::arch::wasm32`, + // but `core::arch::wasm` depends on `#![feature(simd_wasm64)]` on current nightly. + // See https://github.com/Craig-Macomber/lol_alloc/issues/1 + pack_ptr_and_len((core::arch::wasm32::memory_size(0) * WASM_PAGE_SIZE) as u32, 1) +} + // Returns an output that overflows the u32 range. It should result in an error. #[no_mangle] +#[cfg(not(feature = "std"))] pub extern "C" fn test_return_overflow(_params: *const u8, _len: usize) -> u64 { pack_ptr_and_len(u32::MAX, 1) } diff --git a/client/executor/src/integration_tests/mod.rs b/client/executor/src/integration_tests/mod.rs index 1d6ccf455ee9f..22e651abe98e2 100644 --- a/client/executor/src/integration_tests/mod.rs +++ b/client/executor/src/integration_tests/mod.rs @@ -19,8 +19,13 @@ #[cfg(target_os = "linux")] mod linux; +use assert_matches::assert_matches; use codec::{Decode, Encode}; -use sc_executor_common::{error::{Error, WasmError}, runtime_blob::RuntimeBlob, wasm_runtime::WasmModule}; +use sc_executor_common::{ + error::{Error, WasmError}, + runtime_blob::RuntimeBlob, + wasm_runtime::WasmModule, +}; use sc_runtime_test::wasm_binary_unwrap; use sp_core::{ blake2_128, blake2_256, ed25519, map, @@ -33,7 +38,6 @@ use sp_runtime::traits::BlakeTwo256; use sp_state_machine::TestExternalities as CoreTestExternalities; use sp_trie::{LayoutV1 as Layout, TrieConfiguration}; use std::sync::Arc; -use assert_matches::assert_matches; use tracing_subscriber::layer::SubscriberExt; use crate::WasmExecutionMethod; @@ -791,9 +795,39 @@ fn return_huge_len(wasm_method: WasmExecutionMethod) { match call_in_wasm("test_return_huge_len", &[], wasm_method, &mut ext).unwrap_err() { Error::Runtime => { assert_matches!(wasm_method, WasmExecutionMethod::Interpreted); - } + }, Error::RuntimeConstruction(WasmError::Other(error)) => { - assert_matches!(wasm_method, WasmExecutionMethod::Compiled{..}); + assert_matches!(wasm_method, WasmExecutionMethod::Compiled { .. }); + assert_eq!(error, "output exceeds bounds of wasm memory"); + }, + error => panic!("unexpected error: {:?}", error), + } +} + +test_wasm_execution!(return_max_memory_offset); +fn return_max_memory_offset(wasm_method: WasmExecutionMethod) { + let mut ext = TestExternalities::default(); + let mut ext = ext.ext(); + + assert_eq!( + call_in_wasm("test_return_max_memory_offset", &[], wasm_method, &mut ext).unwrap(), + ().encode() + ); +} + +test_wasm_execution!(return_max_memory_offset_plus_one); +fn return_max_memory_offset_plus_one(wasm_method: WasmExecutionMethod) { + let mut ext = TestExternalities::default(); + let mut ext = ext.ext(); + + match call_in_wasm("test_return_max_memory_offset_plus_one", &[], wasm_method, &mut ext) + .unwrap_err() + { + Error::Runtime => { + assert_matches!(wasm_method, WasmExecutionMethod::Interpreted); + }, + Error::RuntimeConstruction(WasmError::Other(error)) => { + assert_matches!(wasm_method, WasmExecutionMethod::Compiled { .. }); assert_eq!(error, "output exceeds bounds of wasm memory"); }, error => panic!("unexpected error: {:?}", error), @@ -808,9 +842,9 @@ fn return_overflow(wasm_method: WasmExecutionMethod) { match call_in_wasm("test_return_overflow", &[], wasm_method, &mut ext).unwrap_err() { Error::Runtime => { assert_matches!(wasm_method, WasmExecutionMethod::Interpreted); - } + }, Error::RuntimeConstruction(WasmError::Other(error)) => { - assert_matches!(wasm_method, WasmExecutionMethod::Compiled{..}); + assert_matches!(wasm_method, WasmExecutionMethod::Compiled { .. }); assert_eq!(error, "output exceeds bounds of wasm memory"); }, error => panic!("unexpected error: {:?}", error), diff --git a/client/executor/wasmtime/src/runtime.rs b/client/executor/wasmtime/src/runtime.rs index 3280ee5ee2fea..be3efc412578f 100644 --- a/client/executor/wasmtime/src/runtime.rs +++ b/client/executor/wasmtime/src/runtime.rs @@ -42,7 +42,7 @@ use std::{ Arc, }, }; -use wasmtime::{Engine, Memory, StoreLimits, Table, AsContext}; +use wasmtime::{AsContext, Engine, Memory, StoreLimits, Table}; pub(crate) struct StoreData { /// The limits we apply to the store. We need to store it here to return a reference to this @@ -802,7 +802,7 @@ fn extract_output_data( // // Get the size of the WASM memory in bytes. let memory_size = ctx.as_context().data().memory().data_size(ctx); - if let None = checked_range(output_ptr as usize, output_len as usize, memory_size) { + if checked_range(output_ptr as usize, output_len as usize, memory_size).is_none() { return Err(WasmError::Other("output exceeds bounds of wasm memory".into()))? } let mut output = vec![0; output_len as usize]; From 1e3a67737aeda04a5515de4f068e3b92cb3dd4ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 20 Jan 2023 10:08:34 +0100 Subject: [PATCH 3/4] Update client/executor/wasmtime/src/runtime.rs --- client/executor/wasmtime/src/runtime.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/executor/wasmtime/src/runtime.rs b/client/executor/wasmtime/src/runtime.rs index be3efc412578f..a45dfa59b954a 100644 --- a/client/executor/wasmtime/src/runtime.rs +++ b/client/executor/wasmtime/src/runtime.rs @@ -803,7 +803,7 @@ fn extract_output_data( // Get the size of the WASM memory in bytes. let memory_size = ctx.as_context().data().memory().data_size(ctx); if checked_range(output_ptr as usize, output_len as usize, memory_size).is_none() { - return Err(WasmError::Other("output exceeds bounds of wasm memory".into()))? + Err(WasmError::Other("output exceeds bounds of wasm memory".into()))? } let mut output = vec![0; output_len as usize]; From b55dcf71583d590f6c55c58be0efbbc83a4aee1d Mon Sep 17 00:00:00 2001 From: Marcin S Date: Fri, 20 Jan 2023 10:16:03 +0100 Subject: [PATCH 4/4] Remove unnecessary comments --- client/executor/runtime-test/src/lib.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/client/executor/runtime-test/src/lib.rs b/client/executor/runtime-test/src/lib.rs index c3a37ed57fb1f..0aa30e4bc9675 100644 --- a/client/executor/runtime-test/src/lib.rs +++ b/client/executor/runtime-test/src/lib.rs @@ -356,9 +356,6 @@ pub extern "C" fn test_return_huge_len(_params: *const u8, _len: usize) -> u64 { #[no_mangle] #[cfg(not(feature = "std"))] pub extern "C" fn test_return_max_memory_offset(_params: *const u8, _len: usize) -> u64 { - // This should use `core::arch::wasm` instead of `core::arch::wasm32`, - // but `core::arch::wasm` depends on `#![feature(simd_wasm64)]` on current nightly. - // See https://github.com/Craig-Macomber/lol_alloc/issues/1 pack_ptr_and_len((core::arch::wasm32::memory_size(0) * WASM_PAGE_SIZE) as u32, 0) } @@ -366,9 +363,6 @@ pub extern "C" fn test_return_max_memory_offset(_params: *const u8, _len: usize) #[no_mangle] #[cfg(not(feature = "std"))] pub extern "C" fn test_return_max_memory_offset_plus_one(_params: *const u8, _len: usize) -> u64 { - // This should use `core::arch::wasm` instead of `core::arch::wasm32`, - // but `core::arch::wasm` depends on `#![feature(simd_wasm64)]` on current nightly. - // See https://github.com/Craig-Macomber/lol_alloc/issues/1 pack_ptr_and_len((core::arch::wasm32::memory_size(0) * WASM_PAGE_SIZE) as u32, 1) }