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

@@ -82,21 +82,22 @@ impl<T: Contains<Location>> ShouldExecute for AllowTopLevelPaidExecutionFrom<T>
 		instructions[..end]
 			.matcher()
 			.match_next_inst(|inst| match inst {
-				WithdrawAsset(ref assets) |
-				ReceiveTeleportedAsset(ref assets) |
-				ReserveAssetDeposited(ref assets) |
-				ClaimAsset { ref assets, .. } =>
+				WithdrawAsset(ref assets)
+				| ReceiveTeleportedAsset(ref assets)
+				| ReserveAssetDeposited(ref assets)
+				| ClaimAsset { ref assets, .. } => {
 					if assets.len() <= MAX_ASSETS_FOR_BUY_EXECUTION {
 						Ok(())
 					} else {
 						Err(ProcessMessageError::BadFormat)
-					},
+					}
+				},
 				_ => Err(ProcessMessageError::BadFormat),
 			})?
 			.skip_inst_while(|inst| {
-				matches!(inst, ClearOrigin | AliasOrigin(..)) ||
-					matches!(inst, DescendOrigin(child) if child != &Here) ||
-					matches!(inst, SetHints { .. })
+				matches!(inst, ClearOrigin | AliasOrigin(..))
+					|| matches!(inst, DescendOrigin(child) if child != &Here)
+					|| matches!(inst, SetHints { .. })
 			})?
 			.match_next_inst(|inst| match inst {
 				BuyExecution { weight_limit: Limited(ref mut weight), .. }
@@ -198,7 +199,7 @@ impl<InnerBarrier: ShouldExecute, LocalUniversal: Get<InteriorLocation>, MaxPref
 					},
 					DescendOrigin(j) => {
 						let Ok(_) = actual_origin.append_with(j.clone()) else {
-							return Err(ProcessMessageError::Unsupported)
+							return Err(ProcessMessageError::Unsupported);
 						};
 					},
 					_ => return Ok(ControlFlow::Break(())),
@@ -371,7 +372,9 @@ impl<ResponseHandler: OnResponse> ShouldExecute for AllowKnownQueryResponses<Res
 			.match_next_inst(|inst| match inst {
 				QueryResponse { query_id, querier, .. }
 					if ResponseHandler::expecting_response(origin, *query_id, querier.as_ref()) =>
-					Ok(()),
+				{
+					Ok(())
+				},
 				_ => Err(ProcessMessageError::BadFormat),
 			})?;
 		Ok(())
@@ -431,9 +434,9 @@ impl ShouldExecute for AllowHrmpNotificationsFromRelayChain {
 			.matcher()
 			.assert_remaining_insts(1)?
 			.match_next_inst(|inst| match inst {
-				HrmpNewChannelOpenRequest { .. } |
-				HrmpChannelAccepted { .. } |
-				HrmpChannelClosing { .. } => Ok(()),
+				HrmpNewChannelOpenRequest { .. }
+				| HrmpChannelAccepted { .. }
+				| HrmpChannelClosing { .. } => Ok(()),
 				_ => Err(ProcessMessageError::BadFormat),
 			})?;
 		Ok(())
@@ -478,9 +481,9 @@ impl DenyExecution for DenyReserveTransferToRelayChain {
 				InitiateReserveWithdraw {
 					reserve: Location { parents: 1, interior: Here },
 					..
-				} |
-				DepositReserveAsset { dest: Location { parents: 1, interior: Here }, .. } |
-				TransferReserveAsset { dest: Location { parents: 1, interior: Here }, .. } => {
+				}
+				| DepositReserveAsset { dest: Location { parents: 1, interior: Here }, .. }
+				| TransferReserveAsset { dest: Location { parents: 1, interior: Here }, .. } => {
 					Err(ProcessMessageError::Unsupported) // Deny
 				},
 
@@ -502,3 +505,71 @@ 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,8 +42,8 @@ mod barriers;
 pub use barriers::{
 	AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain,
 	AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom,
-	AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain,
-	IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit,
+	AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyInstructionsWithXcm, DenyThenTry,
+	IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit,
 	TrailingSetTopicAsId, WithComputedOrigin,
 };