From 926520292e3454913dc3a7550ada21239ef4df89 Mon Sep 17 00:00:00 2001 From: Andreas Doerr Date: Tue, 26 Jan 2021 14:32:59 +0100 Subject: [PATCH] Prevent potential signature reuse (#667) * patch audit findings #42 * extend msg signature for substrate relay * signature verification test * make proof dependet on call_dispatch crate * silence clippy * revert deny exception * address code review * since it's not really a proof, call it digest --- bridges/bin/millau/runtime/src/lib.rs | 23 ++++++++ bridges/bin/rialto/runtime/src/lib.rs | 23 ++++++++ bridges/modules/call-dispatch/src/lib.rs | 35 +++++++++-- bridges/relays/substrate/src/main.rs | 74 ++++++++++++++++++++---- 4 files changed, 140 insertions(+), 15 deletions(-) diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index e79b34d2790e..90498c7cde80 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -596,6 +596,29 @@ impl_runtime_apis! { } } +/// Rialto account ownership digest from Millau. +/// +/// The byte vector returned by this function should be signed with a Rialto account private key. +/// This way, the owner of `millau_account_id` on Millau proves that the Rialto account private key +/// is also under his control. +pub fn rialto_account_ownership_digest( + rialto_call: Call, + millau_account_id: AccountId, + rialto_spec_version: SpecVersion, +) -> sp_std::vec::Vec +where + Call: codec::Encode, + AccountId: codec::Encode, + SpecVersion: codec::Encode, +{ + pallet_bridge_call_dispatch::account_ownership_digest( + rialto_call, + millau_account_id, + rialto_spec_version, + bp_runtime::MILLAU_BRIDGE_INSTANCE, + ) +} + #[cfg(test)] mod tests { use super::*; diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index 060784ab3ec5..313948e6a05b 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -985,6 +985,29 @@ impl_runtime_apis! { } } +/// Millau account ownership digest from Rialto. +/// +/// The byte vector returned by this function should be signed with a Millau account private key. +/// This way, the owner of `rialto_account_id` on Rialto proves that the 'millau' account private key +/// is also under his control. +pub fn millau_account_ownership_digest( + millau_call: Call, + rialto_account_id: AccountId, + millau_spec_version: SpecVersion, +) -> sp_std::vec::Vec +where + Call: codec::Encode, + AccountId: codec::Encode, + SpecVersion: codec::Encode, +{ + pallet_bridge_call_dispatch::account_ownership_digest( + millau_call, + rialto_account_id, + millau_spec_version, + bp_runtime::RIALTO_BRIDGE_INSTANCE, + ) +} + #[cfg(test)] mod tests { use super::*; diff --git a/bridges/modules/call-dispatch/src/lib.rs b/bridges/modules/call-dispatch/src/lib.rs index c0b1e578d75a..d60492be34ae 100644 --- a/bridges/modules/call-dispatch/src/lib.rs +++ b/bridges/modules/call-dispatch/src/lib.rs @@ -65,7 +65,7 @@ pub enum CallOrigin, I: Instance> MessageDispatch for Module { target_id } CallOrigin::TargetAccount(source_account_id, target_public, target_signature) => { - let mut signed_message = Vec::new(); - message.call.encode_to(&mut signed_message); - source_account_id.encode_to(&mut signed_message); + let digest = + account_ownership_digest(message.call.clone(), source_account_id, message.spec_version, bridge); let target_account = target_public.into_account(); - if !target_signature.verify(&signed_message[..], &target_account) { + if !target_signature.verify(&digest[..], &target_account) { frame_support::debug::trace!( "Message {:?}/{:?}: origin proof is invalid. Expected account: {:?} from signature: {:?}", bridge, @@ -334,6 +333,32 @@ where } } +/// Target account ownership digest from the source chain. +/// +/// The byte vector returned by this function will be signed with a target chain account +/// private key. This way, the owner of `source_account_id` on the source chain proves that +/// the target chain account private key is also under his control. +pub fn account_ownership_digest( + call: Call, + source_account_id: AccountId, + target_spec_version: SpecVersion, + source_instance_id: BridgeId, +) -> Vec +where + Call: Encode, + AccountId: Encode, + SpecVersion: Encode, + BridgeId: Encode, +{ + let mut proof = Vec::new(); + call.encode_to(&mut proof); + source_account_id.encode_to(&mut proof); + target_spec_version.encode_to(&mut proof); + source_instance_id.encode_to(&mut proof); + + proof +} + #[cfg(test)] mod tests { use super::*; diff --git a/bridges/relays/substrate/src/main.rs b/bridges/relays/substrate/src/main.rs index 88d436371e34..c0f0ab2bcac9 100644 --- a/bridges/relays/substrate/src/main.rs +++ b/bridges/relays/substrate/src/main.rs @@ -297,10 +297,13 @@ async fn run_command(command: cli::Command) -> Result<(), String> { call: rialto_call.encode(), }, cli::Origins::Target => { - let mut rialto_origin_signature_message = Vec::new(); - rialto_call.encode_to(&mut rialto_origin_signature_message); - millau_account_id.encode_to(&mut rialto_origin_signature_message); - let rialto_origin_signature = rialto_sign.signer.sign(&rialto_origin_signature_message); + let digest = millau_runtime::rialto_account_ownership_digest( + rialto_call.clone(), + millau_account_id.clone(), + rialto_runtime::VERSION.spec_version, + ); + + let digest_signature = rialto_sign.signer.sign(&digest); MessagePayload { spec_version: rialto_runtime::VERSION.spec_version, @@ -308,7 +311,7 @@ async fn run_command(command: cli::Command) -> Result<(), String> { origin: CallOrigin::TargetAccount( millau_account_id, rialto_origin_public.into(), - rialto_origin_signature.into(), + digest_signature.into(), ), call: rialto_call.encode(), } @@ -445,10 +448,13 @@ async fn run_command(command: cli::Command) -> Result<(), String> { call: millau_call.encode(), }, cli::Origins::Target => { - let mut millau_origin_signature_message = Vec::new(); - millau_call.encode_to(&mut millau_origin_signature_message); - rialto_account_id.encode_to(&mut millau_origin_signature_message); - let millau_origin_signature = millau_sign.signer.sign(&millau_origin_signature_message); + let digest = rialto_runtime::millau_account_ownership_digest( + millau_call.clone(), + rialto_account_id.clone(), + millau_runtime::VERSION.spec_version, + ); + + let digest_signature = millau_sign.signer.sign(&digest); MessagePayload { spec_version: millau_runtime::VERSION.spec_version, @@ -456,7 +462,7 @@ async fn run_command(command: cli::Command) -> Result<(), String> { origin: CallOrigin::TargetAccount( rialto_account_id, millau_origin_public.into(), - millau_origin_signature.into(), + digest_signature.into(), ), call: millau_call.encode(), } @@ -517,3 +523,51 @@ async fn estimate_message_delivery_and_dispatch_fee