diff --git a/acvm-repo/blackbox_solver/src/ecdsa/secp256k1.rs b/acvm-repo/blackbox_solver/src/ecdsa/secp256k1.rs index 64651147ebc..090b88a5715 100644 --- a/acvm-repo/blackbox_solver/src/ecdsa/secp256k1.rs +++ b/acvm-repo/blackbox_solver/src/ecdsa/secp256k1.rs @@ -11,6 +11,27 @@ use k256::{ }; use k256::{Scalar, ecdsa::Signature}; +/// Verifies an ECDSA signature over the Secp256k1 elliptic curve. +/// +/// This function implements ECDSA signature verification on the Secp256k1 curve +/// +/// # Parameters: +/// +/// * `hashed_msg` - The 32-byte hash of the message that was signed +/// * `public_key_x_bytes` - The x-coordinate of the public key (32 bytes, big-endian) +/// * `public_key_y_bytes` - The y-coordinate of the public key (32 bytes, big-endian) +/// * `signature` - The 64-byte signature in (r, s) format, where r and s are 32 bytes each +/// +/// Returns `true` if the signature is valid, `false` otherwise. +/// +/// The function panic if the following is not true: +/// - The signature components `r` and `s` must be non-zero +/// - The public key point must lie on the Secp256k1 curve +/// - The signature must be "low S" normalized per BIP 0062 to prevent malleability +/// +/// The function will also panic if `hashed_msg >= k256::Secp256k1::ORDER`. +/// According to ECDSA specification, the message hash leftmost bits should be truncated +/// up to the curve order length, and then reduced modulo the curve order. pub(super) fn verify_signature( hashed_msg: &[u8; 32], public_key_x_bytes: &[u8; 32], @@ -31,15 +52,14 @@ pub(super) fn verify_signature( ); let pubkey = PublicKey::from_encoded_point(&point); - let pubkey = if pubkey.is_some().into() { - pubkey.unwrap() - } else { + if pubkey.is_none().into() { // Public key must sit on the Secp256k1 curve. log::warn!("Invalid public key provided for ECDSA verification"); return false; - }; + } + let pubkey = pubkey.unwrap(); - // Note: This is incorrect as it will panic if `hashed_msg >= k256::Secp256k1::ORDER`. + // Note: This will panic if `hashed_msg >= k256::Secp256k1::ORDER`. // In this scenario we should just take the leftmost bits from `hashed_msg` up to the group order length. let z = Scalar::from_repr(*GenericArray::from_slice(hashed_msg)).unwrap(); diff --git a/acvm-repo/blackbox_solver/src/ecdsa/secp256r1.rs b/acvm-repo/blackbox_solver/src/ecdsa/secp256r1.rs index 66005135987..98bb4476794 100644 --- a/acvm-repo/blackbox_solver/src/ecdsa/secp256r1.rs +++ b/acvm-repo/blackbox_solver/src/ecdsa/secp256r1.rs @@ -11,6 +11,27 @@ use p256::{ }; use p256::{Scalar, ecdsa::Signature}; +/// Verifies an ECDSA signature over the Secp256r1 elliptic curve. +/// +/// This function implements ECDSA signature verification on the Secp256r1 curve +/// +/// # Parameters: +/// +/// * `hashed_msg` - The 32-byte hash of the message that was signed +/// * `public_key_x_bytes` - The x-coordinate of the public key (32 bytes, big-endian) +/// * `public_key_y_bytes` - The y-coordinate of the public key (32 bytes, big-endian) +/// * `signature` - The 64-byte signature in (r, s) format, where r and s are 32 bytes each +/// +/// Returns `true` if the signature is valid, `false` otherwise. +/// +/// The function panic if the following is not true: +/// - The signature components `r` and `s` must be non-zero +/// - The public key point must lie on the Secp256r1 curve +/// - The signature must be "low S" normalized per BIP 0062 to prevent malleability +/// +/// The function will also panic if `hashed_msg >= p256::NistP256::ORDER`. +/// According to ECDSA specification, the message hash leftmost bits should be truncated +/// up to the curve order length, and then reduced modulo the curve order. pub(super) fn verify_signature( hashed_msg: &[u8], public_key_x_bytes: &[u8; 32], @@ -31,15 +52,14 @@ pub(super) fn verify_signature( ); let pubkey = PublicKey::from_encoded_point(&point); - let pubkey = if pubkey.is_some().into() { - pubkey.unwrap() - } else { + if pubkey.is_none().into() { // Public key must sit on the Secp256r1 curve. log::warn!("Invalid public key provided for ECDSA verification"); return false; }; + let pubkey = pubkey.unwrap(); - // Note: This is incorrect as it will panic if `hashed_msg >= p256::NistP256::ORDER`. + // Note: This will panic if `hashed_msg >= p256::NistP256::ORDER`. // In this scenario we should just take the leftmost bits from `hashed_msg` up to the group order length. let z = Scalar::from_repr(*GenericArray::from_slice(hashed_msg)).unwrap(); diff --git a/acvm-repo/blackbox_solver/src/hash.rs b/acvm-repo/blackbox_solver/src/hash.rs index de322f00341..fd9b69a892e 100644 --- a/acvm-repo/blackbox_solver/src/hash.rs +++ b/acvm-repo/blackbox_solver/src/hash.rs @@ -32,7 +32,7 @@ pub fn sha256_compression(state: &mut [u32; 8], msg_blocks: &[u32; 16]) { } const KECCAK_LANES: usize = 25; - +/// Keccak permutation for a state of size 1600 bits, represented by 25 lanes of 64 bits (25*64 = 1600) pub fn keccakf1600( mut state: [u64; KECCAK_LANES], ) -> Result<[u64; KECCAK_LANES], BlackBoxResolutionError> { diff --git a/acvm-repo/blackbox_solver/src/logic.rs b/acvm-repo/blackbox_solver/src/logic.rs index ed12462af29..ec6664d800a 100644 --- a/acvm-repo/blackbox_solver/src/logic.rs +++ b/acvm-repo/blackbox_solver/src/logic.rs @@ -8,16 +8,15 @@ pub fn bit_xor(lhs: F, rhs: F, num_bits: u32) -> F { bitwise_op(lhs, rhs, num_bits, |lhs_byte, rhs_byte| lhs_byte ^ rhs_byte) } +/// Performs a bitwise operation on two field elements by treating them as byte arrays. +/// +/// Both field elements are converted to little-endian byte arrays and masked to keep only +/// the lowest `num_bits` bits. The provided operation `op` is then applied byte-by-byte, +/// and the result is converted back to a field element. +/// This function works for any `num_bits` value and does not assume it to be a multiple of 8. fn bitwise_op(lhs: F, rhs: F, num_bits: u32, op: fn(u8, u8) -> u8) -> F { - // XXX: Gadgets like SHA256 need to have their input be a multiple of 8 - // This is not a restriction caused by SHA256, as it works on bits - // but most backends assume bytes. - // We could implicitly pad, however this may not be intuitive for users. - // assert!( - // num_bits % 8 == 0, - // "num_bits is not a multiple of 8, it is {}", - // num_bits - // ); + // We could explicitly expect `num_bits` to be a multiple of 8 as most backends assume bytes: + // assert!(num_bits % 8 == 0, "num_bits is not a multiple of 8, it is {num_bits}"); let lhs_bytes = mask_to_le_bytes(lhs, num_bits); let rhs_bytes = mask_to_le_bytes(rhs, num_bits); @@ -50,8 +49,9 @@ fn mask_vector_le(bytes: &mut [u8], num_bits: usize) { // Find how many bits to keep in that byte (0-7) let mask_power = num_bits % 8; - // Mask the partial byte if any. Otherwise, if our `num_bits` is a multiple of 8 - // we will get a `mask_power` of zero and thus zero out the byte at `array_mask_index`. + // If `mask_power` is non-zero, this keeps only the lower `mask_power` bits of the byte. + // If `mask_power` is zero (when `num_bits` is a multiple of 8), this zeros out the byte, + // which is correct since that byte is the first one beyond what we want to keep. bytes[array_mask_index] &= 2u8.pow(mask_power as u32) - 1; // Zero out all remaining bytes