#37811 [BC-High] Missing length check when parsing `SignatureShareRequest` in the signers allows the

Submitted on Dec 16th 2024 at 14:55:45 UTC by @n4nika for Attackathon | Stacks

  • Report ID: #37811

  • 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

The coordinator can DoS all other signers by sending a SignatureShareRequest with a big nonce_responses array since signers do not validate the length of that vector and each element in it increases the required computing power by a lot.

Finding Description

When handling incoming SignatureShareRequest requests, signers do no checks on the length of the message's nonce_responses element which is a vector of rather complex objects. This message is passed to SignerStateMachine::process which handles the signing of that message, which in the case of a SignatureShareRequest, calls wsts::sign_share_request which, as the name suggests, handles the signing of share requests. This signing process is quite compute-heavy and the amount of computing power needed increases with each element in nonce_responses whose signer_id matches the one of the signer executing this.

fn sign_share_request(
    &mut self,
    sign_request: &SignatureShareRequest,
) -> Result<Vec<Message>, Error> {
    let mut msgs = vec![];

    let signer_ids = sign_request
        .nonce_responses
        .iter()
        .map(|nr| nr.signer_id)
        .collect::<Vec<u32>>();

    for signer_id in &signer_ids {
        if *signer_id == self.signer_id {
            let key_ids: Vec<u32> = sign_request
                .nonce_responses
                .iter()
                .flat_map(|nr| nr.key_ids.iter().copied())
                .collect::<Vec<u32>>();
            let nonces = sign_request // [1] <-----
                .nonce_responses
                .iter()
                .flat_map(|nr| nr.nonces.clone())
                .collect::<Vec<PublicNonce>>();
            // [...]
        } else {
            debug!("SignatureShareRequest for {} dropped.", signer_id);
        }
    }
    Ok(msgs)
}

We can see at [1], that for every matching nonce request (for signer_id in &signer_ids), we iterate over ALL nonce_responses again, showing that this is very compute-heavy.

Note: the wsts library is out of scope for this contest, however here I am not stating that the rootcause lies in that library itself, but the unrestricted usage of said library. Namely, the missing basic checks on the sanity of the received payload.

Impact

This allows an attacker (the coordinator) to send malformed SignatureShareRequest to signers (one for each signer since the signer_id needs to match the receiving signer's), halting them due to the high required computing power. This, in turn, will shut down the system once we DoSed >4 signers.

This becomes worse when we look at the specified minimum requirements for signers stated here: https://docs.stacks.co/guides-and-tutorials/running-a-signer#minimum-system-requirements

Mitigation

Consider rejecting signing requests with too many nonce_requests. It seems to me like we need one request per signer in the signer set. In that case we could limit it to that.

Proof of Concept

PoC

This PoC halted the test on my machine for at least 30 minutes after which I stopped it.

In order to execute it, please apply the following diff and execute it with cargo test --package signer --test integration -- transaction_coordinator::sign_bitcoin_transaction_poc --exact --show-output --ignored --nocapture. In parallel you can inspect the ressource consumption with for example btop.

Last updated

Was this helpful?