Harden XCM v1 for Recursions (#3586)

* Guard against XCM recursive bombs by setting a recursion limit

* Add test and set a lower recursion limit

* Use u32 instead of usize for recursion limit

* Make spellcheck happy

* Cargo fmt

* Limit XCM decoding depth in UMP message processing

* Modify test to check for recursion in BuyExecution

* Update xcm/xcm-simulator/example/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Make cargo fmt happy

* WIP for testing recursion limit in WASM

* Revert "WIP for testing recursion limit in WASM"

This reverts commit 39181b46d1adf79358f5ae8aafcf480e0c0c22e6.

* Remove XCM recursion limit test

* Add recursion test for XCM message execution

* Set a more sensible recursion limit

* Cargo fmt

* Implement successful_origin for benchmarks

* Set recursion limit to 8 and create integration tests directory for xcm-executor

* Cargo fmt

* Add runtime-benchmarks feature to test-runtime

* Give up creating ConvertOriginToLocal and use EnsureXcm

* Re-add ConvertOriginToLocal

* Fix compilation

* Update xcm/xcm-executor/src/lib.rs

Co-authored-by: Gavin Wood <gavin@parity.io>

* Add decoding limit to all versioned XCM decode calls

* Fix recursion limit test

* Set a lower recursion count for recursion test

* move integration tests to their own folder, fix recursion check in execute_effects

* Remove xcm-executor integration tests directory

* fix up

* Update Cargo.lock

* Update runtime/parachains/src/ump.rs

* use proper decode limit

* fix decode depth limit

* here too

* Update traits.rs

* fix compile

* fix test

* Revert `decode_all_with_depth_limit` changes in parachain.rs

* Remove unused imports in parachain.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Bastian Köcher <info@kchr.de>
Co-authored-by: Gavin Wood <gavin@parity.io>

polkadot/xcm/src/double_encoded.rs

@@ -14,12 +14,10 @@
 // You should have received a copy of the GNU General Public License
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
+use crate::MAX_XCM_DECODE_DEPTH;
 use alloc::vec::Vec;
 use parity_scale_codec::{Decode, DecodeLimit, Encode};
 
-/// Maximum nesting level for XCM decoding.
-pub const MAX_XCM_DECODE_DEPTH: u32 = 8;
-
 /// Wrapper around the encoded and decoded versions of a value.
 /// Caches the decoded value once computed.
 #[derive(Encode, Decode)]

polkadot/xcm/src/lib.rs

@@ -40,6 +40,9 @@ pub mod latest {
 mod double_encoded;
 pub use double_encoded::DoubleEncoded;
 
+/// Maximum nesting level for XCM decoding.
+pub const MAX_XCM_DECODE_DEPTH: u32 = 8;
+
 #[derive(Clone, Eq, PartialEq, Debug)]
 pub enum Unsupported {}
 impl Encode for Unsupported {}

polkadot/xcm/src/v0/traits.rs

@@ -87,6 +87,8 @@ pub enum Error {
 	TooExpensive,
 	/// The given asset is not handled.
 	AssetNotFound,
+	/// `execute_xcm` has been called too many times recursively.
+	RecursionLimitReached,
 }
 
 impl From<()> for Error {

polkadot/xcm/src/v1/traits.rs

@@ -89,6 +89,8 @@ pub enum Error {
 	AssetNotFound,
 	/// The given message cannot be translated into a format that the destination can be expected to interpret.
 	DestinationUnsupported,
+	/// `execute_xcm` has been called too many times recursively.
+	RecursionLimitReached,
 }
 
 impl From<()> for Error {

polkadot/xcm/xcm-executor/integration-tests/Cargo.toml

@@ -0,0 +1,30 @@
+[package]
+authors = ["Parity Technologies <admin@parity.io>"]
+edition = "2018"
+name = "xcm-executor-integration-tests"
+description = "Integration tests for the XCM Executor"
+version = "0.9.9"
+
+[dependencies]
+frame-support = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
+frame-system = { git = "https://github.com/paritytech/substrate", branch = "master" }
+futures = "0.3.15"
+pallet-xcm = { path = "../../pallet-xcm" }
+polkadot-test-client = { path = "../../../node/test/client" }
+polkadot-test-runtime = { path = "../../../runtime/test-runtime" }
+polkadot-test-service = { path = "../../../node/test/service" }
+sp-consensus = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sp-keyring = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sp-runtime = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
+sp-state-machine = { git = "https://github.com/paritytech/substrate", branch = "master" }
+xcm = { path = "../..", default-features = false }
+xcm-executor = { path = ".." }
+sp-tracing = { git = "https://github.com/paritytech/substrate", branch = "master" }
+
+[features]
+default = ["std"]
+std = [
+	"xcm/std",
+	"sp-runtime/std",
+	"frame-support/std",
+]

polkadot/xcm/xcm-executor/integration-tests/src/lib.rs

@@ -0,0 +1,141 @@
+// Copyright 2021 Parity Technologies (UK) Ltd.
+// This file is part of Polkadot.
+
+// Polkadot is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Polkadot is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
+
+#![cfg_attr(not(feature = "std"), no_std)]
+#![cfg(test)]
+
+use polkadot_test_client::{
+	BlockBuilderExt, ClientBlockImportExt, DefaultTestClientBuilderExt, ExecutionStrategy,
+	InitPolkadotBlockBuilder, TestClientBuilder, TestClientBuilderExt,
+};
+use polkadot_test_service::construct_extrinsic;
+use sp_runtime::{generic::BlockId, traits::Block};
+use sp_state_machine::InspectState;
+use xcm::latest::{Error as XcmError, Junction::*, MultiLocation::*, Order, Outcome, Xcm::*};
+use xcm_executor::MAX_RECURSION_LIMIT;
+
+// This is the inflection point where the test should either fail or pass.
+const MAX_RECURSION_CHECK: u32 = MAX_RECURSION_LIMIT / 2;
+
+#[test]
+fn execute_within_recursion_limit() {
+	sp_tracing::try_init_simple();
+	let mut client = TestClientBuilder::new()
+		.set_execution_strategy(ExecutionStrategy::AlwaysWasm)
+		.build();
+
+	let mut msg = WithdrawAsset { assets: (X1(Parent), 100).into(), effects: vec![] };
+	for _ in 0..MAX_RECURSION_CHECK {
+		msg = WithdrawAsset {
+			assets: (X1(Parent), 100).into(),
+			effects: vec![Order::BuyExecution {
+				fees: (X1(Parent), 1).into(),
+				weight: 0,
+				debt: 0,
+				halt_on_error: true,
+				orders: vec![],
+				// nest `msg` into itself on each iteration.
+				instructions: vec![msg],
+			}],
+		};
+	}
+
+	let mut block_builder = client.init_polkadot_block_builder();
+
+	let execute = construct_extrinsic(
+		&client,
+		polkadot_test_runtime::Call::Xcm(pallet_xcm::Call::execute(
+			Box::new(msg.clone()),
+			1_000_000_000,
+		)),
+		sp_keyring::Sr25519Keyring::Alice,
+	);
+
+	block_builder.push_polkadot_extrinsic(execute).expect("pushes extrinsic");
+
+	let block = block_builder.build().expect("Finalizes the block").block;
+	let block_hash = block.hash();
+
+	futures::executor::block_on(client.import(sp_consensus::BlockOrigin::Own, block))
+		.expect("imports the block");
+
+	client
+		.state_at(&BlockId::Hash(block_hash))
+		.expect("state should exist")
+		.inspect_state(|| {
+			assert!(polkadot_test_runtime::System::events().iter().any(|r| matches!(
+				r.event,
+				polkadot_test_runtime::Event::Xcm(pallet_xcm::Event::Attempted(Outcome::Complete(
+					_
+				)),),
+			)));
+		});
+}
+
+#[test]
+fn exceed_recursion_limit() {
+	sp_tracing::try_init_simple();
+	let mut client = TestClientBuilder::new()
+		.set_execution_strategy(ExecutionStrategy::AlwaysWasm)
+		.build();
+
+	let mut msg = WithdrawAsset { assets: (X1(Parent), 100).into(), effects: vec![] };
+	for _ in 0..(MAX_RECURSION_CHECK + 1) {
+		msg = WithdrawAsset {
+			assets: (X1(Parent), 100).into(),
+			effects: vec![Order::BuyExecution {
+				fees: (X1(Parent), 1).into(),
+				weight: 0,
+				debt: 0,
+				halt_on_error: true,
+				orders: vec![],
+				// nest `msg` into itself on each iteration.
+				instructions: vec![msg],
+			}],
+		};
+	}
+
+	let mut block_builder = client.init_polkadot_block_builder();
+
+	let execute = construct_extrinsic(
+		&client,
+		polkadot_test_runtime::Call::Xcm(pallet_xcm::Call::execute(
+			Box::new(msg.clone()),
+			1_000_000_000,
+		)),
+		sp_keyring::Sr25519Keyring::Alice,
+	);
+
+	block_builder.push_polkadot_extrinsic(execute).expect("pushes extrinsic");
+
+	let block = block_builder.build().expect("Finalizes the block").block;
+	let block_hash = block.hash();
+
+	futures::executor::block_on(client.import(sp_consensus::BlockOrigin::Own, block))
+		.expect("imports the block");
+
+	client
+		.state_at(&BlockId::Hash(block_hash))
+		.expect("state should exist")
+		.inspect_state(|| {
+			assert!(polkadot_test_runtime::System::events().iter().any(|r| matches!(
+				r.event,
+				polkadot_test_runtime::Event::Xcm(pallet_xcm::Event::Attempted(
+					Outcome::Incomplete(_, XcmError::RecursionLimitReached),
+				)),
+			)));
+		});
+}

polkadot/xcm/xcm-executor/src/lib.rs

@@ -41,6 +41,9 @@ pub use config::Config;
 /// The XCM executor.
 pub struct XcmExecutor<Config>(PhantomData<Config>);
 
+/// The maximum recursion limit for `execute_xcm` and `execute_effects`.
+pub const MAX_RECURSION_LIMIT: u32 = 8;
+
 impl<Config: config::Config> ExecuteXcm<Config::Call> for XcmExecutor<Config> {
 	fn execute_xcm_in_credit(
 		origin: MultiLocation,
@@ -56,7 +59,6 @@ impl<Config: config::Config> ExecuteXcm<Config::Call> for XcmExecutor<Config> {
 			weight_limit,
 			weight_credit,
 		);
-		// TODO: #2841 #HARDENXCM We should identify recursive bombs here and bail.
 		let mut message = Xcm::<Config::Call>::from(message);
 		let shallow_weight = match Config::Weigher::shallow(&mut message) {
 			Ok(x) => x,
@@ -81,6 +83,7 @@ impl<Config: config::Config> ExecuteXcm<Config::Call> for XcmExecutor<Config> {
 			&mut weight_credit,
 			Some(shallow_weight),
 			&mut trader,
+			0,
 		);
 		drop(trader);
 		log::trace!(target: "xcm::execute_xcm", "result: {:?}", &result);
@@ -110,16 +113,23 @@ impl<Config: config::Config> XcmExecutor<Config> {
 		weight_credit: &mut Weight,
 		maybe_shallow_weight: Option<Weight>,
 		trader: &mut Config::Trader,
+		num_recursions: u32,
 	) -> Result<Weight, XcmError> {
 		log::trace!(
 			target: "xcm::do_execute_xcm",
-			"origin: {:?}, top_level: {:?}, message: {:?}, weight_credit: {:?}, maybe_shallow_weight: {:?}",
+			"origin: {:?}, top_level: {:?}, message: {:?}, weight_credit: {:?}, maybe_shallow_weight: {:?}, recursion: {:?}",
 			origin,
 			top_level,
 			message,
 			weight_credit,
 			maybe_shallow_weight,
+			num_recursions,
 		);
+
+		if num_recursions > MAX_RECURSION_LIMIT {
+			return Err(XcmError::RecursionLimitReached)
+		}
+
 		// This is the weight of everything that cannot be paid for. This basically means all computation
 		// except any XCM which is behind an Order::BuyExecution.
 		let shallow_weight = maybe_shallow_weight
@@ -237,8 +247,15 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				ensure!(who.is_interior(), XcmError::EscalationOfPrivilege);
 				let mut origin = origin;
 				origin.append_with(who).map_err(|_| XcmError::MultiLocationFull)?;
-				let surplus =
-					Self::do_execute_xcm(origin, top_level, *message, weight_credit, None, trader)?;
+				let surplus = Self::do_execute_xcm(
+					origin,
+					top_level,
+					*message,
+					weight_credit,
+					None,
+					trader,
+					num_recursions + 1,
+				)?;
 				total_surplus = total_surplus.saturating_add(surplus);
 				None
 			},
@@ -247,7 +264,13 @@ impl<Config: config::Config> XcmExecutor<Config> {
 
 		if let Some((mut holding, effects)) = maybe_holding_effects {
 			for effect in effects.into_iter() {
-				total_surplus += Self::execute_orders(&origin, &mut holding, effect, trader)?;
+				total_surplus += Self::execute_orders(
+					&origin,
+					&mut holding,
+					effect,
+					trader,
+					num_recursions + 1,
+				)?;
 			}
 		}
 
@@ -259,14 +282,21 @@ impl<Config: config::Config> XcmExecutor<Config> {
 		holding: &mut Assets,
 		order: Order<Config::Call>,
 		trader: &mut Config::Trader,
+		num_recursions: u32,
 	) -> Result<Weight, XcmError> {
 		log::trace!(
 			target: "xcm::execute_orders",
-			"origin: {:?}, holding: {:?}, effect: {:?}",
+			"origin: {:?}, holding: {:?}, order: {:?}, recursion: {:?}",
 			origin,
 			holding,
 			order,
+			num_recursions,
 		);
+
+		if num_recursions > MAX_RECURSION_LIMIT {
+			return Err(XcmError::RecursionLimitReached)
+		}
+
 		let mut total_surplus = 0;
 		match order {
 			Order::DepositAsset { assets, max_assets, beneficiary } => {
@@ -314,7 +344,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
 
 				let mut remaining_weight = weight;
 				for order in orders.into_iter() {
-					match Self::execute_orders(origin, holding, order, trader) {
+					match Self::execute_orders(origin, holding, order, trader, num_recursions + 1) {
 						Err(e) if halt_on_error => return Err(e),
 						Err(_) => {},
 						Ok(surplus) => total_surplus += surplus,
@@ -328,6 +358,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
 						&mut remaining_weight,
 						None,
 						trader,
+						num_recursions + 1,
 					) {
 						Err(e) if halt_on_error => return Err(e),
 						Err(_) => {},

polkadot/xcm/xcm-simulator/example/Cargo.toml

@@ -24,4 +24,4 @@ xcm-builder = { path = "../../xcm-builder" }
 pallet-xcm = { path = "../../pallet-xcm" }
 polkadot-core-primitives = { path = "../../../core-primitives"}
 polkadot-runtime-parachains = { path = "../../../runtime/parachains" }
-polkadot-parachain = { path = "../../../parachain" }
+polkadot-parachain = { path = "../../../parachain" }