From 81a3e7cce34b130dfa9761fbb21e7d8f70016d68 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Fri, 4 Dec 2020 14:14:39 +0300 Subject: [PATCH] reject too large (by size) messages (#551) --- bridges/bin/millau/runtime/src/lib.rs | 2 +- .../bin/millau/runtime/src/rialto_messages.rs | 11 +- bridges/bin/rialto/runtime/src/lib.rs | 2 +- .../bin/rialto/runtime/src/millau_messages.rs | 11 +- bridges/bin/runtime-common/src/messages.rs | 110 +++++++++++++++++- bridges/primitives/millau/src/lib.rs | 4 + bridges/primitives/rialto/src/lib.rs | 4 + 7 files changed, 127 insertions(+), 17 deletions(-) diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index 85d4e3a7ec4b..0abd9ec5a440 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -153,7 +153,7 @@ parameter_types! { pub const ExtrinsicBaseWeight: Weight = 10_000_000; pub const AvailableBlockRatio: Perbill = Perbill::from_percent(bp_millau::AVAILABLE_BLOCK_RATIO); pub MaximumExtrinsicWeight: Weight = bp_millau::MAXIMUM_EXTRINSIC_WEIGHT; - pub const MaximumBlockLength: u32 = 5 * 1024 * 1024; + pub const MaximumBlockLength: u32 = bp_millau::MAXIMUM_BLOCK_SIZE; pub const Version: RuntimeVersion = VERSION; pub const DbWeight: RuntimeDbWeight = RuntimeDbWeight { read: 60_000_000, // ~0.06 ms = ~60 µs diff --git a/bridges/bin/millau/runtime/src/rialto_messages.rs b/bridges/bin/millau/runtime/src/rialto_messages.rs index 4ff4f093d8fc..061137e2151b 100644 --- a/bridges/bin/millau/runtime/src/rialto_messages.rs +++ b/bridges/bin/millau/runtime/src/rialto_messages.rs @@ -88,6 +88,10 @@ impl MessageBridge for WithRialtoMessageBridge { type ThisChain = Millau; type BridgedChain = Rialto; + fn maximal_extrinsic_size_on_target_chain() -> u32 { + bp_rialto::MAXIMUM_EXTRINSIC_SIZE + } + fn weight_limits_of_message_on_bridged_chain(message_payload: &[u8]) -> RangeInclusive { // we don't want to relay too large messages + keep reserve for future upgrades let upper_limit = bp_rialto::MAXIMUM_EXTRINSIC_WEIGHT / 2; @@ -167,12 +171,7 @@ impl TargetHeaderChain for Rialto type MessagesDeliveryProof = ToRialtoMessagesDeliveryProof; fn verify_message(payload: &ToRialtoMessagePayload) -> Result<(), Self::Error> { - let weight_limits = WithRialtoMessageBridge::weight_limits_of_message_on_bridged_chain(&payload.call); - if !weight_limits.contains(&payload.weight) { - return Err("Incorrect message weight declared"); - } - - Ok(()) + messages::source::verify_chain_message::(payload) } fn verify_messages_delivery_proof( diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index f59137be091d..5b6716578c25 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -161,7 +161,7 @@ parameter_types! { pub const ExtrinsicBaseWeight: Weight = 10_000_000; pub const AvailableBlockRatio: Perbill = Perbill::from_percent(bp_rialto::AVAILABLE_BLOCK_RATIO); pub MaximumExtrinsicWeight: Weight = bp_rialto::MAXIMUM_EXTRINSIC_WEIGHT; - pub const MaximumBlockLength: u32 = 5 * 1024 * 1024; + pub const MaximumBlockLength: u32 = bp_rialto::MAXIMUM_BLOCK_SIZE; pub const Version: RuntimeVersion = VERSION; pub const DbWeight: RuntimeDbWeight = RuntimeDbWeight { read: 60_000_000, // ~0.06 ms = ~60 µs diff --git a/bridges/bin/rialto/runtime/src/millau_messages.rs b/bridges/bin/rialto/runtime/src/millau_messages.rs index 6a8dfba84baf..db2076eaa170 100644 --- a/bridges/bin/rialto/runtime/src/millau_messages.rs +++ b/bridges/bin/rialto/runtime/src/millau_messages.rs @@ -88,6 +88,10 @@ impl MessageBridge for WithMillauMessageBridge { type ThisChain = Rialto; type BridgedChain = Millau; + fn maximal_extrinsic_size_on_target_chain() -> u32 { + bp_millau::MAXIMUM_EXTRINSIC_SIZE + } + fn weight_limits_of_message_on_bridged_chain(message_payload: &[u8]) -> RangeInclusive { // we don't want to relay too large messages + keep reserve for future upgrades let upper_limit = bp_millau::MAXIMUM_EXTRINSIC_WEIGHT / 2; @@ -167,12 +171,7 @@ impl TargetHeaderChain for Millau type MessagesDeliveryProof = ToMillauMessagesDeliveryProof; fn verify_message(payload: &ToMillauMessagePayload) -> Result<(), Self::Error> { - let weight_limits = WithMillauMessageBridge::weight_limits_of_message_on_bridged_chain(&payload.call); - if !weight_limits.contains(&payload.weight) { - return Err("Incorrect message weight declared"); - } - - Ok(()) + messages::source::verify_chain_message::(payload) } fn verify_messages_delivery_proof( diff --git a/bridges/bin/runtime-common/src/messages.rs b/bridges/bin/runtime-common/src/messages.rs index 29caf13f95a5..82c28bfa7f68 100644 --- a/bridges/bin/runtime-common/src/messages.rs +++ b/bridges/bin/runtime-common/src/messages.rs @@ -48,6 +48,9 @@ pub trait MessageBridge { /// Bridged chain in context of message bridge. type BridgedChain: ChainWithMessageLanes; + /// Maximal extrinsic size on target chain. + fn maximal_extrinsic_size_on_target_chain() -> u32; + /// Returns feasible weights range for given message payload on the target chain. /// /// If message is being sent with the weight that is out of this range, then it @@ -98,7 +101,7 @@ pub trait ChainWithMessageLanes { /// `frame_support::weight::Weight`. But since the meaning of weight on different chains /// may be different, the `WeightOf<>` construct is used to avoid confusion between /// different weights. - type Weight: From; + type Weight: From + PartialOrd; /// Type of balances that is used on the chain. type Balance: Decode + CheckedAdd + CheckedDiv + CheckedMul + PartialOrd + From + Copy; @@ -171,6 +174,36 @@ pub mod source { } } + /// Do basic Bridged-chain specific verification of This -> Bridged chain message. + /// + /// Ok result from this function means that the delivery transaction with this message + /// may be 'mined' by the target chain. But the lane may have its own checks (e.g. fee + /// check) that would reject message (see `FromThisChainMessageVerifier`). + pub fn verify_chain_message( + payload: &FromThisChainMessagePayload, + ) -> Result<(), &'static str> { + let weight_limits = B::weight_limits_of_message_on_bridged_chain(&payload.call); + if !weight_limits.contains(&payload.weight.into()) { + return Err("Incorrect message weight declared"); + } + + // The maximal size of extrinsic at Substrate-based chain depends on the + // `frame_system::Trait::MaximumBlockLength` and `frame_system::Trait::AvailableBlockRatio` + // constants. This check is here to be sure that the lane won't stuck because message is too + // large to fit into delivery transaction. + // + // **IMPORTANT NOTE**: the delivery transaction contains storage proof of the message, not + // the message itself. The proof is always larger than the message. But unless chain state + // is enormously large, it should be several dozens/hundreds of bytes. The delivery + // transaction also contains signatures and signed extensions. Because of this, we reserve + // 1/3 of the the maximal extrinsic weight for this data. + if payload.call.len() > B::maximal_extrinsic_size_on_target_chain() as usize * 2 / 3 { + return Err("The message is too large to be sent over the lane"); + } + + Ok(()) + } + /// Estimate delivery and dispatch fee that must be paid for delivering a message to the Bridged chain. /// /// The fee is paid in This chain Balance, but we use Bridged chain balance to avoid additional conversions. @@ -511,6 +544,8 @@ mod tests { const THIS_CHAIN_WEIGHT_TO_BALANCE_RATE: Weight = 2; const BRIDGED_CHAIN_WEIGHT_TO_BALANCE_RATE: Weight = 4; const THIS_CHAIN_TO_BRIDGED_CHAIN_BALANCE_RATE: u32 = 6; + const BRIDGED_CHAIN_MAX_EXTRINSIC_WEIGHT: Weight = 2048; + const BRIDGED_CHAIN_MAX_EXTRINSIC_SIZE: u32 = 1024; /// Bridge that is deployed on ThisChain and allows sending/receiving messages to/from BridgedChain; struct OnThisChainBridge; @@ -522,8 +557,13 @@ mod tests { type ThisChain = ThisChain; type BridgedChain = BridgedChain; - fn weight_limits_of_message_on_bridged_chain(_message_payload: &[u8]) -> RangeInclusive { - unreachable!() + fn maximal_extrinsic_size_on_target_chain() -> u32 { + BRIDGED_CHAIN_MAX_EXTRINSIC_SIZE + } + + fn weight_limits_of_message_on_bridged_chain(message_payload: &[u8]) -> RangeInclusive { + let begin = std::cmp::min(BRIDGED_CHAIN_MAX_EXTRINSIC_WEIGHT, message_payload.len() as Weight); + begin..=BRIDGED_CHAIN_MAX_EXTRINSIC_WEIGHT } fn weight_of_delivery_transaction() -> Weight { @@ -561,6 +601,10 @@ mod tests { type ThisChain = BridgedChain; type BridgedChain = ThisChain; + fn maximal_extrinsic_size_on_target_chain() -> u32 { + unreachable!() + } + fn weight_limits_of_message_on_bridged_chain(_message_payload: &[u8]) -> RangeInclusive { unreachable!() } @@ -773,6 +817,66 @@ mod tests { ); } + #[test] + fn verify_chain_message_rejects_message_with_too_small_declared_weight() { + assert!( + source::verify_chain_message::(&source::FromThisChainMessagePayload::< + OnThisChainBridge, + > { + spec_version: 1, + weight: 5, + origin: pallet_bridge_call_dispatch::CallOrigin::SourceRoot, + call: vec![1, 2, 3, 4, 5, 6], + },) + .is_err() + ); + } + + #[test] + fn verify_chain_message_rejects_message_with_too_large_declared_weight() { + assert!( + source::verify_chain_message::(&source::FromThisChainMessagePayload::< + OnThisChainBridge, + > { + spec_version: 1, + weight: BRIDGED_CHAIN_MAX_EXTRINSIC_WEIGHT + 1, + origin: pallet_bridge_call_dispatch::CallOrigin::SourceRoot, + call: vec![1, 2, 3, 4, 5, 6], + },) + .is_err() + ); + } + + #[test] + fn verify_chain_message_rejects_message_too_large_message() { + assert!( + source::verify_chain_message::(&source::FromThisChainMessagePayload::< + OnThisChainBridge, + > { + spec_version: 1, + weight: BRIDGED_CHAIN_MAX_EXTRINSIC_WEIGHT, + origin: pallet_bridge_call_dispatch::CallOrigin::SourceRoot, + call: vec![0; BRIDGED_CHAIN_MAX_EXTRINSIC_SIZE as usize * 2 / 3 + 1], + },) + .is_err() + ); + } + + #[test] + fn verify_chain_message_accepts_maximal_message() { + assert_eq!( + source::verify_chain_message::(&source::FromThisChainMessagePayload::< + OnThisChainBridge, + > { + spec_version: 1, + weight: BRIDGED_CHAIN_MAX_EXTRINSIC_WEIGHT, + origin: pallet_bridge_call_dispatch::CallOrigin::SourceRoot, + call: vec![0; BRIDGED_CHAIN_MAX_EXTRINSIC_SIZE as usize * 2 / 3], + },), + Ok(()), + ); + } + #[derive(Debug)] struct TestMessageProofParser { failing: bool, diff --git a/bridges/primitives/millau/src/lib.rs b/bridges/primitives/millau/src/lib.rs index 1996b488fe4d..f3e90851be32 100644 --- a/bridges/primitives/millau/src/lib.rs +++ b/bridges/primitives/millau/src/lib.rs @@ -75,6 +75,10 @@ pub const AVAILABLE_BLOCK_RATIO: u32 = 75; /// Maximal weight of single Millau extrinsic (65% of maximum block weight = 75% for regular /// transactions minus 10% for initialization). pub const MAXIMUM_EXTRINSIC_WEIGHT: Weight = MAXIMUM_BLOCK_WEIGHT / 100 * (AVAILABLE_BLOCK_RATIO as Weight - 10); +/// Maximal size of Millau block. +pub const MAXIMUM_BLOCK_SIZE: u32 = 2 * 1024 * 1024; +/// Maximal size of single normal Millau extrinsic (75% of maximal block size). +pub const MAXIMUM_EXTRINSIC_SIZE: u32 = MAXIMUM_BLOCK_SIZE / 100 * AVAILABLE_BLOCK_RATIO; // TODO: may need to be updated after https://github.com/paritytech/parity-bridges-common/issues/78 /// Maximal number of messages in single delivery transaction. diff --git a/bridges/primitives/rialto/src/lib.rs b/bridges/primitives/rialto/src/lib.rs index 618ddf5e1204..ac7b260e1656 100644 --- a/bridges/primitives/rialto/src/lib.rs +++ b/bridges/primitives/rialto/src/lib.rs @@ -37,6 +37,10 @@ pub const AVAILABLE_BLOCK_RATIO: u32 = 75; /// Maximal weight of single Rialto extrinsic (65% of maximum block weight = 75% for regular /// transactions minus 10% for initialization). pub const MAXIMUM_EXTRINSIC_WEIGHT: Weight = MAXIMUM_BLOCK_WEIGHT / 100 * (AVAILABLE_BLOCK_RATIO as Weight - 10); +/// Maximal size of Rialto block. +pub const MAXIMUM_BLOCK_SIZE: u32 = 5 * 1024 * 1024; +/// Maximal size of single normal Rialto extrinsic (75% of maximal block size). +pub const MAXIMUM_EXTRINSIC_SIZE: u32 = MAXIMUM_BLOCK_SIZE / 100 * AVAILABLE_BLOCK_RATIO; // TODO: may need to be updated after https://github.com/paritytech/parity-bridges-common/issues/78 /// Maximal number of messages in single delivery transaction.