From 9c80770f0aa89004ebe52cd3c1b824228f033a2e Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 15 Jan 2025 15:36:14 +0100 Subject: [PATCH] PoC: DenyInstructionsWithXcm for validating nested XCM instructions --- Cargo.lock | 1 + polkadot/xcm/xcm-builder/Cargo.toml | 5 +- polkadot/xcm/xcm-builder/src/barriers.rs | 70 +++++++++++- polkadot/xcm/xcm-builder/src/lib.rs | 4 +- .../xcm/xcm-builder/src/tests/barriers.rs | 101 ++++++++++++++++++ polkadot/xcm/xcm-executor/src/lib.rs | 2 +- 6 files changed, 178 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 55cc1721bdde..35a8c3a6df0e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -28387,6 +28387,7 @@ name = "staging-xcm-builder" version = "7.0.0" dependencies = [ "assert_matches", + "environmental", "frame-support 28.0.0", "frame-system 28.0.0", "impl-trait-for-tuples", diff --git a/polkadot/xcm/xcm-builder/Cargo.toml b/polkadot/xcm/xcm-builder/Cargo.toml index f75c984c068e..bd588c97207a 100644 --- a/polkadot/xcm/xcm-builder/Cargo.toml +++ b/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 } @@ -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", diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index adba9a3ef79f..62e8dfd4ac64 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -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(PhantomData); +impl ShouldExecute for DenyInstructionsWithXcm { + fn should_execute( + origin: &Location, + message: &mut [Instruction], + 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(()) + } +} diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index e23412a97ebc..ef9b0900cf93 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/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, }; diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index d8805274d3a5..c65a7bd8ccd1 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,6 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . +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(_: &Location, message: &mut [Instruction], _: 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>, origin, expected_result| { + assert_eq!( + DenyInstructionsWithXcm::::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), + ); +} diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index d0f18aea1ab3..c1059a8b445f 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/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);