Repository: CosmWasm/cosmwasm
Pull request #2057 (2024-03-15 16:20:29)

Allow recover ID 2 and 3 for secp256r1_recover_pubkey
For secp256k1 it seems to very unlikely that recovery param 2 and 3 exists in practive. This is why Ethereum and an old version of k256 disallowed them. However, now k256 has support for 2 and 3 and I don't see a reason to not support them. For secp256r1 I don't know if they have the same super low probability. So I would say the newly introduced secp256r1 verifier coming with CosmWasm 2.1 gets support for all 0/1/2/3 right away and we keep secp256k1 as is for now.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3199efaad..df522c5d7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,9 +9,10 @@ and this project adheres to
 ### Added
 
 - cosmwasm-vm: Add `secp256r1_verify` and `secp256r1_recover_pubkey` imports for
-  ECDSA signature verification over secp256r1. ([#1983])
+  ECDSA signature verification over secp256r1. ([#1983], [#2057])
 
 [#1983]: https://github.com/CosmWasm/cosmwasm/pull/1983
+[#2057]: https://github.com/CosmWasm/cosmwasm/pull/2057
 
 ### Changed
 
diff --git a/packages/crypto/src/secp256r1.rs b/packages/crypto/src/secp256r1.rs
index 2f4829d76..0a0256d69 100644
--- a/packages/crypto/src/secp256r1.rs
+++ b/packages/crypto/src/secp256r1.rs
@@ -53,12 +53,10 @@ pub fn secp256r1_verify(
 
 /// Recovers a public key from a message hash and a signature.
 ///
-/// This is required when working with Ethereum where public keys
-/// are not stored on chain directly.
+/// This is required when working with an application where public keys
+/// are not stored directly.
 ///
-/// `recovery_param` must be 0 or 1. The values 2 and 3 are unsupported by this implementation,
-/// which is the same restriction as Ethereum has (https://github.com/ethereum/go-ethereum/blob/v1.9.25/internal/ethapi/api.go#L466-L469).
-/// All other values are invalid.
+/// `recovery_param` must be 0, 1, 2 or 3.
 ///
 /// Returns the recovered pubkey in compressed form, which can be used
 /// in secp256r1_verify directly.
@@ -66,18 +64,6 @@ pub fn secp256r1_verify(
 /// This implementation accepts both high-S and low-S signatures. This is the
 /// same behavior as Ethereum's `ecrecover`. The reason is that high-S signatures
 /// may be perfectly valid if the application protocol does not disallow them.
-/// Or as [EIP-2] put it "The ECDSA recover precompiled contract remains unchanged
-/// and will keep accepting high s-values; this is useful e.g. if a contract
-/// recovers old Bitcoin signatures.".
-///
-/// See also OpenZeppelin's [ECDSA.recover implementation](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.1/contracts/utils/cryptography/ECDSA.sol#L138-L149)
-/// which adds further restrictions to avoid potential signature malleability.
-/// Please note that restricting signatures to low-S does not make signatures unique
-/// in the sense that for each (pubkey, message) there is only one signature. The
-/// signer can generate an arbitrary amount of valid signatures.
-/// <https://medium.com/@simonwarta/signature-determinism-for-blockchain-developers-dbd84865a93e>
-///
-/// [EIP-2]: https://eips.ethereum.org/EIPS/eip-2
 pub fn secp256r1_recover_pubkey(
     message_hash: &[u8],
     signature: &[u8],
@@ -86,11 +72,9 @@ pub fn secp256r1_recover_pubkey(
     let message_hash = read_hash(message_hash)?;
     let signature = read_signature(signature)?;
 
-    // params other than 0 and 1 are explicitly not supported
-    let id = match recovery_param {
-        0 => RecoveryId::new(false, false),
-        1 => RecoveryId::new(true, false),
-        _ => return Err(CryptoError::invalid_recovery_param()),
+    // params other than 0, 1, 2 or 3 are explicitly not supported
+    let Some(id) = RecoveryId::from_byte(recovery_param) else {
+        return Err(CryptoError::invalid_recovery_param());
     };
 
     // Compose extended signature
@@ -344,19 +328,6 @@ mod tests {
         let r_s = hex::decode(COSMOS_SECP256R1_SIGNATURE_HEX1).unwrap();
         let message_hash = Sha256::digest(hex::decode(COSMOS_SECP256R1_MSG_HEX1).unwrap());
 
-        // 2 and 3 are explicitly unsupported
-        let recovery_param: u8 = 2;
-        match secp256r1_recover_pubkey(&message_hash, &r_s, recovery_param).unwrap_err() {
-            CryptoError::InvalidRecoveryParam { .. } => {}
-            err => panic!("Unexpected error: {err}"),
-        }
-        let recovery_param: u8 = 3;
-        match secp256r1_recover_pubkey(&message_hash, &r_s, recovery_param).unwrap_err() {
-            CryptoError::InvalidRecoveryParam { .. } => {}
-            err => panic!("Unexpected error: {err}"),
-        }
-
-        // Other values are garbage
         let recovery_param: u8 = 4;
         match secp256r1_recover_pubkey(&message_hash, &r_s, recovery_param).unwrap_err() {
             CryptoError::InvalidRecoveryParam { .. } => {}


Test patch:
diff --git a/packages/crypto/tests/wycheproof_secp256k1.rs b/packages/crypto/tests/wycheproof_secp256k1.rs
index 931761f54..e382252fb 100644
--- a/packages/crypto/tests/wycheproof_secp256k1.rs
+++ b/packages/crypto/tests/wycheproof_secp256k1.rs
@@ -102,8 +102,12 @@ fn ecdsa_secp256k1_sha256() {
                     let signature = from_der(&der_signature).unwrap();
                     let valid = secp256k1_verify(&message_hash, &signature, &public_key).unwrap();
                     assert!(valid);
-                    if tc.comment != "k*G has a large x-coordinate" {
-                        test_secp256k1_recover_pubkey(&message_hash, &signature, &public_key);
+                    if tc.comment == "k*G has a large x-coordinate" {
+                        // This case (recovery ID 2 and 3) was never supported in the implementation of
+                        // secp256k1_recover_pubkey because the library we used at that time did not support it.
+                        // If needed, we could enable it now in a consensus breaking change.
+                    } else {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
                     }
                 }
                 "invalid" => {
@@ -150,8 +154,12 @@ fn ecdsa_secp256k1_sha512() {
                     let signature = from_der(&der_signature).unwrap();
                     let valid = secp256k1_verify(&message_hash, &signature, &public_key).unwrap();
                     assert!(valid);
-                    if tc.comment != "k*G has a large x-coordinate" {
-                        test_secp256k1_recover_pubkey(&message_hash, &signature, &public_key);
+                    if tc.comment == "k*G has a large x-coordinate" {
+                        // This case (recovery ID 2 and 3) was never supported in the implementation of
+                        // secp256k1_recover_pubkey because the library we used at that time did not support it.
+                        // If needed, we could enable it now in a consensus breaking change.
+                    } else {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
                     }
                 }
                 "invalid" => {
@@ -198,8 +206,12 @@ fn ecdsa_secp256k1_sha3_256() {
                     let signature = from_der(&der_signature).unwrap();
                     let valid = secp256k1_verify(&message_hash, &signature, &public_key).unwrap();
                     assert!(valid);
-                    if tc.comment != "k*G has a large x-coordinate" {
-                        test_secp256k1_recover_pubkey(&message_hash, &signature, &public_key);
+                    if tc.comment == "k*G has a large x-coordinate" {
+                        // This case (recovery ID 2 and 3) was never supported in the implementation of
+                        // secp256k1_recover_pubkey because the library we used at that time did not support it.
+                        // If needed, we could enable it now in a consensus breaking change.
+                    } else {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
                     }
                 }
                 "invalid" => {
@@ -246,8 +258,12 @@ fn ecdsa_secp256k1_sha3_512() {
                     let signature = from_der(&der_signature).unwrap();
                     let valid = secp256k1_verify(&message_hash, &signature, &public_key).unwrap();
                     assert!(valid);
-                    if tc.comment != "k*G has a large x-coordinate" {
-                        test_secp256k1_recover_pubkey(&message_hash, &signature, &public_key);
+                    if tc.comment == "k*G has a large x-coordinate" {
+                        // This case (recovery ID 2 and 3) was never supported in the implementation of
+                        // secp256k1_recover_pubkey because the library we used at that time did not support it.
+                        // If needed, we could enable it now in a consensus breaking change.
+                    } else {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
                     }
                 }
                 "invalid" => {
@@ -268,10 +284,10 @@ fn ecdsa_secp256k1_sha3_512() {
     assert_eq!(tested, number_of_tests);
 }
 
-fn test_secp256k1_recover_pubkey(message_hash: &[u8], signature: &[u8], public_key: &[u8]) {
-    // Since the recovery param is missing in the test vectors, we try both 0 and 1
-    let recovered0 = secp256k1_recover_pubkey(message_hash, signature, 0).unwrap();
-    let recovered1 = secp256k1_recover_pubkey(message_hash, signature, 1).unwrap();
+fn test_recover_pubkey(message_hash: &[u8], signature: &[u8], public_key: &[u8], params: [u8; 2]) {
+    // Since the recovery param is missing in the test vectors, we try both
+    let recovered0 = secp256k1_recover_pubkey(message_hash, signature, params[0]).unwrap();
+    let recovered1 = secp256k1_recover_pubkey(message_hash, signature, params[1]).unwrap();
     // Got two different pubkeys. Without the recovery param, we don't know which one is the right one.
     assert_ne!(recovered0, recovered1);
     assert!(recovered0 == public_key || recovered1 == public_key);
diff --git a/packages/crypto/tests/wycheproof_secp256r1.rs b/packages/crypto/tests/wycheproof_secp256r1.rs
index 200303574..9464e3235 100644
--- a/packages/crypto/tests/wycheproof_secp256r1.rs
+++ b/packages/crypto/tests/wycheproof_secp256r1.rs
@@ -1,6 +1,6 @@
 #![allow(clippy::single_match)]
 
-use cosmwasm_crypto::secp256r1_verify;
+use cosmwasm_crypto::{secp256r1_recover_pubkey, secp256r1_verify};
 use serde::Deserialize;
 
 // See ./testdata/wycheproof/README.md for how to get/update those files
@@ -33,6 +33,7 @@ struct Key {
 #[serde(rename_all = "camelCase")]
 struct TestCase {
     tc_id: u32,
+    comment: String,
     msg: String,
     sig: String,
     // "acceptable", "valid" or "invalid"
@@ -100,6 +101,11 @@ fn ecdsa_secp256r1_sha256() {
                     let signature = from_der(&der_signature).unwrap();
                     let valid = secp256r1_verify(&message_hash, &signature, &public_key).unwrap();
                     assert!(valid);
+                    if tc.comment == "k*G has a large x-coordinate" {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [2, 3]);
+                    } else {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
+                    }
                 }
                 "invalid" => {
                     let message = hex::decode(tc.msg).unwrap();
@@ -148,6 +154,11 @@ fn ecdsa_secp256r1_sha512() {
                     let signature = from_der(&der_signature).unwrap();
                     let valid = secp256r1_verify(&message_hash, &signature, &public_key).unwrap();
                     assert!(valid);
+                    if tc.comment == "k*G has a large x-coordinate" {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [2, 3]);
+                    } else {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
+                    }
                 }
                 "invalid" => {
                     let message = hex::decode(tc.msg).unwrap();
@@ -196,6 +207,11 @@ fn ecdsa_secp256r1_sha3_256() {
                     let signature = from_der(&der_signature).unwrap();
                     let valid = secp256r1_verify(&message_hash, &signature, &public_key).unwrap();
                     assert!(valid);
+                    if tc.comment == "k*G has a large x-coordinate" {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [2, 3]);
+                    } else {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
+                    }
                 }
                 "invalid" => {
                     let message = hex::decode(tc.msg).unwrap();
@@ -244,6 +260,11 @@ fn ecdsa_secp256r1_sha3_512() {
                     let signature = from_der(&der_signature).unwrap();
                     let valid = secp256r1_verify(&message_hash, &signature, &public_key).unwrap();
                     assert!(valid);
+                    if tc.comment == "k*G has a large x-coordinate" {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [2, 3]);
+                    } else {
+                        test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
+                    }
                 }
                 "invalid" => {
                     let message = hex::decode(tc.msg).unwrap();
@@ -267,6 +288,15 @@ fn ecdsa_secp256r1_sha3_512() {
     assert_eq!(tested, number_of_tests);
 }
 
+fn test_recover_pubkey(message_hash: &[u8], signature: &[u8], public_key: &[u8], params: [u8; 2]) {
+    // Since the recovery param is missing in the test vectors, we try both
+    let recovered0 = secp256r1_recover_pubkey(message_hash, signature, params[0]).unwrap();
+    let recovered1 = secp256r1_recover_pubkey(message_hash, signature, params[1]).unwrap();
+    // Got two different pubkeys. Without the recovery param, we don't know which one is the right one.
+    assert_ne!(recovered0, recovered1);
+    assert!(recovered0 == public_key || recovered1 == public_key);
+}
+
 fn from_der(data: &[u8]) -> Result<[u8; 64], String> {
     const DER_TAG_INTEGER: u8 = 0x02;