check that wsts messages are signed by the specified signer_id (#723)

* add coordinator checks

* once we have a chain tip, use the correct signer as coordinator

* use HashMap::get rather than index directly; replace resulting copypasta with macro

* move message authentication into separate fn

* pass chain tip report into handle_wsts_message

* clippy fixes

* use non-panic hashmap access; convert macro to closure

* no need to ref when calling hashmap::get

* fix weird cargo fmt spacing on tracing call; warn about wrong coordinator then return Ok

* resolve scoping issue with warn

signer/src/error.rs

@@ -487,6 +487,10 @@ pub enum Error {
     /// The smart contract has already been deployed
     #[error("smart contract already deployed, contract name: {0}")]
     ContractAlreadyDeployed(&'static str),
+
+    /// Received coordinator message wasn't from coordinator for this chain tip
+    #[error("not chain tip coordinator")]
+    NotChainTipCoordinator,
 }
 
 impl From<std::convert::Infallible> for Error {

signer/src/testing/transaction_signer.rs

@@ -562,6 +562,18 @@ where
             .expect("storage error")
             .expect("no chain tip");
 
+        // now that we have a chain tip, get the real coordinator
+        let coordinator_public_key =
+            crate::transaction_coordinator::coordinator_public_key(&bitcoin_chain_tip, signer_set)
+                .unwrap();
+        let coordinator_signer_info = signer_info
+            .iter()
+            .find(|signer| {
+                PublicKey::from_private_key(&signer.signer_private_key) == coordinator_public_key
+            })
+            .unwrap()
+            .clone();
+
         run_dkg_and_store_results_for_signers(
             &signer_info,
             &bitcoin_chain_tip,

signer/src/transaction_coordinator.rs

@@ -884,6 +884,13 @@ where
         coordinator_state_machine: &mut CoordinatorStateMachine,
         txid: bitcoin::Txid,
     ) -> Result<WstsOperationResult, Error> {
+        // this assumes that the signer set doesn't change for the duration of this call,
+        // but we're already assuming that the bitcoin chain tip doesn't change
+        // alternately we could hit the DB every time we get a new message
+        let (_, signer_set) = self
+            .get_signer_set_and_aggregate_key(bitcoin_chain_tip)
+            .await?;
+
         loop {
             // Let's get the next message from the network or the
             // TxSignerEventLoop.
@@ -894,6 +901,11 @@ where
                 continue;
             }
 
+            if !msg.verify() {
+                tracing::warn!(?msg, "invalid signature");
+                continue;
+            }
+
             let Payload::WstsMessage(wsts_msg) = msg.inner.payload else {
                 continue;
             };
@@ -903,6 +915,26 @@ where
                 sig: Vec::new(),
             };
 
+            let msg_public_key = msg.signer_pub_key;
+
+            let sender_is_coordinator =
+                given_key_is_coordinator(msg_public_key, bitcoin_chain_tip, &signer_set);
+
+            let public_keys = &coordinator_state_machine.get_config().signer_public_keys;
+            let public_key_point = p256k1::point::Point::from(msg_public_key);
+
+            // check that messages were signed by correct key
+            let is_authenticated = Self::authenticate_message(
+                &packet,
+                public_keys,
+                public_key_point,
+                sender_is_coordinator,
+            );
+
+            if !is_authenticated {
+                continue;
+            }
+
             let (outbound_packet, operation_result) =
                 match coordinator_state_machine.process_message(&packet) {
                     Ok(val) => val,
@@ -924,6 +956,66 @@ where
         }
     }
 
+    fn authenticate_message(
+        packet: &wsts::net::Packet,
+        public_keys: &hashbrown::HashMap<u32, p256k1::point::Point>,
+        public_key_point: p256k1::point::Point,
+        sender_is_coordinator: bool,
+    ) -> bool {
+        let check_signer_public_key = |signer_id| match public_keys.get(&signer_id) {
+            Some(signer_public_key) if public_key_point != *signer_public_key => {
+                tracing::warn!(
+                    ?packet.msg,
+                    reason = "message was signed by the wrong signer",
+                    "ignoring packet"
+                );
+                false
+            }
+            None => {
+                tracing::warn!(
+                    ?packet.msg,
+                    reason = "no public key for signer",
+                    %signer_id,
+                    "ignoring packet"
+                );
+                false
+            }
+            _ => true,
+        };
+        match &packet.msg {
+            wsts::net::Message::DkgBegin(_)
+            | wsts::net::Message::DkgPrivateBegin(_)
+            | wsts::net::Message::DkgEndBegin(_)
+            | wsts::net::Message::NonceRequest(_)
+            | wsts::net::Message::SignatureShareRequest(_) => {
+                if !sender_is_coordinator {
+                    tracing::warn!(
+                        ?packet,
+                        reason = "got coordinator message from sender who is not coordinator",
+                        "ignoring packet"
+                    );
+                    false
+                } else {
+                    true
+                }
+            }
+
+            wsts::net::Message::DkgPublicShares(dkg_public_shares) => {
+                check_signer_public_key(dkg_public_shares.signer_id)
+            }
+            wsts::net::Message::DkgPrivateShares(dkg_private_shares) => {
+                check_signer_public_key(dkg_private_shares.signer_id)
+            }
+            wsts::net::Message::DkgEnd(dkg_end) => check_signer_public_key(dkg_end.signer_id),
+            wsts::net::Message::NonceResponse(nonce_response) => {
+                check_signer_public_key(nonce_response.signer_id)
+            }
+            wsts::net::Message::SignatureShareResponse(sig_share_response) => {
+                check_signer_public_key(sig_share_response.signer_id)
+            }
+        }
+    }
+
     // Determine if the current coordinator is the coordinator.
     //
     // The coordinator is decided using the hash of the bitcoin

signer/src/transaction_signer.rs

@@ -349,8 +349,13 @@ where
             }
 
             (message::Payload::WstsMessage(wsts_msg), _, _) => {
-                self.handle_wsts_message(wsts_msg, &msg.bitcoin_chain_tip)
-                    .await?;
+                self.handle_wsts_message(
+                    wsts_msg,
+                    &msg.bitcoin_chain_tip,
+                    msg.signer_pub_key,
+                    &chain_tip_report,
+                )
+                .await?;
             }
 
             (
@@ -579,10 +584,18 @@ where
         &mut self,
         msg: &message::WstsMessage,
         bitcoin_chain_tip: &model::BitcoinBlockHash,
+        msg_public_key: PublicKey,
+        chain_tip_report: &MsgChainTipReport,
     ) -> Result<(), Error> {
         tracing::info!("handling message");
+
         match &msg.inner {
             wsts::net::Message::DkgBegin(_) => {
+                if !chain_tip_report.sender_is_coordinator {
+                    tracing::warn!("Got coordinator message from wrong signer");
+                    return Ok(());
+                }
+
                 let signer_public_keys = self.get_signer_public_keys(bitcoin_chain_tip).await?;
 
                 let state_machine = wsts_state_machine::SignerStateMachine::new(
@@ -594,13 +607,54 @@ where
                 self.relay_message(msg.txid, &msg.inner, bitcoin_chain_tip)
                     .await?;
             }
-            wsts::net::Message::DkgPublicShares(_)
-            | wsts::net::Message::DkgPrivateBegin(_)
-            | wsts::net::Message::DkgPrivateShares(_) => {
+            wsts::net::Message::DkgPrivateBegin(_) => {
+                if !chain_tip_report.sender_is_coordinator {
+                    tracing::warn!("Got coordinator message from wrong signer");
+                    return Ok(());
+                }
+
+                self.relay_message(msg.txid, &msg.inner, bitcoin_chain_tip)
+                    .await?;
+            }
+            wsts::net::Message::DkgPublicShares(dkg_public_shares) => {
+                let public_keys = match self.wsts_state_machines.get(&msg.txid) {
+                    Some(state_machine) => &state_machine.public_keys,
+                    None => return Err(Error::MissingStateMachine),
+                };
+                let signer_public_key = match public_keys.signers.get(&dkg_public_shares.signer_id)
+                {
+                    Some(key) => PublicKey::from(key),
+                    None => return Err(Error::MissingPublicKey),
+                };
+
+                if signer_public_key != msg_public_key {
+                    return Err(Error::InvalidSignature);
+                }
+                self.relay_message(msg.txid, &msg.inner, bitcoin_chain_tip)
+                    .await?;
+            }
+            wsts::net::Message::DkgPrivateShares(dkg_private_shares) => {
+                let public_keys = match self.wsts_state_machines.get(&msg.txid) {
+                    Some(state_machine) => &state_machine.public_keys,
+                    None => return Err(Error::MissingStateMachine),
+                };
+                let signer_public_key = match public_keys.signers.get(&dkg_private_shares.signer_id)
+                {
+                    Some(key) => PublicKey::from(key),
+                    None => return Err(Error::MissingPublicKey),
+                };
+
+                if signer_public_key != msg_public_key {
+                    return Err(Error::InvalidSignature);
+                }
                 self.relay_message(msg.txid, &msg.inner, bitcoin_chain_tip)
                     .await?;
             }
             wsts::net::Message::DkgEndBegin(_) => {
+                if !chain_tip_report.sender_is_coordinator {
+                    tracing::warn!("Got coordinator message from wrong signer");
+                    return Ok(());
+                }
                 self.relay_message(msg.txid, &msg.inner, bitcoin_chain_tip)
                     .await?;
                 self.store_dkg_shares(&msg.txid).await?;
@@ -614,6 +668,10 @@ where
             // warning.
             #[allow(clippy::map_entry)]
             wsts::net::Message::NonceRequest(_) => {
+                if !chain_tip_report.sender_is_coordinator {
+                    tracing::warn!("Got coordinator message from wrong signer");
+                    return Ok(());
+                }
                 // TODO(296): Validate that message is the appropriate sighash
                 if !self.wsts_state_machines.contains_key(&msg.txid) {
                     let (maybe_aggregate_key, _) = self
@@ -634,6 +692,11 @@ where
                     .await?;
             }
             wsts::net::Message::SignatureShareRequest(_) => {
+                if !chain_tip_report.sender_is_coordinator {
+                    tracing::warn!("Got coordinator message from wrong signer");
+                    return Ok(());
+                }
+
                 // TODO(296): Validate that message is the appropriate sighash
                 self.relay_message(msg.txid, &msg.inner, bitcoin_chain_tip)
                     .await?;