XCM: Deny barrier checks for nested XCMs with specific instructions to be executed on the local chain

polkadot/xcm/xcm-builder/Cargo.toml

@@ -13,6 +13,7 @@ workspace = true
 
 [dependencies]
 codec = { features = ["derive"], workspace = true }
+environmental = { workspace = true }
 frame-support = { workspace = true }
 frame-system = { workspace = true }
 impl-trait-for-tuples = { workspace = true }
@@ -21,6 +22,7 @@ pallet-asset-conversion = { workspace = true }
 pallet-transaction-payment = { workspace = true }
 scale-info = { features = ["derive"], workspace = true }
 sp-arithmetic = { workspace = true }
+sp-core = { workspace = true }
 sp-io = { workspace = true }
 sp-runtime = { workspace = true }
 sp-weights = { workspace = true }
@@ -39,7 +41,6 @@ polkadot-primitives = { workspace = true, default-features = true }
 polkadot-runtime-parachains = { workspace = true, default-features = true }
 polkadot-test-runtime = { workspace = true }
 primitive-types = { features = ["codec", "num-traits", "scale-info"], workspace = true }
-sp-core = { workspace = true, default-features = true }
 
 [features]
 default = ["std"]
@@ -62,6 +63,7 @@ runtime-benchmarks = [
 ]
 std = [
 	"codec/std",
+	"environmental/std",
 	"frame-support/std",
 	"frame-system/std",
 	"log/std",
@@ -71,6 +73,7 @@ std = [
 	"primitive-types/std",
 	"scale-info/std",
 	"sp-arithmetic/std",
+	"sp-core/std",
 	"sp-io/std",
 	"sp-runtime/std",
 	"sp-weights/std",

polkadot/xcm/xcm-builder/src/barriers.rs

@@ -502,3 +502,72 @@ impl DenyExecution for DenyReserveTransferToRelayChain {
 		Ok(())
 	}
 }
+
+environmental::environmental!(recursion_count: u8);
+
+// TBD:
+/// Applies the `Inner` filter to the nested XCM for the `SetAppendix`, `SetErrorHandler`, and
+/// `ExecuteWithOrigin` instructions.
+///
+/// Note: The nested XCM is checked recursively!
+pub struct DenyInstructionsWithXcm<Inner>(PhantomData<Inner>);
+impl<Inner: ShouldExecute> ShouldExecute for DenyInstructionsWithXcm<Inner> {
+	fn should_execute<RuntimeCall>(
+		origin: &Location,
+		message: &mut [Instruction<RuntimeCall>],
+		max_weight: Weight,
+		properties: &mut Properties,
+	) -> Result<(), ProcessMessageError> {
+		message.matcher().match_next_inst_while(
+			|_| true,
+			|inst| match inst {
+				SetAppendix(nested_xcm) |
+				SetErrorHandler(nested_xcm) |
+				ExecuteWithOrigin { xcm: nested_xcm, .. } => {
+					// check xcm instructions with `Inner` filter
+					let _ = Inner::should_execute(origin, nested_xcm.inner_mut(), max_weight, properties)
+						.inspect_err(|e| {
+							log::warn!(
+								target: "xcm::barriers",
+								"`DenyInstructionsWithXcm`'s `Inner::should_execute` did not pass for origin: {:?} and nested_xcm: {:?} with error: {:?}",
+								origin,
+								nested_xcm,
+								e
+							);
+						})?;
+
+					// Initialize the recursion count only the first time we hit this code in our
+					// potential recursive execution.
+					let _ = recursion_count::using_once(&mut 1, || {
+						recursion_count::with(|count| {
+							if *count > xcm_executor::RECURSION_LIMIT {
+								return Err(ProcessMessageError::StackLimitReached)
+							}
+							*count = count.saturating_add(1);
+							Ok(())
+						})
+							// This should always return `Some`, but let's play it safe.
+							.unwrap_or(Ok(()))?;
+
+						// Ensure that we always decrement the counter whenever we finish processing
+						// the `DenyInstructionsWithXcm`.
+						sp_core::defer! {
+							recursion_count::with(|count| {
+								*count = count.saturating_sub(1);
+							});
+						}
+
+						// check recursively with `DenyInstructionsWithXcm`
+						Self::should_execute(origin, nested_xcm.inner_mut(), max_weight, properties)
+					})?;
+
+					Ok(ControlFlow::Continue(()))
+				},
+				_ => Ok(ControlFlow::Continue(())),
+			},
+		)?;
+
+		// Permit everything else
+		Ok(())
+	}
+}

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

@@ -42,9 +42,9 @@ mod barriers;
 pub use barriers::{
 	AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain,
 	AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom,
-	AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain,
-	IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit,
-	TrailingSetTopicAsId, WithComputedOrigin,
+	AllowUnpaidExecutionFrom, DenyInstructionsWithXcm, DenyReserveTransferToRelayChain,
+	DenyThenTry, IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain,
+	RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin,
 };
 
 mod controller;

polkadot/xcm/xcm-builder/src/tests/barriers.rs

@@ -764,3 +764,80 @@ fn deny_reserve_transfer_to_relaychain_should_work() {
 	// others instructions should pass
 	assert_deny_execution(vec![ClearOrigin], Here.into_location(), Ok(()));
 }
+
+#[test]
+fn deny_instructions_with_xcm_works() {
+	frame_support::__private::sp_tracing::try_init_simple();
+
+	// dummy filter which denies `ClearOrigin`
+	struct DenyClearOrigin;
+	impl ShouldExecute for DenyClearOrigin {
+		fn should_execute<RuntimeCall>(
+			_: &Location,
+			message: &mut [Instruction<RuntimeCall>],
+			_: Weight,
+			_: &mut Properties,
+		) -> Result<(), ProcessMessageError> {
+			message.matcher().match_next_inst_while(
+				|_| true,
+				|inst| match inst {
+					ClearOrigin => Err(ProcessMessageError::Unsupported),
+					_ => Ok(ControlFlow::Continue(())),
+				},
+			)?;
+			Ok(())
+		}
+	}
+
+	// closure for (xcm, origin) testing with `DenyInstructionsWithXcm` which denies `ClearOrigin`
+	// instruction
+	let assert_should_execute = |mut xcm: Vec<Instruction<()>>, origin, expected_result| {
+		assert_eq!(
+			DenyInstructionsWithXcm::<DenyClearOrigin>::should_execute(
+				&origin,
+				&mut xcm,
+				Weight::from_parts(10, 10),
+				&mut props(Weight::zero()),
+			),
+			expected_result
+		);
+	};
+
+	// ok
+	assert_should_execute(vec![ClearTransactStatus], Location::parent(), Ok(()));
+	// ok top-level contains `ClearOrigin`
+	assert_should_execute(vec![ClearOrigin], Location::parent(), Ok(()));
+	// ok - SetAppendix with XCM without ClearOrigin
+	assert_should_execute(
+		vec![SetAppendix(Xcm(vec![ClearTransactStatus]))],
+		Location::parent(),
+		Ok(()),
+	);
+
+	// invalid - empty XCM
+	assert_should_execute(vec![], Location::parent(), Err(ProcessMessageError::BadFormat));
+	// invalid - SetAppendix with empty XCM
+	assert_should_execute(
+		vec![SetAppendix(Xcm(vec![]))],
+		Location::parent(),
+		Err(ProcessMessageError::BadFormat),
+	);
+	// invalid SetAppendix contains `ClearOrigin`
+	assert_should_execute(
+		vec![SetAppendix(Xcm(vec![ClearOrigin]))],
+		Location::parent(),
+		Err(ProcessMessageError::Unsupported),
+	);
+	// invalid nested SetAppendix contains `ClearOrigin`
+	assert_should_execute(
+		vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(
+			Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![
+				SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![
+					SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![ClearOrigin]))])),
+				]))]))])),
+			]))]))]))]),
+		)]))]))]))],
+		Location::parent(),
+		Err(ProcessMessageError::StackLimitReached),
+	);
+}

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

@@ -60,7 +60,7 @@ pub struct FeesMode {
 	pub jit_withdraw: bool,
 }
 
-const RECURSION_LIMIT: u8 = 10;
+pub const RECURSION_LIMIT: u8 = 10;
 
 environmental::environmental!(recursion_count: u8);