Fix potential huge allocation as a result of validate_block output (#13183)

* Fix potential huge allocation as a result of `validate_block` output

* Address review comments; add more tests

* Update client/executor/wasmtime/src/runtime.rs

* Remove unnecessary comments

Co-authored-by: Bastian Köcher <git@kchr.de>

substrate/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" }

substrate/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]

substrate/client/executor/runtime-test/src/lib.rs

@@ -29,6 +29,8 @@ use sp_runtime::{
 	print,
 	traits::{BlakeTwo256, Hash},
 };
+#[cfg(not(feature = "std"))]
+use sp_runtime_interface::pack_ptr_and_len;
 
 extern "C" {
 	#[allow(dead_code)]
@@ -38,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.
@@ -92,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;
@@ -337,3 +343,32 @@ sp_core::wasm_export_functions! {
 		return 1234;
 	}
 }
+
+// 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 {
+	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 {
+	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)
+}

substrate/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, 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,
@@ -781,3 +786,67 @@ 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_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),
+	}
+}
+
+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),
+	}
+}

substrate/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::{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
@@ -793,7 +794,19 @@ fn extract_output_data(
 	output_ptr: u32,
 	output_len: u32,
 ) -> Result<Vec<u8>> {
+	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 checked_range(output_ptr as usize, output_len as usize, memory_size).is_none() {
+		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)
 }