diff --git a/crypto/evrf/embedwards25519/src/lib.rs b/crypto/evrf/embedwards25519/src/lib.rs index b3826fcf..913854d6 100644 --- a/crypto/evrf/embedwards25519/src/lib.rs +++ b/crypto/evrf/embedwards25519/src/lib.rs @@ -49,7 +49,7 @@ impl ShortWeierstrass for Embedwards25519 { type Repr = [u8; 32]; // Use an all-zero encoding for the identity as `0` isn't the `x` coordinate of an on-curve point const IDENTITY: [u8; 32] = [0; 32]; - fn compress(x: Self::FieldElement, odd_y: Choice) -> Self::Repr { + fn encode_compressed(x: Self::FieldElement, odd_y: Choice) -> Self::Repr { // The LE `x` coordinate, with if `y` is odd in the unused 256th bit let mut res = [0; 32]; res.as_mut().copy_from_slice(x.to_repr().as_ref()); diff --git a/crypto/evrf/secq256k1/src/lib.rs b/crypto/evrf/secq256k1/src/lib.rs index c0a7b8ef..e365c758 100644 --- a/crypto/evrf/secq256k1/src/lib.rs +++ b/crypto/evrf/secq256k1/src/lib.rs @@ -19,6 +19,7 @@ use k256::elliptic_curve::{ ff::{PrimeField, FromUniformBytes}, Group, }, + sec1::Tag, }; prime_field::odd_prime_field!( @@ -71,13 +72,12 @@ impl ShortWeierstrass for Secq256k1 { type Scalar = Scalar; type Repr = GenericArray; - // Use an all-zero encoding for the identity as `0` isn't the `x` coordinate of an on-curve point - // This uses `unsafe` to construct a `GenericArray` at compile-time as `` + /// Use the SEC1-encoded identity point, which happens to be all zeroes const IDENTITY: Self::Repr = GenericArray::from_array([0; 33]); - fn compress(x: Self::FieldElement, odd_y: Choice) -> Self::Repr { - // If `y` is odd, followed by the big-endian `x` coordinate + fn encode_compressed(x: Self::FieldElement, odd_y: Choice) -> Self::Repr { let mut res = GenericArray::default(); - res[0] = odd_y.unwrap_u8(); + res[0] = + <_>::conditional_select(&(Tag::CompressedEvenY as u8), &(Tag::CompressedOddY as u8), odd_y); { let res: &mut [u8] = res.as_mut(); res[1 ..].copy_from_slice(x.to_repr().as_ref()); @@ -86,9 +86,11 @@ impl ShortWeierstrass for Secq256k1 { } fn decode_compressed(bytes: &Self::Repr) -> (::Repr, Choice) { // Parse out if `y` is odd - let odd_y = Choice::from(bytes[0] & 1); - // Check if the extra byte was malleated - let invalid = !bytes[0].ct_eq(&odd_y.unwrap_u8()); + let odd_y = bytes[0].ct_eq(&(Tag::CompressedOddY as u8)); + // Check if the tag was malleated + let expected_tag = + <_>::conditional_select(&(Tag::CompressedEvenY as u8), &(Tag::CompressedOddY as u8), odd_y); + let invalid = !bytes[0].ct_eq(&expected_tag); // Copy the alleged `x` coordinate, overwriting with `0xffffff...` if the sign byte was // malleated (causing the `x` coordinate to be invalid) @@ -164,7 +166,7 @@ fn generator() { assert_eq!( Point::generator(), Point::from_bytes(GenericArray::from_slice(&hex_literal::hex!( - "000000000000000000000000000000000000000000000000000000000000000001" + "020000000000000000000000000000000000000000000000000000000000000001" ))) .unwrap() ); diff --git a/crypto/ff-group-tests/src/group.rs b/crypto/ff-group-tests/src/group.rs index 90ff4110..f462ec5f 100644 --- a/crypto/ff-group-tests/src/group.rs +++ b/crypto/ff-group-tests/src/group.rs @@ -158,16 +158,20 @@ pub fn test_encoding() { let bytes = point.to_bytes(); let mut repr = G::Repr::default(); repr.as_mut().copy_from_slice(bytes.as_ref()); - let decoded = G::from_bytes(&repr).unwrap(); + let decoded = G::from_bytes(&repr).expect("couldn't decode encoded point"); assert_eq!(point, decoded, "{msg} couldn't be encoded and decoded"); assert_eq!( point, - G::from_bytes_unchecked(&repr).unwrap(), - "{msg} couldn't be encoded and decoded", + G::from_bytes_unchecked(&repr) + .expect("couldn't decode encoded point with `from_bytes_unchecked`"), + "{msg} couldn't be encoded and decoded with `from_bytes_unchecked`", ); decoded }; - assert!(bool::from(test(G::identity(), "identity").is_identity())); + assert!( + bool::from(test(G::identity(), "identity").is_identity()), + "decoded identity was no longer the identity" + ); test(G::generator(), "generator"); test(G::generator() + G::generator(), "(generator * 2)"); } diff --git a/crypto/short-weierstrass/src/affine.rs b/crypto/short-weierstrass/src/affine.rs index 3bd8e118..aca4d029 100644 --- a/crypto/short-weierstrass/src/affine.rs +++ b/crypto/short-weierstrass/src/affine.rs @@ -89,8 +89,9 @@ impl Affine { pub fn decompress(x: C::FieldElement, odd_y: Choice) -> CtOption { let y_square = ((x.square() + C::A) * x) + C::B; y_square.sqrt().and_then(|mut y| { - y = <_>::conditional_select(&y, &-y, odd_y.ct_ne(&y.is_odd())); - CtOption::new(Self { x, y }, 1.into()) + y = <_>::conditional_select(&y, &-y, y.is_odd().ct_ne(&odd_y)); + // Handles the exceptional case of `y = 0, odd_y = 1` + CtOption::new(Self { x, y }, y.is_odd().ct_eq(&odd_y)) }) } diff --git a/crypto/short-weierstrass/src/lib.rs b/crypto/short-weierstrass/src/lib.rs index 3df9a446..5b586ee1 100644 --- a/crypto/short-weierstrass/src/lib.rs +++ b/crypto/short-weierstrass/src/lib.rs @@ -35,13 +35,13 @@ pub trait ShortWeierstrass: 'static + Sized + Debug { type Repr: 'static + Send + Sync + Copy + Default + AsRef<[u8]> + AsMut<[u8]>; /// The representation of the identity point. const IDENTITY: Self::Repr; - /// Compress an affine point its byte encoding. + /// Encode a compresed, on-curve point to its byte encoding. /// /// The space of potential outputs MUST exclude `Self::IDENTITY`. - fn compress(x: Self::FieldElement, odd_y: Choice) -> Self::Repr; - /// Decode a compressed point. + fn encode_compressed(x: Self::FieldElement, odd_y: Choice) -> Self::Repr; + /// Decode the `x` coordinate and if the `y` coordinate is odd from a compressed representation. /// - /// This is expected to return the `x` coordinate and if the `y` coordinate is odd. + /// This MAY return any value if the bytes represent the identity. fn decode_compressed(bytes: &Self::Repr) -> (::Repr, Choice); /// If the point is outside the largest prime-order subgroup and isn't the identity point. diff --git a/crypto/short-weierstrass/src/projective.rs b/crypto/short-weierstrass/src/projective.rs index c4218c9d..37ebe133 100644 --- a/crypto/short-weierstrass/src/projective.rs +++ b/crypto/short-weierstrass/src/projective.rs @@ -369,13 +369,11 @@ impl GroupEncoding for Projective { let (x, odd_y) = C::decode_compressed(bytes); - let result = C::FieldElement::from_repr(x).and_then(|x| { - // Parse x and recover y - let non_identity_on_curve_point = Affine::decompress(x, odd_y).map(Projective::from); - // Set the identity, if the identity - let identity = CtOption::new(Projective::IDENTITY, identity); - non_identity_on_curve_point.or_else(|| identity) - }); + let result = C::FieldElement::from_repr(x) + .and_then(|x| Affine::decompress(x, odd_y).map(Projective::from)); + // Set the identity, if the identity + let identity = CtOption::new(Projective::IDENTITY, identity); + let result = result.or_else(|| identity); let mut result_is_valid = result.is_some(); let result = result.unwrap_or(Projective::IDENTITY); @@ -394,7 +392,7 @@ impl GroupEncoding for Projective { let compressed_if_not_identity = { let affine_on_curve = affine_on_curve.unwrap_or(C::GENERATOR); let (x, y) = affine_on_curve.coordinates(); - C::compress(x, y.is_odd()) + C::encode_compressed(x, y.is_odd()) }; let mut res = C::Repr::default();