Permanent freezing of funds (fix requires hardfork)
API crash preventing correct processing of deposits
Description
Brief/Intro
When the signer processes a gossip message, it verifies the signature of the source of the message. This prevents attackers from forwarding tampered messages.
The signer determines where the digest starts based on tag 1 of the message. However, an attacker can customize the position of tag 1 to customize the position where the digest starts.
Vulnerability Details
The decode_with_digest code is as follows.
pub fn decode_with_digest(data: &[u8]) -> Result<(Self, [u8; 32]), Error> {
let mut buf = data;
let mut pre_hash_data = data;
// This is almost exactly what prost does when decoding protobuf
// bytes.
let mut message = proto::Signed::default();
let ctx = prost::encoding::DecodeContext::default();
while buf.has_remaining() {
let (tag, wire_type) =
prost::encoding::decode_key(&mut buf).map_err(Error::DecodeProtobuf)?;
message
.merge_field(tag, wire_type, &mut buf, ctx.clone())
.map_err(Error::DecodeProtobuf)?;
// This part here is not in prost. The purpose is to note the
// pre-hashed-data that is hashed and then signed, and the
// underlying assumption is that all non-signature field bytes
// are signed. The approach here assumes that protobuf fields
// are serialized in order of their tag and the field with tag
// 1 is the signature field. We copy a reference to the
// remaining bytes because these bytes were used to create the
// digest that was signed over.
if tag == 1 {
pre_hash_data = buf;
}
}
// Okay now we transform the protobuf type into our local type.
let msg = Signed::<SignerMessage>::try_from(message)?;
// Now we construct the digest that was signed over.
let mut hasher = sha2::Sha256::new_with_prefix(msg.type_tag());
hasher.update(pre_hash_data);
Ok((msg, hasher.finalize().into()))
}
The pre_hash_data is used to mark the beginning of the digest. As long as tag is 1, the pre_hash_data will be reassigned to the current buf. Therefore, we can determine that the expected message structure is:
+--------------------------------+
| tag = 1, signature data |
+--------------------------------+ >>>>> digest start
| tag = 2, <field 2> data |
+--------------------------------+
| ... |
+--------------------------------+
| tag = x, <field x> data |
+--------------------------------+
>>>>> digest end
However, the attacker can manipulate the digest content by disrupting the order of tags. As shown in the figure below.
+--------------------------------+
| tag = 2, <field 2> data |
+--------------------------------+
| ... |
+--------------------------------+
| tag = x, <field x> data |
+--------------------------------+
| tag = 1, signature data |
+--------------------------------+ >>>>> digest start
>>>>> digest end
The attacker can put tag 1 at the end of the message. Then when decode_with_digest finds tag 1, it will assign pre_hash_data to the current buf, which is the end of the message, and the digest content will be empty.
Fortunately, the signer will add a prefix to all digests. So, for an attacker, he needs to induce other signers to sign a signature with only a prefix like "SBTC_SIGNER_WITHDRAWAL_DECISION". Then the attacker can use this signature to forge any SignerWithdrawalDecision message.
Fix
It is recommended to put the signature field first in the message instead of allowing the message to customize the signature field position.
Impact Details
This bug allows a signer to spread gossip messages as other signers. He can spread wrong SignerDepositDecision, SignerWithdrawalDecision (which can freeze the user's sBTC) and WstsMessage (which can prevent new signer set) messages.
However, this bug requires the attacker to be one of the signers (only signers can forward gossip) and requires the signer to induce other signers to sign a signature with only the prefix. So, I consider this bug to be a Medium.
References
None
Proof of Concept
Proof of Concept
Add this test case to signer/src/ecdsa.rs. This PoC demonstrates that after obtaining the "SBTC_SIGNER_WITHDRAWAL_DECISION" signature, it is possible to forge any SignerWithdrawalDecision message.
