fix: allow secp256k1-recover? to accept high-S signatures

clarity/src/vm/tests/crypto.rs

@@ -49,6 +49,29 @@ fn zeroed_buff_literal(len: usize) -> String {
     buff_literal(&vec![0u8; len])
 }
 
+#[test]
+fn test_secp256k1_recover_accepts_high_s_signature() {
+    let message = "0x7147f89f7ba4980c8628b52c2f0351f018ed31ba593e5ed676ad428c67c23ffb";
+    let signature = "0xe120eaed297a125259ee235a702c3f8dc18f8e65cdb28625061dd9e80197b0e6d29c9b9a200ecffee51033a93c896e9e00907789888eef42f3ede3a81dd7730201";
+    let expected_pubkey = "0x034170a2083dccbc2be253885a8d0e9f7ce859eb370d0c5cae3b6994af4cb9d666";
+    let fallback = zeroed_buff_literal(33);
+    let program = format!(
+        "(is-eq (unwrap! (secp256k1-recover? {message} {signature}) {fallback}) {expected_pubkey})"
+    );
+
+    assert_eq!(
+        Value::Bool(true),
+        execute_with_parameters(
+            program.as_str(),
+            ClarityVersion::Clarity1,
+            StacksEpochId::Epoch20,
+            false
+        )
+        .expect("execution should succeed")
+        .expect("should return a value")
+    );
+}
+
 #[test]
 fn test_secp256r1_verify_valid_signature_returns_true() {
     let (message, signature, pubkey) = secp256r1_vectors();

stacks-common/Cargo.toml

@@ -75,6 +75,7 @@ sha2 = { version = "0.10" }
 
 [dev-dependencies]
 proptest = "1.6.0"
+stacks_common_v3_1_00_13 = { package = "stacks-common", git = "https://github.com/stacks-network/stacks-core.git", rev="8a79aaa7df0f13dfc5ab0d0d0bcb8201c90bcba2", features = ["testing", "default"]}
 
 [target.'cfg(windows)'.dev-dependencies]
 winapi = { version = "0.3", features = ["fileapi", "processenv", "winnt"] }

stacks-common/src/util/secp256k1.rs

@@ -250,12 +250,22 @@ impl Secp256k1PublicKey {
             .to_secp256k1_recoverable()
             .ok_or("Invalid signature: failed to decode recoverable signature")?;
 
-        let recovered_key = K256VerifyingKey::recover_from_prehash(
-            msg,
-            &recoverable_sig.signature,
-            recoverable_sig.recovery_id,
-        )
-        .map_err(|_| "Invalid signature: failed to recover public key")?;
+        let RecoverableSignature {
+            signature,
+            recovery_id,
+        } = recoverable_sig;
+
+        let normalized_signature = signature.normalize_s();
+        let (signature_ref, recovery_id) = if let Some(normalized) = normalized_signature.as_ref() {
+            let flipped_recovery =
+                K256RecoveryId::new(!recovery_id.is_y_odd(), recovery_id.is_x_reduced());
+            (normalized, flipped_recovery)
+        } else {
+            (&signature, recovery_id)
+        };
+
+        let recovered_key = K256VerifyingKey::recover_from_prehash(msg, signature_ref, recovery_id)
+            .map_err(|_| "Invalid signature: failed to recover public key")?;
 
         Ok(Secp256k1PublicKey {
             key: recovered_key,
@@ -475,14 +485,25 @@ pub fn secp256k1_recover(
         return Err(Secp256k1Error::InvalidSignature);
     }
 
-    let recovery_id = K256RecoveryId::from_byte(serialized_signature_arr[64])
+    let mut recovery_id = K256RecoveryId::from_byte(serialized_signature_arr[64])
         .ok_or(Secp256k1Error::InvalidRecoveryId)?;
 
     let signature = K256Signature::from_slice(&serialized_signature_arr[..64])
         .map_err(|_| Secp256k1Error::InvalidSignature)?;
 
+    let normalized_signature = signature.normalize_s();
+
+    let signature_ref: &K256Signature = if let Some(normalized) = normalized_signature.as_ref() {
+        let flipped_recovery_id =
+            K256RecoveryId::new(!recovery_id.is_y_odd(), recovery_id.is_x_reduced());
+        recovery_id = flipped_recovery_id;
+        normalized
+    } else {
+        &signature
+    };
+
     let recovered_pub =
-        K256VerifyingKey::recover_from_prehash(message_arr, &signature, recovery_id)
+        K256VerifyingKey::recover_from_prehash(message_arr, signature_ref, recovery_id)
             .map_err(|_| Secp256k1Error::RecoveryFailed)?;
 
     let public_key = K256PublicKey::from(&recovered_pub);
@@ -525,7 +546,12 @@ pub fn secp256k1_verify(
 
 #[cfg(test)]
 mod tests {
+    use proptest::prelude::*;
     use rand::RngCore;
+    use stacks_common_v3_1_00_13::types::{
+        PrivateKey as LegacyPrivateKey, PublicKey as LegacyPublicKey,
+    };
+    use stacks_common_v3_1_00_13::util::secp256k1 as legacy;
 
     use super::*;
     use crate::util::get_epoch_time_ms;
@@ -1012,4 +1038,137 @@ mod tests {
             panic!("High-s signature should normalize to low-s");
         }
     }
+
+    fn is_high_s_signature(sig_bytes: &[u8]) -> bool {
+        assert_eq!(sig_bytes.len(), 64);
+        let signature =
+            K256Signature::from_slice(sig_bytes).expect("signature bytes should form valid r,s");
+        signature.normalize_s().is_some()
+    }
+
+    fn to_high_s_rsv(signature_rsv: &[u8]) -> Vec<u8> {
+        assert_eq!(signature_rsv.len(), 65);
+        if is_high_s_signature(&signature_rsv[..64]) {
+            return signature_rsv.to_vec();
+        }
+
+        let low_sig =
+            K256Signature::from_slice(&signature_rsv[..64]).expect("valid signature bytes");
+        let (r, s) = (low_sig.r(), low_sig.s());
+        let high_sig =
+            K256Signature::from_scalars(*r, -*s).expect("valid signature when flipping s");
+        let mut signature = signature_rsv.to_vec();
+        let high_bytes = high_sig.to_bytes();
+        signature[..64].copy_from_slice(high_bytes.as_slice());
+        signature[64] ^= 0x01;
+
+        assert!(is_high_s_signature(&signature[..64]));
+        signature
+    }
+
+    proptest! {
+        #[test]
+        fn prop_from_seed_matches_legacy(seed in proptest::collection::vec(any::<u8>(), 1..128)) {
+            let new_sk = Secp256k1PrivateKey::from_seed(&seed);
+            let legacy_sk = legacy::Secp256k1PrivateKey::from_seed(&seed);
+
+            prop_assert_eq!(new_sk.to_hex(), legacy_sk.to_hex());
+
+            let new_pk = Secp256k1PublicKey::from_private(&new_sk);
+            let legacy_pk = legacy::Secp256k1PublicKey::from_private(&legacy_sk);
+
+            prop_assert_eq!(new_pk.to_hex(), legacy_pk.to_hex());
+        }
+
+        #[test]
+        fn prop_low_s_paths_match_legacy(
+            seed in proptest::collection::vec(any::<u8>(), 1..128),
+            msg in any::<[u8; 32]>()
+        ) {
+            let new_sk = Secp256k1PrivateKey::from_seed(&seed);
+            let legacy_sk = legacy::Secp256k1PrivateKey::from_seed(&seed);
+
+            let new_pk = Secp256k1PublicKey::from_private(&new_sk);
+            let legacy_pk = legacy::Secp256k1PublicKey::from_private(&legacy_sk);
+
+            let sig_new = new_sk.sign(&msg).expect("new signature");
+            let sig_legacy = legacy_sk.sign(&msg).expect("legacy signature");
+
+            prop_assert_eq!(sig_new.to_rsv(), sig_legacy.to_rsv());
+
+            prop_assert!(new_pk.verify(&msg, &sig_new).expect("new verify low-S"));
+            prop_assert!(legacy_pk.verify(&msg, &sig_legacy).expect("legacy verify low-S"));
+
+            let recovered_new = Secp256k1PublicKey::recover_to_pubkey(&msg, &sig_new)
+                .expect("new recover");
+            let recovered_legacy = legacy::Secp256k1PublicKey::recover_to_pubkey(&msg, &sig_legacy)
+                .expect("legacy recover");
+            prop_assert_eq!(recovered_new.to_hex(), recovered_legacy.to_hex());
+
+            let sig_rsv = sig_new.to_rsv();
+            let pk_bytes_new = new_pk.to_bytes_compressed();
+            let pk_bytes_legacy = legacy_pk.to_bytes();
+
+            let recover_new = secp256k1_recover(&msg, &sig_rsv);
+            let recover_legacy = legacy::secp256k1_recover(&msg, &sig_rsv);
+            prop_assert!(recover_new.is_ok());
+            prop_assert!(recover_legacy.is_ok());
+            prop_assert_eq!(recover_new.unwrap(), recover_legacy.unwrap());
+
+            let verify_new = secp256k1_verify(&msg, &sig_rsv, &pk_bytes_new);
+            let verify_legacy = legacy::secp256k1_verify(&msg, &sig_rsv, &pk_bytes_legacy);
+            prop_assert!(verify_new.is_ok());
+            prop_assert!(verify_legacy.is_ok());
+        }
+
+        #[test]
+        fn prop_high_s_paths_match_legacy(
+            seed in proptest::collection::vec(any::<u8>(), 1..128),
+            msg in any::<[u8; 32]>()
+        ) {
+            let new_sk = Secp256k1PrivateKey::from_seed(&seed);
+            let legacy_sk = legacy::Secp256k1PrivateKey::from_seed(&seed);
+
+            let new_pk = Secp256k1PublicKey::from_private(&new_sk);
+            let legacy_pk = legacy::Secp256k1PublicKey::from_private(&legacy_sk);
+
+            let sig_new = new_sk.sign(&msg).expect("new signature");
+
+            let high_s_rsv = to_high_s_rsv(&sig_new.to_rsv());
+            prop_assert!(is_high_s_signature(&high_s_rsv[..64]));
+
+            let mut high_s_vrs = high_s_rsv.clone();
+            high_s_vrs.rotate_right(1);
+
+            let high_s_new = MessageSignature::from_raw(&high_s_vrs);
+            let high_s_legacy = legacy::MessageSignature::from_raw(&high_s_vrs);
+
+            let verify_new = new_pk.verify(&msg, &high_s_new);
+            let verify_legacy = legacy_pk.verify(&msg, &high_s_legacy);
+            prop_assert_eq!(
+                verify_new.map_err(|_| ()),
+                verify_legacy.map_err(|_| ())
+            );
+            if let Ok(valid) = verify_new {
+                prop_assert!(!valid);
+            }
+
+            let recovered_new = Secp256k1PublicKey::recover_to_pubkey(&msg, &high_s_new)
+                .map(|pk| pk.to_hex())
+                .map_err(|_| ());
+            let recovered_legacy =
+                legacy::Secp256k1PublicKey::recover_to_pubkey(&msg, &high_s_legacy)
+                    .map(|pk| pk.to_hex())
+                    .map_err(|_| ());
+            prop_assert_eq!(recovered_new, recovered_legacy);
+
+            let recover_new = secp256k1_recover(&msg, &high_s_rsv).map_err(|_| ());
+            let recover_legacy = legacy::secp256k1_recover(&msg, &high_s_rsv).map_err(|_| ());
+            prop_assert_eq!(recover_new, recover_legacy);
+
+            let verify_new = secp256k1_verify(&msg, &high_s_rsv, &new_pk.to_bytes_compressed());
+            let verify_legacy = legacy::secp256k1_verify(&msg, &high_s_rsv, &legacy_pk.to_bytes());
+            prop_assert_eq!(verify_new.is_ok(), verify_legacy.is_ok());
+        }
+    }
 }