Skip to content

Commit 5aad41e

Browse files
webmaster128webmaster128
authored
Merge pull request #2057 from CosmWasm/secp256r1-recoverid23
Allow recover ID 2 and 3 for secp256r1_recover_pubkey
2 parents 11cc041 + 3a99dee commit 5aad41e

File tree

4 files changed

+67
-49
lines changed

4 files changed

+67
-49
lines changed

‎CHANGELOG.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,10 @@ and this project adheres to
99
### Added
1010

1111
- cosmwasm-vm: Add `secp256r1_verify` and `secp256r1_recover_pubkey` imports for
12-
ECDSA signature verification over secp256r1. ([#1983])
12+
ECDSA signature verification over secp256r1. ([#1983], [#2057])
1313

1414
[#1983]: https://github.com/CosmWasm/cosmwasm/pull/1983
15+
[#2057]: https://github.com/CosmWasm/cosmwasm/pull/2057
1516

1617
### Changed
1718

‎packages/crypto/src/secp256r1.rs

Lines changed: 6 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -53,31 +53,17 @@ pub fn secp256r1_verify(
5353

5454
/// Recovers a public key from a message hash and a signature.
5555
///
56-
/// This is required when working with Ethereum where public keys
57-
/// are not stored on chain directly.
56+
/// This is required when working with an application where public keys
57+
/// are not stored directly.
5858
///
59-
/// `recovery_param` must be 0 or 1. The values 2 and 3 are unsupported by this implementation,
60-
/// which is the same restriction as Ethereum has (https://github.com/ethereum/go-ethereum/blob/v1.9.25/internal/ethapi/api.go#L466-L469).
61-
/// All other values are invalid.
59+
/// `recovery_param` must be 0, 1, 2 or 3.
6260
///
6361
/// Returns the recovered pubkey in compressed form, which can be used
6462
/// in secp256r1_verify directly.
6563
///
6664
/// This implementation accepts both high-S and low-S signatures. This is the
6765
/// same behavior as Ethereum's `ecrecover`. The reason is that high-S signatures
6866
/// may be perfectly valid if the application protocol does not disallow them.
69-
/// Or as [EIP-2] put it "The ECDSA recover precompiled contract remains unchanged
70-
/// and will keep accepting high s-values; this is useful e.g. if a contract
71-
/// recovers old Bitcoin signatures.".
72-
///
73-
/// See also OpenZeppelin's [ECDSA.recover implementation](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.1/contracts/utils/cryptography/ECDSA.sol#L138-L149)
74-
/// which adds further restrictions to avoid potential signature malleability.
75-
/// Please note that restricting signatures to low-S does not make signatures unique
76-
/// in the sense that for each (pubkey, message) there is only one signature. The
77-
/// signer can generate an arbitrary amount of valid signatures.
78-
/// <https://medium.com/@simonwarta/signature-determinism-for-blockchain-developers-dbd84865a93e>
79-
///
80-
/// [EIP-2]: https://eips.ethereum.org/EIPS/eip-2
8167
pub fn secp256r1_recover_pubkey(
8268
message_hash: &[u8],
8369
signature: &[u8],
@@ -86,11 +72,9 @@ pub fn secp256r1_recover_pubkey(
8672
let message_hash = read_hash(message_hash)?;
8773
let signature = read_signature(signature)?;
8874

89-
// params other than 0 and 1 are explicitly not supported
90-
let id = match recovery_param {
91-
0 => RecoveryId::new(false, false),
92-
1 => RecoveryId::new(true, false),
93-
_ => return Err(CryptoError::invalid_recovery_param()),
75+
// params other than 0, 1, 2 or 3 are explicitly not supported
76+
let Some(id) = RecoveryId::from_byte(recovery_param) else {
77+
return Err(CryptoError::invalid_recovery_param());
9478
};
9579

9680
// Compose extended signature
@@ -344,19 +328,6 @@ mod tests {
344328
let r_s = hex::decode(COSMOS_SECP256R1_SIGNATURE_HEX1).unwrap();
345329
let message_hash = Sha256::digest(hex::decode(COSMOS_SECP256R1_MSG_HEX1).unwrap());
346330

347-
// 2 and 3 are explicitly unsupported
348-
let recovery_param: u8 = 2;
349-
match secp256r1_recover_pubkey(&message_hash, &r_s, recovery_param).unwrap_err() {
350-
CryptoError::InvalidRecoveryParam { .. } => {}
351-
err => panic!("Unexpected error: {err}"),
352-
}
353-
let recovery_param: u8 = 3;
354-
match secp256r1_recover_pubkey(&message_hash, &r_s, recovery_param).unwrap_err() {
355-
CryptoError::InvalidRecoveryParam { .. } => {}
356-
err => panic!("Unexpected error: {err}"),
357-
}
358-
359-
// Other values are garbage
360331
let recovery_param: u8 = 4;
361332
match secp256r1_recover_pubkey(&message_hash, &r_s, recovery_param).unwrap_err() {
362333
CryptoError::InvalidRecoveryParam { .. } => {}

‎packages/crypto/tests/wycheproof_secp256k1.rs

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,12 @@ fn ecdsa_secp256k1_sha256() {
102102
let signature = from_der(&der_signature).unwrap();
103103
let valid = secp256k1_verify(&message_hash, &signature, &public_key).unwrap();
104104
assert!(valid);
105-
if tc.comment != "k*G has a large x-coordinate" {
106-
test_secp256k1_recover_pubkey(&message_hash, &signature, &public_key);
105+
if tc.comment == "k*G has a large x-coordinate" {
106+
// This case (recovery ID 2 and 3) was never supported in the implementation of
107+
// secp256k1_recover_pubkey because the library we used at that time did not support it.
108+
// If needed, we could enable it now in a consensus breaking change.
109+
} else {
110+
test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
107111
}
108112
}
109113
"invalid" => {
@@ -150,8 +154,12 @@ fn ecdsa_secp256k1_sha512() {
150154
let signature = from_der(&der_signature).unwrap();
151155
let valid = secp256k1_verify(&message_hash, &signature, &public_key).unwrap();
152156
assert!(valid);
153-
if tc.comment != "k*G has a large x-coordinate" {
154-
test_secp256k1_recover_pubkey(&message_hash, &signature, &public_key);
157+
if tc.comment == "k*G has a large x-coordinate" {
158+
// This case (recovery ID 2 and 3) was never supported in the implementation of
159+
// secp256k1_recover_pubkey because the library we used at that time did not support it.
160+
// If needed, we could enable it now in a consensus breaking change.
161+
} else {
162+
test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
155163
}
156164
}
157165
"invalid" => {
@@ -198,8 +206,12 @@ fn ecdsa_secp256k1_sha3_256() {
198206
let signature = from_der(&der_signature).unwrap();
199207
let valid = secp256k1_verify(&message_hash, &signature, &public_key).unwrap();
200208
assert!(valid);
201-
if tc.comment != "k*G has a large x-coordinate" {
202-
test_secp256k1_recover_pubkey(&message_hash, &signature, &public_key);
209+
if tc.comment == "k*G has a large x-coordinate" {
210+
// This case (recovery ID 2 and 3) was never supported in the implementation of
211+
// secp256k1_recover_pubkey because the library we used at that time did not support it.
212+
// If needed, we could enable it now in a consensus breaking change.
213+
} else {
214+
test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
203215
}
204216
}
205217
"invalid" => {
@@ -246,8 +258,12 @@ fn ecdsa_secp256k1_sha3_512() {
246258
let signature = from_der(&der_signature).unwrap();
247259
let valid = secp256k1_verify(&message_hash, &signature, &public_key).unwrap();
248260
assert!(valid);
249-
if tc.comment != "k*G has a large x-coordinate" {
250-
test_secp256k1_recover_pubkey(&message_hash, &signature, &public_key);
261+
if tc.comment == "k*G has a large x-coordinate" {
262+
// This case (recovery ID 2 and 3) was never supported in the implementation of
263+
// secp256k1_recover_pubkey because the library we used at that time did not support it.
264+
// If needed, we could enable it now in a consensus breaking change.
265+
} else {
266+
test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
251267
}
252268
}
253269
"invalid" => {
@@ -268,10 +284,10 @@ fn ecdsa_secp256k1_sha3_512() {
268284
assert_eq!(tested, number_of_tests);
269285
}
270286

271-
fn test_secp256k1_recover_pubkey(message_hash: &[u8], signature: &[u8], public_key: &[u8]) {
272-
// Since the recovery param is missing in the test vectors, we try both 0 and 1
273-
let recovered0 = secp256k1_recover_pubkey(message_hash, signature, 0).unwrap();
274-
let recovered1 = secp256k1_recover_pubkey(message_hash, signature, 1).unwrap();
287+
fn test_recover_pubkey(message_hash: &[u8], signature: &[u8], public_key: &[u8], params: [u8; 2]) {
288+
// Since the recovery param is missing in the test vectors, we try both
289+
let recovered0 = secp256k1_recover_pubkey(message_hash, signature, params[0]).unwrap();
290+
let recovered1 = secp256k1_recover_pubkey(message_hash, signature, params[1]).unwrap();
275291
// Got two different pubkeys. Without the recovery param, we don't know which one is the right one.
276292
assert_ne!(recovered0, recovered1);
277293
assert!(recovered0 == public_key || recovered1 == public_key);

‎packages/crypto/tests/wycheproof_secp256r1.rs

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
#![allow(clippy::single_match)]
22

3-
use cosmwasm_crypto::secp256r1_verify;
3+
use cosmwasm_crypto::{secp256r1_recover_pubkey, secp256r1_verify};
44
use serde::Deserialize;
55

66
// See ./testdata/wycheproof/README.md for how to get/update those files
@@ -33,6 +33,7 @@ struct Key {
3333
#[serde(rename_all = "camelCase")]
3434
struct TestCase {
3535
tc_id: u32,
36+
comment: String,
3637
msg: String,
3738
sig: String,
3839
// "acceptable", "valid" or "invalid"
@@ -100,6 +101,11 @@ fn ecdsa_secp256r1_sha256() {
100101
let signature = from_der(&der_signature).unwrap();
101102
let valid = secp256r1_verify(&message_hash, &signature, &public_key).unwrap();
102103
assert!(valid);
104+
if tc.comment == "k*G has a large x-coordinate" {
105+
test_recover_pubkey(&message_hash, &signature, &public_key, [2, 3]);
106+
} else {
107+
test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
108+
}
103109
}
104110
"invalid" => {
105111
let message = hex::decode(tc.msg).unwrap();
@@ -148,6 +154,11 @@ fn ecdsa_secp256r1_sha512() {
148154
let signature = from_der(&der_signature).unwrap();
149155
let valid = secp256r1_verify(&message_hash, &signature, &public_key).unwrap();
150156
assert!(valid);
157+
if tc.comment == "k*G has a large x-coordinate" {
158+
test_recover_pubkey(&message_hash, &signature, &public_key, [2, 3]);
159+
} else {
160+
test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
161+
}
151162
}
152163
"invalid" => {
153164
let message = hex::decode(tc.msg).unwrap();
@@ -196,6 +207,11 @@ fn ecdsa_secp256r1_sha3_256() {
196207
let signature = from_der(&der_signature).unwrap();
197208
let valid = secp256r1_verify(&message_hash, &signature, &public_key).unwrap();
198209
assert!(valid);
210+
if tc.comment == "k*G has a large x-coordinate" {
211+
test_recover_pubkey(&message_hash, &signature, &public_key, [2, 3]);
212+
} else {
213+
test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
214+
}
199215
}
200216
"invalid" => {
201217
let message = hex::decode(tc.msg).unwrap();
@@ -244,6 +260,11 @@ fn ecdsa_secp256r1_sha3_512() {
244260
let signature = from_der(&der_signature).unwrap();
245261
let valid = secp256r1_verify(&message_hash, &signature, &public_key).unwrap();
246262
assert!(valid);
263+
if tc.comment == "k*G has a large x-coordinate" {
264+
test_recover_pubkey(&message_hash, &signature, &public_key, [2, 3]);
265+
} else {
266+
test_recover_pubkey(&message_hash, &signature, &public_key, [0, 1]);
267+
}
247268
}
248269
"invalid" => {
249270
let message = hex::decode(tc.msg).unwrap();
@@ -267,6 +288,15 @@ fn ecdsa_secp256r1_sha3_512() {
267288
assert_eq!(tested, number_of_tests);
268289
}
269290

291+
fn test_recover_pubkey(message_hash: &[u8], signature: &[u8], public_key: &[u8], params: [u8; 2]) {
292+
// Since the recovery param is missing in the test vectors, we try both
293+
let recovered0 = secp256r1_recover_pubkey(message_hash, signature, params[0]).unwrap();
294+
let recovered1 = secp256r1_recover_pubkey(message_hash, signature, params[1]).unwrap();
295+
// Got two different pubkeys. Without the recovery param, we don't know which one is the right one.
296+
assert_ne!(recovered0, recovered1);
297+
assert!(recovered0 == public_key || recovered1 == public_key);
298+
}
299+
270300
fn from_der(data: &[u8]) -> Result<[u8; 64], String> {
271301
const DER_TAG_INTEGER: u8 = 0x02;
272302

0 commit comments

Comments
 (0)