PoC: DenyInstructionsWithXcm for validating nested XCM instructions

Author: bkontur

Cargo.lock

@@ -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 }
@@ -40,7 +42,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"]
@@ -63,6 +64,7 @@ runtime-benchmarks = [
 ]
 std = [
 	"codec/std",
+	"environmental/std",
 	"frame-support/std",
 	"frame-system/std",
 	"log/std",
@@ -72,6 +74,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/Cargo.toml

@@ -490,7 +490,7 @@ impl ShouldExecute for DenyReserveTransferToRelayChain {
 					if matches!(origin, Location { parents: 1, interior: Here }) =>
 				{
 					log::warn!(
-						target: "xcm::barrier",
+						target: "xcm::barriers",
 						"Unexpected ReserveAssetDeposited from the Relay Chain",
 					);
 					Ok(ControlFlow::Continue(()))
@@ -504,3 +504,71 @@ impl ShouldExecute 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/barriers.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,
 };
 

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

@@ -14,6 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
+use core::ops::ControlFlow;
 use xcm_executor::traits::Properties;
 
 use super::*;
@@ -532,3 +533,103 @@ fn allow_hrmp_notifications_from_relay_chain_should_work() {
 		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-builder/src/tests/barriers.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);