Skip to content

Commit

Permalink
[fix] #4155: ensure the secp256k1 signatures coming out of OpenSSL ar…
Browse files Browse the repository at this point in the history 
…e normalized

This is an interoperability problem between OpenSSL and newer cryptographic libraries, see [https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki#user-content-Low_S_values_in_signatures)[BIP-0062] for details

This fixes the flaky secp256k1_sign test

Also includes a drive-by cleanup of useless `assert(result.is_ok())`

Signed-off-by: Nikita Strygin <[email protected]>
  • Loading branch information
@DCNick3
DCNick3 committed Dec 25, 2023
1 parent ba01819 commit 4091454
Showing 1 changed file with 48 additions and 36 deletions.
84 changes: 48 additions & 36 deletions crypto/src/signature/secp256k1.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,8 @@ mod test {
#[test]
fn secp256k1_load_keys() {
let secret = PrivateKey::from_hex(ALGORITHM, PRIVATE_KEY).unwrap();
let sres = EcdsaSecp256k1Sha256::keypair(Some(KeyGenOption::FromPrivateKey(secret)));
assert!(sres.is_ok());
let _sres =
EcdsaSecp256k1Sha256::keypair(Some(KeyGenOption::FromPrivateKey(secret))).unwrap();
}

#[test]
Expand All @@ -158,16 +158,14 @@ mod test {
let (p, s) =
EcdsaSecp256k1Sha256::keypair(Some(KeyGenOption::FromPrivateKey(secret))).unwrap();

let sk = secp256k1::SecretKey::from_slice(s.payload());
assert!(sk.is_ok());
let pk = secp256k1::PublicKey::from_slice(p.payload());
assert!(pk.is_ok());
let _sk = secp256k1::SecretKey::from_slice(s.payload()).unwrap();
let _pk = secp256k1::PublicKey::from_slice(p.payload()).unwrap();

let openssl_group = EcGroup::from_curve_name(Nid::SECP256K1).unwrap();
let mut ctx = BigNumContext::new().unwrap();
let openssl_point =
EcPoint::from_bytes(&openssl_group, &public_key_uncompressed(&p)[..], &mut ctx);
assert!(openssl_point.is_ok());
let _openssl_point =
EcPoint::from_bytes(&openssl_group, &public_key_uncompressed(&p)[..], &mut ctx)
.unwrap();
}

#[test]
Expand All @@ -179,7 +177,8 @@ mod test {
hex::decode(SIGNATURE_1).unwrap().as_slice(),
&p,
);
assert!(result.is_ok());
// we are returning a `Result<bool>`
// unwrap will catch the `Err(_)`, and assert will catch the `false`
assert!(result.unwrap());

let context = secp256k1::Secp256k1::new();
Expand All @@ -189,13 +188,11 @@ mod test {
let h = sha2::Sha256::digest(MESSAGE_1);
let msg = secp256k1::Message::from_digest_slice(h.as_slice()).unwrap();

//Check if signatures produced here can be verified by secp256k1
let mut signature =
// Check if signatures produced here can be verified by secp256k1
let signature =
secp256k1::ecdsa::Signature::from_compact(&hex::decode(SIGNATURE_1).unwrap()[..])
.unwrap();
signature.normalize_s();
let result = context.verify_ecdsa(&msg, &signature, &pk);
assert!(result.is_ok());
context.verify_ecdsa(&msg, &signature, &pk).unwrap();

let openssl_group = EcGroup::from_curve_name(Nid::SECP256K1).unwrap();
let mut ctx = BigNumContext::new().unwrap();
Expand All @@ -209,19 +206,19 @@ mod test {
let openssl_s = BigNum::from_hex_str(s).unwrap();
let openssl_sig = EcdsaSig::from_private_components(openssl_r, openssl_s).unwrap();
let openssl_result = openssl_sig.verify(h.as_slice(), &openssl_pkey);
assert!(openssl_result.is_ok());
assert!(openssl_result.unwrap());
}

#[test]
fn secp256k1_sign() {
let secret = PrivateKey::from_hex(ALGORITHM, PRIVATE_KEY).unwrap();
let (p, s) =
let (pk, sk) =
EcdsaSecp256k1Sha256::keypair(Some(KeyGenOption::FromPrivateKey(secret))).unwrap();

let sig = EcdsaSecp256k1Sha256::sign(MESSAGE_1, &s).unwrap();
let result = EcdsaSecp256k1Sha256::verify(MESSAGE_1, &sig, &p);
assert!(result.is_ok());
let sig = EcdsaSecp256k1Sha256::sign(MESSAGE_1, &sk).unwrap();
let result = EcdsaSecp256k1Sha256::verify(MESSAGE_1, &sig, &pk);
// we are returning a `Result<bool>`
// unwrap will catch the `Err(_)`, and assert will catch the `false`
assert!(result.unwrap());

assert_eq!(sig.len(), 64);
Expand All @@ -237,16 +234,13 @@ mod test {
let msg = secp256k1::Message::from_digest_slice(h.as_slice()).unwrap();
let sig_1 = context.sign_ecdsa(&msg, &sk).serialize_compact();

let result = EcdsaSecp256k1Sha256::verify(MESSAGE_1, &sig_1, &p);

assert!(result.is_ok());
let result = EcdsaSecp256k1Sha256::verify(MESSAGE_1, &sig_1, &pk);
assert!(result.unwrap());

let openssl_group = EcGroup::from_curve_name(Nid::SECP256K1).unwrap();
let mut ctx = BigNumContext::new().unwrap();
let openssl_point =
EcPoint::from_bytes(&openssl_group, &public_key_uncompressed(&p)[..], &mut ctx)
.unwrap();
EcPoint::from_bytes(&openssl_group, &public_key_uncompressed(&pk), &mut ctx).unwrap();
let openssl_public_key = EcKey::from_public_key(&openssl_group, &openssl_point).unwrap();
let openssl_secret_key = EcKey::from_private_components(
&openssl_group,
Expand All @@ -257,23 +251,41 @@ mod test {

let openssl_sig = EcdsaSig::sign(h.as_slice(), &openssl_secret_key).unwrap();
let openssl_result = openssl_sig.verify(h.as_slice(), &openssl_public_key);
assert!(openssl_result.is_ok());
assert!(openssl_result.unwrap());
let mut temp_sig = Vec::new();
temp_sig.extend(openssl_sig.r().to_vec());
temp_sig.extend(openssl_sig.s().to_vec());

// secp256k1 expects normalized "s"'s.
// scheme.normalize_s(temp_sig.as_mut_slice()).unwrap();
// k256 seems to be normalizing always now
let result = EcdsaSecp256k1Sha256::verify(MESSAGE_1, temp_sig.as_slice(), &p);
assert!(result.is_ok());

let openssl_sig = {
use std::ops::{Shr, Sub};

// ensure the S value is "low" (see BIP-0062) https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki#user-content-Low_S_values_in_signatures
// this is required for k256 to successfully verify the signature, as it will fail verification of any signature with a High S value
// Based on https://github.com/bitcoin/bitcoin/blob/v0.9.3/src/key.cpp#L202-L227
// this is only required for interoperability with OpenSSL
// if we are only using signatures from iroha_crypto, all of this dance is not necessary
let mut s = openssl_sig.s().to_owned().unwrap();
let mut order = BigNum::new().unwrap();
openssl_group.order(&mut order, &mut ctx).unwrap();
let half_order = order.shr(1);

// if the S is "high" (s > half_order), convert it to "low" form (order - s)
if s.cmp(&half_order) == std::cmp::Ordering::Greater {
s = order.sub(&s);
}

let r = openssl_sig.r();

// serialize the key
let mut res = Vec::new();
res.extend(r.to_vec());
res.extend(s.to_vec());
res
};

let result = EcdsaSecp256k1Sha256::verify(MESSAGE_1, openssl_sig.as_slice(), &pk);
assert!(result.unwrap());

let (p, s) = EcdsaSecp256k1Sha256::keypair(None).unwrap();
let signed = EcdsaSecp256k1Sha256::sign(MESSAGE_1, &s).unwrap();
let result = EcdsaSecp256k1Sha256::verify(MESSAGE_1, &signed, &p);
assert!(result.is_ok());
assert!(result.unwrap());
}
}

0 comments on commit 4091454

Please sign in to comment.