[Feature]: Missing integrity protection for Stacks transactions issued by signer

[evonide]

(High) Missing integrity protection for Stacks transactions issued by signer

1. Description

In order to issue Stacks transactions, the signer has to sign the hash over members in structs such as StacksTransactionSignRequest: https://github.com/stacks-network/sbtc/blob/033e2cbd2def829a3e30df77bc274b325d68cdbe/signer/src/message.rs#L116-L129

However, the current hash seems to exclude a struct member such as contract_call and instead relies on a custom digest member:

https://github.com/stacks-network/sbtc/blob/033e2cbd2def829a3e30df77bc274b325d68cdbe/signer/src/message.rs#L221-L231

Currently, the verification of this digest seems to be missing. We assume this is work in progress but still file an explicit tracking bug as this is a critical check that needs to take place. Without it signer messages are not fully integrity protected and could be partially spoofed.

From a cursory look we were unable to understand the necessity of having a custom digest instead of adding the contract_call to be included in the hash here as well.

[djordon]

Currently, the verification of this digest seems to be missing. We assume this is work in progress but still file an explicit tracking bug as this is a critical check that needs to take place. Without it signer messages are not fully integrity protected and could be partially spoofed.

Yeah, this just work in progress. All of that will be checked, good call out though.

From a cursory look we were unable to understand the necessity of having a custom digest instead of adding the contract_call to be included in the hash here as well.

I viewed using the digest as sufficient. If the contract_call transaction generates the same digest then using the digest in place of the actual transaction fine. I guess I thought that it was analogous to many cryptographic signature schemes where the signature is over a digest and the check is that the transaction generates the digest and that the signature is valid, but maybe that analogy is inappropriate here? Anyway, is that reasoning incorrect/problematic?

[evonide]

The custom digest seems to introduce redundancy. Most of the struct members are already being hashed and signed, ensuring the integrity of StacksTransactionSignRequest.

In the current approach, the sender computes a digest of the contract_call, adds it to the transaction, and signs the hash of the remaining fields. The receiver then verifies both the signature on the partial transaction and checks that the digest matches the contract_call.

A simpler approach would be to sign the hash of the full transaction by including the contract_call and dropping the custom digest. This eliminates the need for a separate digest and verification, reducing complexity and potential redundancy.

However, I assume there's a good reason for the custom digest that I might not have seen. Semantically, both approaches will be safe once the digest is correctly computed and verified.

[djordon]

I don't think there is a good reason per se. I just didn't want to implement AsRef<[u8]> for ContractCall, which is necessary for the sha2::Sha256 type and the digest seemed simplest but secure "enough".

But I asked not to push back, I'm genuinely curious about how best to think about these things. It makes everyone's lives easier if I think about these trade-offs correctly in the future.

[evonide]

@AshtonStephens and @djordon: I've just double checked pending AR issues and their milestone mapping. I would suggest to move this one to the sBTC: Deposit ready milestone (https://github.com/stacks-network/sbtc/milestone/9). My reasoning is that:

https://github.com/stacks-network/sbtc/blob/9aabc8bd0aa2ceac52056e3047115a872a887534/signer/src/transaction_signer.rs#L505-L509 currently contains debug assert statements which could leave sBTC in a potentially vulnerable state for a mainnet launch next month otherwise.

[djordon]

Oh we're going to fix this. The reason why we haven't directly tackled this already is because I'm itching to pull in https://github.com/stacks-network/sbtc/issues/412, where I'll remove this field and sign the entire message bytes all the time. Using protobufs and signing the entire message will solve this issue, and will make life better as we upgrade the protocol.