k256: v0.13.2 breaking verification about elliptic-curves HOT 9 CLOSED

@webmaster128 yep, that looks fine for now and can be removed in the next release

from elliptic-curves.

This was actually a bugfix: #675 accidentally removed checks for low-S normalization which was released as k256 v0.12, which was reported in #908, as well as a recent NCC audit here: https://research.nccgroup.com/2023/08/30/public-report-entropy-rust-cryptography-review/

If you would like for the non-normalized signatures to verify, you need to normalize them first with Signature::normalize_s

from elliptic-curves.

In order to accept both low and high S, is it safe to do Signature::normalize_s also in VerifyingKey::recover_from_digest? If I recall correctly, low S and high S retruned a different pubkey before.

from elliptic-curves.

@webmaster128 you'll also need to flip the lowest bit in the recovery ID if sig.s.is_high()

Also hopefully recovery now accepts non-normalized signatures as of this PR: RustCrypto/signatures#793

However you'd have to try k256 v0.14.0-pre as sourced from git.

from elliptic-curves.

Oh sweet, thanks a lot! Just to double check: this diff correctly restores the 0.13.1 behaviour for recover pubkey and from 0.14 onwards I can remove it again?

--- a/packages/crypto/src/secp256k1.rs
+++ b/packages/crypto/src/secp256k1.rs
@@ -105,18 +105,24 @@ pub fn secp256k1_recover_pubkey(
     let signature = read_signature(signature)?;
 
     // params other than 0 and 1 are explicitly not supported
-    let id = match recovery_param {
+    let mut id = match recovery_param {
         0 => RecoveryId::new(false, false),
         1 => RecoveryId::new(true, false),
         _ => return Err(CryptoError::invalid_recovery_param()),
     };
 
     // Compose extended signature
-    let signature = Signature::from_bytes(&signature.into())
+    let mut signature = Signature::from_bytes(&signature.into())
         .map_err(|e| CryptoError::generic_err(e.to_string()))?;
 
     // Recover
     let message_digest = Identity256::new().chain(message_hash);
+
+    if let Some(normalized) = signature.normalize_s() {
+        signature = normalized;
+        id = RecoveryId::new(!id.is_y_odd(), id.is_x_reduced());
+    }
+
     let pubkey = VerifyingKey::recover_from_digest(message_digest, &signature, id)
         .map_err(|e| CryptoError::generic_err(e.to_string()))?;
     let encoded: Vec<u8> = pubkey.to_encoded_point(false).as_bytes().into();

At least tests seem to be passing with this.

from elliptic-curves.

I tried using the current version from git, but it fails to compile in sha2 with 0.11.0-pre.2:

error[E0432]: unresolved import `digest::array::ArrayOps`
 --> /[...]/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sha2-0.11.0-pre.2/src/core_api.rs:4:5
  |
4 |     array::ArrayOps,
  |     ^^^^^^^^^^^^^^^ no `ArrayOps` in the root

However, the above diff passes all tests I have currently available using k256 version 0.13.3: CosmWasm/cosmwasm@818ec58

from elliptic-curves.

@webmaster128 I'll try to get another release out which fixes that, but in the meantime you can do the following:

[patch.crates-io.sha2]
git = "https://github.com/RustCrypto/hashes.git"

from elliptic-curves.

The version of k256 in git has been updated if you have the opportunity to test it out

from elliptic-curves.

Very nice, thanks! I got it working in two ways now:

1. k256 0.14 pre-release requires no change to the VerifyingKey::recover_from_digest call: CosmWasm/cosmwasm#2015
2. k256 ^0.13.3 works with by normalizing signatures and flipping is_y_odd (same as #991 (comment)): CosmWasm/cosmwasm#2014

Could you confirm the solution in 2. is correct? It passes tests but my math is not strong enough to ensure it's the right thing to do. Would be great to have something before 0.14.0 is out that does not require us to pin k256 to 0.13.1.

from elliptic-curves.