Prevent potential signature reuse (paritytech#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

@@ -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<Call, AccountId, SpecVersion>(
+	rialto_call: Call,
+	millau_account_id: AccountId,
+	rialto_spec_version: SpecVersion,
+) -> sp_std::vec::Vec<u8>
+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::*;

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<Call, AccountId, SpecVersion>(
+	millau_call: Call,
+	rialto_account_id: AccountId,
+	millau_spec_version: SpecVersion,
+) -> sp_std::vec::Vec<u8>
+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::*;

bridges/modules/call-dispatch/src/lib.rs

@@ -65,7 +65,7 @@ pub enum CallOrigin<SourceChainAccountId, TargetChainAccountPublic, TargetChainS
 	///
 	/// The account can be identified by `TargetChainAccountPublic`. The proof that the
 	/// `SourceChainAccountId` controls `TargetChainAccountPublic` is the `TargetChainSignature`
-	/// over `(Call, SourceChainAccountId).encode()`.
+	/// over `(Call, SourceChainAccountId, TargetChainSpecVersion, SourceChainBridgeId).encode()`.
 	///
 	/// NOTE sending messages using this origin (or any other) does not have replay protection!
 	/// The assumption is that both the source account and the target account is controlled by
@@ -247,12 +247,11 @@ impl<T: Config<I>, I: Instance> MessageDispatch<T::MessageId> for Module<T, I> {
 				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, AccountId, SpecVersion, BridgeId>(
+	call: Call,
+	source_account_id: AccountId,
+	target_spec_version: SpecVersion,
+	source_instance_id: BridgeId,
+) -> Vec<u8>
+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::*;

bridges/relays/substrate/src/main.rs

@@ -297,18 +297,21 @@ 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,
 						weight: rialto_call_weight,
 						origin: CallOrigin::TargetAccount(
 							millau_account_id,
 							rialto_origin_public.into(),
-							rialto_origin_signature.into(),
+							digest_signature.into(),
 						),
 						call: rialto_call.encode(),
 					}
@@ -445,18 +448,21 @@ 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,
 						weight: millau_call_weight,
 						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<Fee: Decode, C: Chain, P: En
 		Decode::decode(&mut &encoded_response.0[..]).map_err(relay_substrate_client::Error::ResponseParseFailed)?;
 	Ok(decoded_response)
 }
+
+#[cfg(test)]
+mod tests {
+	use sp_core::Pair;
+	use sp_runtime::traits::{IdentifyAccount, Verify};
+
+	#[test]
+	fn millau_signature_is_valid_on_rialto() {
+		let millau_sign = relay_millau_client::SigningParams::from_suri("//Dave", None).unwrap();
+
+		let call = rialto_runtime::Call::System(rialto_runtime::SystemCall::remark(vec![]));
+
+		let millau_public: bp_millau::AccountSigner = millau_sign.signer.public().clone().into();
+		let millau_account_id: bp_millau::AccountId = millau_public.into_account();
+
+		let digest = millau_runtime::rialto_account_ownership_digest(
+			call,
+			millau_account_id,
+			rialto_runtime::VERSION.spec_version,
+		);
+
+		let rialto_signer = relay_rialto_client::SigningParams::from_suri("//Dave", None).unwrap();
+		let signature = rialto_signer.signer.sign(&digest);
+
+		assert!(signature.verify(&digest[..], &rialto_signer.signer.public()));
+	}
+
+	#[test]
+	fn rialto_signature_is_valid_on_millau() {
+		let rialto_sign = relay_rialto_client::SigningParams::from_suri("//Dave", None).unwrap();
+
+		let call = millau_runtime::Call::System(millau_runtime::SystemCall::remark(vec![]));
+
+		let rialto_public: bp_rialto::AccountSigner = rialto_sign.signer.public().clone().into();
+		let rialto_account_id: bp_rialto::AccountId = rialto_public.into_account();
+
+		let digest = rialto_runtime::millau_account_ownership_digest(
+			call,
+			rialto_account_id,
+			millau_runtime::VERSION.spec_version,
+		);
+
+		let millau_signer = relay_millau_client::SigningParams::from_suri("//Dave", None).unwrap();
+		let signature = millau_signer.signer.sign(&digest);
+
+		assert!(signature.verify(&digest[..], &millau_signer.signer.public()));
+	}
+}