From 01753e42cbb23c26ef4789649f7a346ac9c42134 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Sat, 10 Jun 2023 07:09:01 +0000 Subject: [PATCH 1/9] chore: enforce `verify_prehashed` input lengths through types --- acvm/src/pwg/blackbox/ecdsa.rs | 56 +++++++++++++++++----------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/acvm/src/pwg/blackbox/ecdsa.rs b/acvm/src/pwg/blackbox/ecdsa.rs index b091137bc..bb93955c5 100644 --- a/acvm/src/pwg/blackbox/ecdsa.rs +++ b/acvm/src/pwg/blackbox/ecdsa.rs @@ -27,6 +27,17 @@ pub(crate) fn secp256k1_prehashed( hashed_message_inputs: &[FunctionInput], output: Witness, ) -> Result { + let hashed_message: [u8; 32] = + to_u8_vec(initial_witness, hashed_message_inputs)?.try_into().map_err(|_| { + OpcodeResolutionError::BlackBoxFunctionFailed( + acir::BlackBoxFunc::EcdsaSecp256k1, + format!( + "expected hashed message size 32 but received {}", + hashed_message_inputs.len() + ), + ) + })?; + let pub_key_x: [u8; 32] = to_u8_vec(initial_witness, public_key_x_inputs)?.try_into().map_err(|_| { OpcodeResolutionError::BlackBoxFunctionFailed( @@ -51,7 +62,6 @@ pub(crate) fn secp256k1_prehashed( ) })?; - let hashed_message = to_u8_vec(initial_witness, hashed_message_inputs)?; let result = ecdsa_secp256k1::verify_prehashed(&hashed_message, &pub_key_x, &pub_key_y, &signature) .is_ok(); @@ -61,10 +71,6 @@ pub(crate) fn secp256k1_prehashed( } mod ecdsa_secp256k1 { - use std::convert::TryInto; - - use blake2::digest::generic_array::GenericArray; - use k256::elliptic_curve::sec1::FromEncodedPoint; use k256::elliptic_curve::PrimeField; @@ -89,9 +95,8 @@ mod ecdsa_secp256k1 { let message = b"ECDSA proves knowledge of a secret number in the context of a single message"; - let mut hasher = Sha256::new(); - hasher.update(message); - let digest = hasher.finalize(); + let digest: [u8; 32] = + Sha256::digest(message).try_into().expect("SHA256 digest should be 256 bits"); let signature: Signature = signing_key.sign(message); // Verification @@ -101,9 +106,13 @@ mod ecdsa_secp256k1 { if let Coordinates::Uncompressed { x, y } = verify_key.to_encoded_point(false).coordinates() { - let signature_bytes: &[u8] = signature.as_ref(); - assert!(Signature::try_from(signature_bytes).unwrap() == signature); - verify_prehashed(&digest, x, y, signature_bytes).unwrap(); + let signature_bytes: [u8; 64] = signature.as_ref().try_into().unwrap(); + assert!(Signature::try_from(signature_bytes.as_slice()).unwrap() == signature); + + let x: [u8; 32] = x.clone().into(); + let y: [u8; 32] = y.clone().into(); + + verify_prehashed(&digest, &x, &y, &signature_bytes).unwrap(); } else { unreachable!(); } @@ -113,32 +122,23 @@ mod ecdsa_secp256k1 { /// Verify an ECDSA signature, given the hashed message pub(super) fn verify_prehashed( - hashed_msg: &[u8], - public_key_x_bytes: &[u8], - public_key_y_bytes: &[u8], - signature: &[u8], + hashed_msg: &[u8; 32], + public_key_x_bytes: &[u8; 32], + public_key_y_bytes: &[u8; 32], + signature: &[u8; 64], ) -> Result<(), ()> { // Convert the inputs into k256 data structures - let signature = Signature::try_from(signature).unwrap(); - - let pub_key_x_arr: [u8; 32] = { - let pub_key_x_bytes: &[u8] = public_key_x_bytes; - pub_key_x_bytes.try_into().unwrap() - }; - let pub_key_y_arr: [u8; 32] = { - let pub_key_y_bytes: &[u8] = public_key_y_bytes; - pub_key_y_bytes.try_into().unwrap() - }; + let signature = Signature::try_from(signature.as_slice()).unwrap(); let point = EncodedPoint::from_affine_coordinates( - &pub_key_x_arr.into(), - &pub_key_y_arr.into(), + public_key_x_bytes.into(), + public_key_y_bytes.into(), true, ); let pubkey = PublicKey::from_encoded_point(&point).unwrap(); - let z = Scalar::from_repr(*GenericArray::from_slice(hashed_msg)).unwrap(); + let z = Scalar::from_repr((*hashed_msg).into()).unwrap(); // Finished converting bytes into data structures From a47b03f3f43de6221c6d912a0974c203d2b44cf9 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 12 Jun 2023 04:35:08 +0000 Subject: [PATCH 2/9] chore: clippy fix --- acvm/src/pwg/blackbox/ecdsa.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/acvm/src/pwg/blackbox/ecdsa.rs b/acvm/src/pwg/blackbox/ecdsa.rs index bb93955c5..60b8ec211 100644 --- a/acvm/src/pwg/blackbox/ecdsa.rs +++ b/acvm/src/pwg/blackbox/ecdsa.rs @@ -99,6 +99,9 @@ mod ecdsa_secp256k1 { Sha256::digest(message).try_into().expect("SHA256 digest should be 256 bits"); let signature: Signature = signing_key.sign(message); + let signature_bytes: [u8; 64] = signature.as_ref().try_into().unwrap(); + assert!(Signature::try_from(signature_bytes.as_slice()).unwrap() == signature); + // Verification use k256::ecdsa::{signature::Verifier, VerifyingKey}; @@ -106,11 +109,8 @@ mod ecdsa_secp256k1 { if let Coordinates::Uncompressed { x, y } = verify_key.to_encoded_point(false).coordinates() { - let signature_bytes: [u8; 64] = signature.as_ref().try_into().unwrap(); - assert!(Signature::try_from(signature_bytes.as_slice()).unwrap() == signature); - - let x: [u8; 32] = x.clone().into(); - let y: [u8; 32] = y.clone().into(); + let x: [u8; 32] = (*x).into(); + let y: [u8; 32] = (*y).into(); verify_prehashed(&digest, &x, &y, &signature_bytes).unwrap(); } else { From 7b15522df03fc62a0b84cb0b3e8abc145b925584 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 12 Jun 2023 05:09:08 +0000 Subject: [PATCH 3/9] chore: restructure ecdsa module --- acvm/src/pwg/blackbox/ecdsa.rs | 129 +++++++++++++++++---------------- 1 file changed, 67 insertions(+), 62 deletions(-) diff --git a/acvm/src/pwg/blackbox/ecdsa.rs b/acvm/src/pwg/blackbox/ecdsa.rs index 60b8ec211..7bfba87d9 100644 --- a/acvm/src/pwg/blackbox/ecdsa.rs +++ b/acvm/src/pwg/blackbox/ecdsa.rs @@ -3,6 +3,17 @@ use acir::{ native_types::{Witness, WitnessMap}, FieldElement, }; +use k256::elliptic_curve::sec1::FromEncodedPoint; +use k256::elliptic_curve::PrimeField; + +use k256::{ecdsa::Signature, Scalar}; +use k256::{ + elliptic_curve::{ + sec1::{Coordinates, ToEncodedPoint}, + IsHigh, + }, + AffinePoint, EncodedPoint, ProjectivePoint, PublicKey, +}; use crate::{pwg::witness_to_value, pwg::OpcodeResolution, OpcodeResolutionError}; @@ -62,26 +73,67 @@ pub(crate) fn secp256k1_prehashed( ) })?; - let result = - ecdsa_secp256k1::verify_prehashed(&hashed_message, &pub_key_x, &pub_key_y, &signature) - .is_ok(); + let result = verify_prehashed(&hashed_message, &pub_key_x, &pub_key_y, &signature).is_ok(); initial_witness.insert(output, FieldElement::from(result)); Ok(OpcodeResolution::Solved) } -mod ecdsa_secp256k1 { - use k256::elliptic_curve::sec1::FromEncodedPoint; - use k256::elliptic_curve::PrimeField; - - use k256::{ecdsa::Signature, Scalar}; - use k256::{ - elliptic_curve::{ - sec1::{Coordinates, ToEncodedPoint}, - IsHigh, - }, - AffinePoint, EncodedPoint, ProjectivePoint, PublicKey, - }; +/// Verify an ECDSA signature, given the hashed message +fn verify_prehashed( + hashed_msg: &[u8; 32], + public_key_x_bytes: &[u8; 32], + public_key_y_bytes: &[u8; 32], + signature: &[u8; 64], +) -> Result<(), ()> { + // Convert the inputs into k256 data structures + + let signature = Signature::try_from(signature.as_slice()).unwrap(); + + let point = EncodedPoint::from_affine_coordinates( + public_key_x_bytes.into(), + public_key_y_bytes.into(), + true, + ); + let pubkey = PublicKey::from_encoded_point(&point).unwrap(); + + let z = Scalar::from_repr((*hashed_msg).into()).unwrap(); + + // Finished converting bytes into data structures + + let r = signature.r(); + let s = signature.s(); + + // Ensure signature is "low S" normalized ala BIP 0062 + if s.is_high().into() { + return Err(()); + } + + let s_inv = s.invert().unwrap(); + let u1 = z * s_inv; + let u2 = *r * s_inv; + + #[allow(non_snake_case)] + let R: AffinePoint = ((ProjectivePoint::GENERATOR * u1) + + (ProjectivePoint::from(*pubkey.as_affine()) * u2)) + .to_affine(); + + if let Coordinates::Uncompressed { x, y: _ } = R.to_encoded_point(false).coordinates() { + if Scalar::from_repr(*x).unwrap().eq(&r) { + return Ok(()); + } + } + Err(()) +} + +#[cfg(test)] +mod test { + + use k256::ecdsa::Signature; + use k256::elliptic_curve::sec1::{Coordinates, ToEncodedPoint}; + + use super::verify_prehashed; + // This method is used to generate test vectors // in noir. TODO: check that it is indeed used #[allow(dead_code)] @@ -120,53 +172,6 @@ mod ecdsa_secp256k1 { assert!(verify_key.verify(message, &signature).is_ok()); } - /// Verify an ECDSA signature, given the hashed message - pub(super) fn verify_prehashed( - hashed_msg: &[u8; 32], - public_key_x_bytes: &[u8; 32], - public_key_y_bytes: &[u8; 32], - signature: &[u8; 64], - ) -> Result<(), ()> { - // Convert the inputs into k256 data structures - - let signature = Signature::try_from(signature.as_slice()).unwrap(); - - let point = EncodedPoint::from_affine_coordinates( - public_key_x_bytes.into(), - public_key_y_bytes.into(), - true, - ); - let pubkey = PublicKey::from_encoded_point(&point).unwrap(); - - let z = Scalar::from_repr((*hashed_msg).into()).unwrap(); - - // Finished converting bytes into data structures - - let r = signature.r(); - let s = signature.s(); - - // Ensure signature is "low S" normalized ala BIP 0062 - if s.is_high().into() { - return Err(()); - } - - let s_inv = s.invert().unwrap(); - let u1 = z * s_inv; - let u2 = *r * s_inv; - - #[allow(non_snake_case)] - let R: AffinePoint = ((ProjectivePoint::GENERATOR * u1) - + (ProjectivePoint::from(*pubkey.as_affine()) * u2)) - .to_affine(); - - if let Coordinates::Uncompressed { x, y: _ } = R.to_encoded_point(false).coordinates() { - if Scalar::from_repr(*x).unwrap().eq(&r) { - return Ok(()); - } - } - Err(()) - } - #[test] fn smoke() { // 0x3a73f4123a5cd2121f21cd7e8d358835476949d035d9c2da6806b4633ac8c1e2, From a0cf20247af47d793a99fd0bb78fc7e62595e6fe Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 12 Jun 2023 05:10:57 +0000 Subject: [PATCH 4/9] chore: remove dead code --- acvm/src/pwg/blackbox/ecdsa.rs | 42 ---------------------------------- 1 file changed, 42 deletions(-) diff --git a/acvm/src/pwg/blackbox/ecdsa.rs b/acvm/src/pwg/blackbox/ecdsa.rs index 7bfba87d9..9ac61994f 100644 --- a/acvm/src/pwg/blackbox/ecdsa.rs +++ b/acvm/src/pwg/blackbox/ecdsa.rs @@ -128,50 +128,8 @@ fn verify_prehashed( #[cfg(test)] mod test { - - use k256::ecdsa::Signature; - use k256::elliptic_curve::sec1::{Coordinates, ToEncodedPoint}; - use super::verify_prehashed; - // This method is used to generate test vectors - // in noir. TODO: check that it is indeed used - #[allow(dead_code)] - fn generate_proof_data() { - use k256::ecdsa::{signature::Signer, SigningKey}; - - use sha2::{Digest, Sha256}; - - // Signing - let signing_key = SigningKey::from_bytes(&[2u8; 32]).unwrap(); - let message = - b"ECDSA proves knowledge of a secret number in the context of a single message"; - - let digest: [u8; 32] = - Sha256::digest(message).try_into().expect("SHA256 digest should be 256 bits"); - - let signature: Signature = signing_key.sign(message); - let signature_bytes: [u8; 64] = signature.as_ref().try_into().unwrap(); - assert!(Signature::try_from(signature_bytes.as_slice()).unwrap() == signature); - - // Verification - use k256::ecdsa::{signature::Verifier, VerifyingKey}; - - let verify_key = VerifyingKey::from(&signing_key); - - if let Coordinates::Uncompressed { x, y } = verify_key.to_encoded_point(false).coordinates() - { - let x: [u8; 32] = (*x).into(); - let y: [u8; 32] = (*y).into(); - - verify_prehashed(&digest, &x, &y, &signature_bytes).unwrap(); - } else { - unreachable!(); - } - - assert!(verify_key.verify(message, &signature).is_ok()); - } - #[test] fn smoke() { // 0x3a73f4123a5cd2121f21cd7e8d358835476949d035d9c2da6806b4633ac8c1e2, From 120eaa2d2eda2ab5fac83b476150efdca01e16db Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Mon, 12 Jun 2023 05:11:38 +0000 Subject: [PATCH 5/9] chore: rename test to be more meaningful --- acvm/src/pwg/blackbox/ecdsa.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acvm/src/pwg/blackbox/ecdsa.rs b/acvm/src/pwg/blackbox/ecdsa.rs index 9ac61994f..420d17333 100644 --- a/acvm/src/pwg/blackbox/ecdsa.rs +++ b/acvm/src/pwg/blackbox/ecdsa.rs @@ -131,7 +131,7 @@ mod test { use super::verify_prehashed; #[test] - fn smoke() { + fn verifies_valid_signature() { // 0x3a73f4123a5cd2121f21cd7e8d358835476949d035d9c2da6806b4633ac8c1e2, let hashed_message: [u8; 32] = [ 0x3a, 0x73, 0xf4, 0x12, 0x3a, 0x5c, 0xd2, 0x12, 0x1f, 0x21, 0xcd, 0x7e, 0x8d, 0x35, From 894d2de4310403bdcdfc7f8932310fe5a7cc9c55 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 12 Jun 2023 10:59:55 +0100 Subject: [PATCH 6/9] Update acvm/src/pwg/blackbox/ecdsa.rs Co-authored-by: kevaundray --- acvm/src/pwg/blackbox/ecdsa.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acvm/src/pwg/blackbox/ecdsa.rs b/acvm/src/pwg/blackbox/ecdsa.rs index 420d17333..098bd1b77 100644 --- a/acvm/src/pwg/blackbox/ecdsa.rs +++ b/acvm/src/pwg/blackbox/ecdsa.rs @@ -131,7 +131,7 @@ mod test { use super::verify_prehashed; #[test] - fn verifies_valid_signature() { + fn verifies_valid_signature_with_low_s_value() { // 0x3a73f4123a5cd2121f21cd7e8d358835476949d035d9c2da6806b4633ac8c1e2, let hashed_message: [u8; 32] = [ 0x3a, 0x73, 0xf4, 0x12, 0x3a, 0x5c, 0xd2, 0x12, 0x1f, 0x21, 0xcd, 0x7e, 0x8d, 0x35, From fa00e961c2003ea19acc78716c80e67f75610269 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 14 Jun 2023 05:21:20 +0000 Subject: [PATCH 7/9] chore: remove duplicated check --- acvm/src/pwg/blackbox/ecdsa.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/acvm/src/pwg/blackbox/ecdsa.rs b/acvm/src/pwg/blackbox/ecdsa.rs index 387f9e1b3..f9e1e73c3 100644 --- a/acvm/src/pwg/blackbox/ecdsa.rs +++ b/acvm/src/pwg/blackbox/ecdsa.rs @@ -101,11 +101,6 @@ fn verify_prehashed( return Err(()); } - // Ensure signature is "low S" normalized ala BIP 0062 - if s.is_high().into() { - return Err(()); - } - let s_inv = s.invert().unwrap(); let u1 = z * s_inv; let u2 = *r * s_inv; From 043e265b3be3a97417852d049681412f67ca2377 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 14 Jun 2023 05:27:21 +0000 Subject: [PATCH 8/9] chore: rename `verify_prehashed` to `verify_secp256k1_ecdsa_signature` --- acvm/src/pwg/blackbox/ecdsa.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/acvm/src/pwg/blackbox/ecdsa.rs b/acvm/src/pwg/blackbox/ecdsa.rs index f9e1e73c3..5610d47b1 100644 --- a/acvm/src/pwg/blackbox/ecdsa.rs +++ b/acvm/src/pwg/blackbox/ecdsa.rs @@ -65,14 +65,16 @@ pub(super) fn secp256k1_prehashed( ) })?; - let result = verify_prehashed(&hashed_message, &pub_key_x, &pub_key_y, &signature).is_ok(); + let result = + verify_secp256k1_ecdsa_signature(&hashed_message, &pub_key_x, &pub_key_y, &signature) + .is_ok(); initial_witness.insert(output, FieldElement::from(result)); Ok(OpcodeResolution::Solved) } -/// Verify an ECDSA signature, given the hashed message -fn verify_prehashed( +/// Verify an ECDSA signature over the secp256k1 elliptic curve, given the hashed message +fn verify_secp256k1_ecdsa_signature( hashed_msg: &[u8], public_key_x_bytes: &[u8; 32], public_key_y_bytes: &[u8; 32], @@ -120,7 +122,7 @@ fn verify_prehashed( #[cfg(test)] mod test { - use super::verify_prehashed; + use super::verify_secp256k1_ecdsa_signature; #[test] fn verifies_valid_signature_with_low_s_value() { @@ -154,7 +156,9 @@ mod test { 0x01, 0x61, 0xe4, 0x9a, 0x71, 0x5f, 0xcd, 0x55, ]; - let valid = verify_prehashed(&hashed_message, &pub_key_x, &pub_key_y, &signature).is_ok(); + let valid = + verify_secp256k1_ecdsa_signature(&hashed_message, &pub_key_x, &pub_key_y, &signature) + .is_ok(); assert!(valid) } From eedeacc59b7fc4a9f1982092c148e7f7d4f4db5e Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 14 Jun 2023 05:28:38 +0000 Subject: [PATCH 9/9] chore: return a `bool` from `verify_secp256k1_ecdsa_signature` --- acvm/src/pwg/blackbox/ecdsa.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/acvm/src/pwg/blackbox/ecdsa.rs b/acvm/src/pwg/blackbox/ecdsa.rs index 5610d47b1..39b4be156 100644 --- a/acvm/src/pwg/blackbox/ecdsa.rs +++ b/acvm/src/pwg/blackbox/ecdsa.rs @@ -65,11 +65,10 @@ pub(super) fn secp256k1_prehashed( ) })?; - let result = - verify_secp256k1_ecdsa_signature(&hashed_message, &pub_key_x, &pub_key_y, &signature) - .is_ok(); + let is_valid = + verify_secp256k1_ecdsa_signature(&hashed_message, &pub_key_x, &pub_key_y, &signature); - initial_witness.insert(output, FieldElement::from(result)); + initial_witness.insert(output, FieldElement::from(is_valid)); Ok(OpcodeResolution::Solved) } @@ -79,7 +78,7 @@ fn verify_secp256k1_ecdsa_signature( public_key_x_bytes: &[u8; 32], public_key_y_bytes: &[u8; 32], signature: &[u8; 64], -) -> Result<(), ()> { +) -> bool { // Convert the inputs into k256 data structures let signature = Signature::try_from(signature.as_slice()).unwrap(); @@ -100,7 +99,7 @@ fn verify_secp256k1_ecdsa_signature( // Ensure signature is "low S" normalized ala BIP 0062 if s.is_high().into() { - return Err(()); + return false; } let s_inv = s.invert().unwrap(); @@ -112,12 +111,10 @@ fn verify_secp256k1_ecdsa_signature( + (ProjectivePoint::from(*pubkey.as_affine()) * u2)) .to_affine(); - if let Coordinates::Uncompressed { x, y: _ } = R.to_encoded_point(false).coordinates() { - if Scalar::from_repr(*x).unwrap().eq(&r) { - return Ok(()); - } + match R.to_encoded_point(false).coordinates() { + Coordinates::Uncompressed { x, y: _ } => Scalar::from_repr(*x).unwrap().eq(&r), + _ => unreachable!("Point is uncompressed"), } - Err(()) } #[cfg(test)] @@ -157,8 +154,7 @@ mod test { ]; let valid = - verify_secp256k1_ecdsa_signature(&hashed_message, &pub_key_x, &pub_key_y, &signature) - .is_ok(); + verify_secp256k1_ecdsa_signature(&hashed_message, &pub_key_x, &pub_key_y, &signature); assert!(valid) }