antiklepto: validate ECDSA signatures

CHANGELOG-rust.md

@@ -1,7 +1,10 @@
 # Changelog
 
 ## [Unreleased]
+
+## 0.13.1
 - Restrict persisted Noise config files to private permissions on Unix.
+- Validate ECDSA signatures and recovery IDs in Anti-Klepto and direct signing flows.
 
 ## 0.13.0
 - Add `BitBox::from_transport()`

Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "bitbox-api"
 authors = ["Marko Bencun <benma@bitbox.swiss>"]
-version = "0.13.0"
+version = "0.13.1"
 homepage = "https://bitbox.swiss/"
 repository = "https://github.com/BitBoxSwiss/bitbox-api-rs/"
 readme = "README-rust.md"

src/antiklepto.rs

@@ -14,6 +14,8 @@ pub enum Error {
     GenNonce,
     #[error("{0}")]
     VerificationErr(&'static str),
+    #[error(transparent)]
+    InvalidSignature(#[from] crate::secp256k1::ValidationError),
     #[error(
         "Could not verify that the host nonce was contributed to the signature. \
 		 If this happens repeatedly, the device might be attempting to leak the \
@@ -43,13 +45,32 @@ pub fn host_commit(host_nonce: &[u8]) -> [u8; 32] {
     tagged_sha256(b"s2c/ecdsa/data", host_nonce)
 }
 
-/// antikleptoVerify verifies that hostNonce was used to tweak the nonce during signature
-/// generation according to k' = k + H(clientCommitment, hostNonce) by checking that
-/// k'*G = signerCommitment + H(signerCommitment, hostNonce)*G.
+/// Verifies that `host_nonce` was used to tweak the nonce during signature generation according
+/// to k' = k + H(signer_commitment, host_nonce) by checking that
+/// k'*G = signer_commitment + H(signer_commitment, host_nonce)*G.
 pub fn verify_ecdsa(
     host_nonce: &[u8],
     signer_commitment: &[u8],
     signature: &[u8],
+) -> Result<(), Error> {
+    crate::secp256k1::validate_signature_compact(signature)?;
+    verify_ecdsa_nonce(host_nonce, signer_commitment, signature)
+}
+
+/// Validates a recoverable ECDSA signature and verifies its Anti-Klepto nonce contribution.
+pub fn verify_recoverable_ecdsa(
+    host_nonce: &[u8],
+    signer_commitment: &[u8],
+    signature: &[u8],
+) -> Result<(), Error> {
+    crate::secp256k1::validate_signature_recoverable(signature)?;
+    verify_ecdsa_nonce(host_nonce, signer_commitment, &signature[..64])
+}
+
+fn verify_ecdsa_nonce(
+    host_nonce: &[u8],
+    signer_commitment: &[u8],
+    signature: &[u8],
 ) -> Result<(), Error> {
     let secp = Secp256k1::new();
     let signer_commitment_pubkey = PublicKey::from_slice(signer_commitment)
@@ -143,4 +164,40 @@ mod tests {
             .is_err());
         }
     }
+
+    #[test]
+    fn test_verify_ecdsa_rejects_high_s() {
+        let unhex = |s| FromHex::from_hex(s).unwrap();
+        let host_nonce: Vec<u8> =
+            unhex("8b4c26aa2695a34bdbc34235f6c91be14b93037a063b13f7c814101359561092");
+        let signer_commitment: Vec<u8> =
+            unhex("0236ff92fe02c08d0d04851e0ce1516104085215f05a178307de60ea53e207f971");
+        let low_s: Vec<u8> = unhex(
+            "7fd66b48ffea2fe048869880bbb3a1819e262af14980e8885df1e5765750cb8f47e01eca356377870356d54853573a955076228e5044cd3dd3a049abe70d5585",
+        );
+        let high_s: Vec<u8> = unhex(
+            "7fd66b48ffea2fe048869880bbb3a1819e262af14980e8885df1e5765750cb8fb81fe135ca9c8878fca92ab7aca8c5696a38ba585f03d2fdec3214e0e928ebbc",
+        );
+
+        assert!(verify_ecdsa(&host_nonce, &signer_commitment, &low_s).is_ok());
+        assert!(verify_ecdsa(&host_nonce, &signer_commitment, &high_s).is_err());
+    }
+
+    #[test]
+    fn test_verify_recoverable_ecdsa() {
+        let unhex = |s| FromHex::from_hex(s).unwrap();
+        let host_nonce: Vec<u8> =
+            unhex("8b4c26aa2695a34bdbc34235f6c91be14b93037a063b13f7c814101359561092");
+        let signer_commitment: Vec<u8> =
+            unhex("0236ff92fe02c08d0d04851e0ce1516104085215f05a178307de60ea53e207f971");
+        let mut signature: Vec<u8> = unhex(
+            "7fd66b48ffea2fe048869880bbb3a1819e262af14980e8885df1e5765750cb8f47e01eca356377870356d54853573a955076228e5044cd3dd3a049abe70d558500",
+        );
+
+        assert!(verify_recoverable_ecdsa(&host_nonce, &signer_commitment, &signature).is_ok());
+        signature[64] = 3;
+        assert!(verify_recoverable_ecdsa(&host_nonce, &signer_commitment, &signature).is_ok());
+        signature[64] = 4;
+        assert!(verify_recoverable_ecdsa(&host_nonce, &signer_commitment, &signature).is_err());
+    }
 }

src/btc.rs

@@ -1112,7 +1112,7 @@ impl<R: Runtime> PairedBitBox<R> {
             }
             _ => return Err(Error::UnexpectedResponse),
         };
-        crate::antiklepto::verify_ecdsa(&host_nonce, &signer_commitment, &signature)?;
+        crate::antiklepto::verify_recoverable_ecdsa(&host_nonce, &signer_commitment, &signature)?;
 
         let sig = signature[..64].to_vec();
         let recid = signature[64];

src/eth.rs

@@ -476,7 +476,11 @@ impl<R: Runtime> PairedBitBox<R> {
                     .await?
                 {
                     pb::eth_response::Response::Sign(pb::EthSignResponse { signature }) => {
-                        crate::antiklepto::verify_ecdsa(&host_nonce, commitment, &signature)?;
+                        crate::antiklepto::verify_recoverable_ecdsa(
+                            &host_nonce,
+                            commitment,
+                            &signature,
+                        )?;
                         signature.try_into().map_err(|_| Error::UnexpectedResponse)
                     }
                     _ => Err(Error::UnexpectedResponse),
@@ -733,10 +737,15 @@ impl<R: Runtime> PairedBitBox<R> {
                 .await?
         } else {
             match response {
-                pb::eth_response::Response::Sign(pb::EthSignResponse { signature }) => signature
-                    .as_slice()
-                    .try_into()
-                    .map_err(|_| Error::UnexpectedResponse)?,
+                pb::eth_response::Response::Sign(pb::EthSignResponse { signature }) => {
+                    let signature: [u8; 65] = signature
+                        .as_slice()
+                        .try_into()
+                        .map_err(|_| Error::UnexpectedResponse)?;
+                    crate::secp256k1::validate_signature_recoverable(&signature)
+                        .map_err(|_| Error::InvalidSignature)?;
+                    signature
+                }
                 _ => return Err(Error::UnexpectedResponse),
             }
         };

src/lib.rs

@@ -22,6 +22,7 @@ mod antiklepto;
 mod communication;
 mod constants;
 mod keypath;
+mod secp256k1;
 mod u2fframing;
 mod util;
 

src/secp256k1.rs

@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: Apache-2.0
+
+use bitcoin::secp256k1::ecdsa::Signature;
+use thiserror::Error;
+
+/// An invalid ECDSA signature encoding.
+#[derive(Debug, Error)]
+#[error("{0}")]
+pub struct ValidationError(&'static str);
+
+/// Validates a 64-byte compact ECDSA signature encoded as `r || s`.
+///
+/// Both scalars must be nonzero and in range, and `s` must use its low-S encoding.
+pub(crate) fn validate_signature_compact(signature: &[u8]) -> Result<(), ValidationError> {
+    if signature.len() != 64 {
+        return Err(ValidationError("Signature must be 64 bytes"));
+    }
+
+    let parsed_signature = Signature::from_compact(signature)
+        .map_err(|_| ValidationError("Failed to parse ECDSA signature"))?;
+    if signature[..32].iter().all(|byte| *byte == 0)
+        || signature[32..].iter().all(|byte| *byte == 0)
+    {
+        return Err(ValidationError("Invalid ECDSA signature"));
+    }
+    let mut normalized_signature = parsed_signature;
+    normalized_signature.normalize_s();
+    if normalized_signature != parsed_signature {
+        return Err(ValidationError("ECDSA signature has high S"));
+    }
+    Ok(())
+}
+
+/// Validates a 65-byte recoverable ECDSA signature encoded as `r || s || recovery_id`.
+///
+/// The compact signature must be valid and the recovery ID must be in the range 0..=3.
+pub(crate) fn validate_signature_recoverable(signature: &[u8]) -> Result<(), ValidationError> {
+    if signature.len() != 65 {
+        return Err(ValidationError("Signature must be 65 bytes"));
+    }
+    validate_signature_compact(&signature[..64])?;
+    if signature[64] > 3 {
+        return Err(ValidationError("Invalid recovery ID"));
+    }
+    Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use bitcoin::hashes::hex::FromHex;
+
+    fn valid_signature() -> Vec<u8> {
+        FromHex::from_hex(
+            "7fd66b48ffea2fe048869880bbb3a1819e262af14980e8885df1e5765750cb8f47e01eca356377870356d54853573a955076228e5044cd3dd3a049abe70d5585",
+        )
+        .unwrap()
+    }
+
+    #[test]
+    fn test_validate_signature_compact() {
+        let signature = valid_signature();
+        assert!(validate_signature_compact(&signature).is_ok());
+
+        let high_s: Vec<u8> = FromHex::from_hex(
+            "7fd66b48ffea2fe048869880bbb3a1819e262af14980e8885df1e5765750cb8fb81fe135ca9c8878fca92ab7aca8c5696a38ba585f03d2fdec3214e0e928ebbc",
+        )
+        .unwrap();
+        assert!(Signature::from_compact(&high_s).is_ok());
+        assert!(validate_signature_compact(&high_s).is_err());
+
+        assert!(validate_signature_compact(&signature[..63]).is_err());
+        let mut too_long = signature.clone();
+        too_long.push(0);
+        assert!(validate_signature_compact(&too_long).is_err());
+
+        for offset in [0, 32] {
+            let mut zero = signature.clone();
+            zero[offset..offset + 32].fill(0);
+            assert!(validate_signature_compact(&zero).is_err());
+
+            let mut out_of_range = signature.clone();
+            out_of_range[offset..offset + 32]
+                .copy_from_slice(&bitcoin::secp256k1::constants::CURVE_ORDER);
+            assert!(validate_signature_compact(&out_of_range).is_err());
+        }
+    }
+
+    #[test]
+    fn test_validate_signature_recoverable() {
+        let mut signature = valid_signature();
+        signature.push(0);
+        assert!(validate_signature_recoverable(&signature).is_ok());
+        assert!(validate_signature_recoverable(&signature[..64]).is_err());
+
+        signature[64] = 3;
+        assert!(validate_signature_recoverable(&signature).is_ok());
+
+        signature[64] = 4;
+        assert!(validate_signature_recoverable(&signature).is_err());
+    }
+}