From 45e336645d1bc9e4b9498f55a33a6ad88a432401 Mon Sep 17 00:00:00 2001 From: David Nevado Date: Thu, 29 Feb 2024 09:46:45 +0100 Subject: [PATCH] fix: review comments add: uncompressed identity test fix: uncompressed serialization fix: cleanup fix: review comments add: compute spare bits from NUM_BITS fix: strict flag decoding fix: imports add: check for the bits in 0 spare bits case --- src/bn256/curve.rs | 5 +- src/derive/curve.rs | 155 ++++++++++++++++++++++++++++------------ src/grumpkin/curve.rs | 4 +- src/pluto_eris/curve.rs | 6 +- src/secp256k1/curve.rs | 5 +- src/secp256r1/curve.rs | 4 +- src/secq256k1/curve.rs | 4 +- src/tests/curve.rs | 12 +++- 8 files changed, 129 insertions(+), 66 deletions(-) diff --git a/src/bn256/curve.rs b/src/bn256/curve.rs index 7787223f..e420a506 100644 --- a/src/bn256/curve.rs +++ b/src/bn256/curve.rs @@ -5,7 +5,7 @@ use crate::arithmetic::EndoParameters; use crate::bn256::Fq; use crate::bn256::Fq2; use crate::bn256::Fr; -use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, NEG_Y_MASK, NEG_Y_SHIFT}; +use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, SIGN_MASK, SIGN_SHIFT}; use crate::endo; use crate::ff::WithSmallOrderMulGroup; use crate::ff::{Field, PrimeField}; @@ -33,7 +33,6 @@ new_curve_impl!( (pub), G1, G1Affine, - 2, Fq, Fr, (G1_GENERATOR_X,G1_GENERATOR_Y), @@ -47,7 +46,6 @@ new_curve_impl!( (pub), G2, G2Affine, - 2, Fq2, Fr, (G2_GENERATOR_X, G2_GENERATOR_Y), @@ -202,6 +200,7 @@ impl G1 { #[cfg(test)] mod test { use super::*; + use group::UncompressedEncoding; crate::curve_testing_suite!(G1, G2); crate::curve_testing_suite!(G1, "hash_to_curve"); crate::curve_testing_suite!(G1, "endo_consistency"); diff --git a/src/derive/curve.rs b/src/derive/curve.rs index 8b7e62ac..981df88e 100644 --- a/src/derive/curve.rs +++ b/src/derive/curve.rs @@ -49,10 +49,10 @@ macro_rules! endo { }; } -// These masks work for 0, 1, and 2 spare bits. -// The only curve that does not fit these masks is Bls12-381. -pub(crate) const NEG_Y_MASK: u8 = 0b1000_0000; -pub(crate) const NEG_Y_SHIFT: u8 = 7; +// Sign mask for 0, 1 and 2 spare bits. +pub(crate) const SIGN_MASK: u8 = 0b1000_0000; +pub(crate) const SIGN_SHIFT: u8 = 7; +// Identity mask for 0 and 2 spare bits (1 spare bit does not use it). pub(crate) const IS_IDENTITY_MASK: u8 = 0b0100_0000; pub(crate) const IS_IDENTITY_SHIFT: u8 = 6; @@ -61,7 +61,6 @@ macro_rules! new_curve_impl { (($($privacy:tt)*), $name:ident, $name_affine:ident, - $spare_bits:expr, $base:ident, $scalar:ident, $generator:expr, @@ -72,10 +71,10 @@ macro_rules! new_curve_impl { ) => { macro_rules! impl_compressed { - () => { + ($spare_bits: expr) => { paste::paste! { - // The copressed size is the size of the x-coordinate (one base field element) + // The compressed size is the size of the x-coordinate (one base field element) // when there is at least 1 spare bit. When there is no spare bits (secp256k1) // the size is increased by 1 byte. #[allow(non_upper_case_globals)] @@ -125,7 +124,7 @@ macro_rules! new_curve_impl { } fn from_bytes_unchecked(bytes: &Self::Repr) -> CtOption { - $name_affine::from_bytes(bytes).map(Self::from) + $name_affine::from_bytes_unchecked(bytes).map(Self::from) } fn to_bytes(&self) -> Self::Repr { @@ -134,7 +133,7 @@ macro_rules! new_curve_impl { } - // The flags are placed in the last byte, the MSB. + // The flags are placed in the last byte (the most significant byte). #[allow(non_upper_case_globals)] const [< $name _FLAG_BYTE_INDEX>]: usize= [< $name _COMPRESSED_SIZE >]-1 ; @@ -155,36 +154,76 @@ macro_rules! new_curve_impl { fn from_bytes(bytes: &Self::Repr) -> CtOption { let mut tmp = bytes.0; - // When only 1 spare bit, there is no identity flag - let is_inf = if $spare_bits == 1 { - Choice::from(1u8) + + let flag_byte = tmp[[< $name _FLAG_BYTE_INDEX>]]; + // Get identity and sign flags. + let identity_flag = if $spare_bits == 0 || $spare_bits == 2 { + Choice::from((flag_byte & IS_IDENTITY_MASK) >> IS_IDENTITY_SHIFT ) + } else { + Choice::from(0u8) + }; + + let sign_flag = Choice::from( (flag_byte & SIGN_MASK) >> SIGN_SHIFT ); + + let extra_bits = if $spare_bits == 0 { + // In the case of 0 spare bits, an extra byte is added to hold the flags. + // In this byte, the 6 least significant bits must be set to 0. + Choice::from(u8::from((flag_byte & 0b0011_1111) !=0) ) } else { - Choice::from((tmp[[< $name _FLAG_BYTE_INDEX>]] & IS_IDENTITY_MASK) >> IS_IDENTITY_SHIFT ) + // There are no extra bits in the rest of cases. + Choice::from(0u8) }; - let ysign = Choice::from( (tmp[[< $name _FLAG_BYTE_INDEX>]] & NEG_Y_MASK) >> NEG_Y_SHIFT ); + // Clear flag bits tmp[[< $name _FLAG_BYTE_INDEX>]] &= [< $name _FLAG_BITS >]; + // Get x-coordinate let mut xbytes = [0u8; $base::size()]; xbytes.copy_from_slice(&tmp[..$base::size()]); + + $base::from_bytes(&xbytes).and_then(|x| { + + // Decide if the point is the identity and the validity of the encoding. + let (is_valid, is_identity) = + if $spare_bits == 0 || $spare_bits == 2 { + // Correct encoding follows one of the following: + // 1. Identity: + // identity_flag = 1, sign = 0, x = 0, extra_bits = 0 + // 2. Non-identity: + // identity_flag = 0, , extra_bits = 0 + // + // is_valid = !identity_flag \/ (!sign /\ x.is_zero()) /\ !extra_bits + ( (!identity_flag | (!sign_flag & x.is_zero())) & !extra_bits, identity_flag) + } else { + // Correct encoding follows one of the following: + // 1. Identity: + // sign = 0, x = 0 + // 2. Non-identity: + // x!=0 + // + // is_valid = !(x.is_zero() /\ sign_flag) + ( !(x.is_zero() & sign_flag), x.is_zero()) + }; + + CtOption::new( Self::identity(), - x.is_zero() & (is_inf)) + is_identity) .or_else(|| { - //Computes corresponding y + // Computes corresponding y coordinate. $name_affine::y2(x).sqrt().and_then(|y| { // Get sign of obtained solution. sign = y % 2. let sign = Choice::from(y.to_bytes()[0] & 1); // Adjust sign if necessary. - let y = $base::conditional_select(&y, &-y, ysign ^ sign); + let y = $base::conditional_select(&y, &-y, sign_flag ^ sign); CtOption::new( $name_affine { x, y, }, - Choice::from(1u8), + is_valid, ) }) }) @@ -199,8 +238,7 @@ macro_rules! new_curve_impl { fn to_bytes(&self) -> Self::Repr { if bool::from(self.is_identity()) { let mut bytes = [0; [< $name _COMPRESSED_SIZE >]]; - // When spare_bits == 1, the identity is all 0's. - if $spare_bits == 0 || $spare_bits ==2 { + if $spare_bits == 0 || $spare_bits == 2 { bytes[[< $name _FLAG_BYTE_INDEX>]] |= IS_IDENTITY_MASK; } [< $name Compressed >](bytes) @@ -209,7 +247,7 @@ macro_rules! new_curve_impl { let mut xbytes = [0u8; [< $name _COMPRESSED_SIZE >]]; xbytes[..$base::size()].copy_from_slice(&x.to_bytes()); if (y.to_bytes()[0] & 1) == 1 { - xbytes[[< $name _FLAG_BYTE_INDEX>]] |= NEG_Y_MASK; + xbytes[[< $name _FLAG_BYTE_INDEX>]] |= SIGN_MASK; } [< $name Compressed >](xbytes) } @@ -222,13 +260,9 @@ macro_rules! new_curve_impl { } macro_rules! impl_uncompressed { - () => { + ($spare_bits: expr) => { paste::paste! { - #[allow(non_upper_case_globals)] - const [< $name _FLAT_BYTE_INDEX >]: usize = - 2 *$base::size() - 1; - #[derive(Copy, Clone)] pub struct [< $name Uncompressed >]([u8; 2*$base::size()]); impl std::fmt::Debug for [< $name Uncompressed >] { @@ -278,32 +312,56 @@ macro_rules! new_curve_impl { } fn from_uncompressed_unchecked(bytes: &Self::Uncompressed) -> CtOption { - let bytes = &bytes.0; - let infinity_flag_set = if $spare_bits == 1 { - // With 1 spare bit there is no infinity flag, so we just rely - // on the x, y coordinates to be set to 0. - Choice::from(1u8) + let mut bytes = bytes.0; + + let flag_idx = 2* $base::size() -1; + + // In the uncompressed format, the bit in the sign flag position must be 0 always. + let sign_flag = if $spare_bits == 2 || $spare_bits == 1 { + Choice::from( (bytes[ flag_idx ] & SIGN_MASK) >> SIGN_SHIFT ) } else { - Choice::from((( bytes[[<$name _FLAT_BYTE_INDEX >]] & IS_IDENTITY_MASK) >> IS_IDENTITY_SHIFT) ) + // For 0 spare bits, there is no sign flag. + Choice::from(0u8) }; + // Get identity flag. + let identity_flag= if $spare_bits == 2 { + let identity_flag = Choice::from( ( ( bytes[ flag_idx ] & IS_IDENTITY_MASK) >> IS_IDENTITY_SHIFT) ); + + // Clear flags. + bytes[flag_idx] &= [< $name _FLAG_BITS >]; + identity_flag + } else { + // With 0 and 1 spare bit there is no identity flag, so we just rely + // on the x, y coordinates to be set to 0. + Choice::from(0u8) + }; + + // Get x, y coordinates. + let mut repr = [0u8; $base::size()]; let x = { - let mut tmp = [0; $base::size()]; - tmp.copy_from_slice(&bytes[0..$base::size()]); - $base::from_bytes(&tmp) + repr.copy_from_slice(&bytes[0..$base::size()]); + $base::from_bytes(&repr) }; let y = { - let mut tmp = [0; $base::size()]; - tmp.copy_from_slice(&bytes[$base::size()..2*$base::size()]); - $base::from_bytes(&tmp) + repr.copy_from_slice(&bytes[$base::size()..2*$base::size()]); + $base::from_bytes(&repr) }; + x.and_then(|x| { y.and_then(|y| { - // The point is the identity iff the x and y coordinates are zero - // and the identity flag is set, in the fomrats where such flag is present. - let is_ident = infinity_flag_set & x.is_zero() & y.is_zero(); + let zero_coords = x.is_zero() & y.is_zero(); + + // Check encoding validity: If the identity flag is set, then the x,y coordinates must be 0. + let (is_valid, is_identity) = if $spare_bits == 2 { + (!( zero_coords ^ identity_flag) & !sign_flag, identity_flag) + } else { + // With 1 spare bit, the encoding is always valid, and the identity is encoded with a 0 x-coordinate. + (!sign_flag, zero_coords) + }; + let p = $name_affine::conditional_select( &$name_affine{ @@ -311,12 +369,13 @@ macro_rules! new_curve_impl { y, }, &$name_affine::identity(), - is_ident, + is_identity, ); + CtOption::new( p, - is_ident, + Choice::from(is_valid), ) }) }) @@ -331,8 +390,8 @@ macro_rules! new_curve_impl { res[$base::size().. 2*$base::size()].copy_from_slice( &$base::conditional_select(&self.y, &$base::zero(), self.is_identity()).to_bytes()[..], ); - if $spare_bits == 0 || $spare_bits == 2 { - res[[< $name _FLAG_BYTE_INDEX >]] |= u8::conditional_select(&0u8, &IS_IDENTITY_MASK, self.is_identity()); + if $spare_bits == 2 { + res[ 2*$base::size() -1 ] |= u8::conditional_select(&0u8, &IS_IDENTITY_MASK, self.is_identity()); } [< $name Uncompressed >](res) @@ -423,8 +482,10 @@ macro_rules! new_curve_impl { #[cfg(feature = "derive_serde")] serialize_deserialize_to_from_bytes!(); - impl_compressed!(); - impl_uncompressed!(); + // Base's num_bits is the number of bits for the base prime field, + // so the computation of spare bits is correct for extensions as well. + impl_compressed!((($base::NUM_BITS-1) / 8 +1) * 8 - $base::NUM_BITS); + impl_uncompressed!((($base::NUM_BITS-1) / 8 +1) * 8 - $base::NUM_BITS); diff --git a/src/grumpkin/curve.rs b/src/grumpkin/curve.rs index aa9e40ac..f0d67a1b 100644 --- a/src/grumpkin/curve.rs +++ b/src/grumpkin/curve.rs @@ -2,7 +2,7 @@ use crate::arithmetic::mul_512; use crate::arithmetic::sbb; use crate::arithmetic::CurveEndo; use crate::arithmetic::EndoParameters; -use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, NEG_Y_MASK, NEG_Y_SHIFT}; +use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, SIGN_MASK, SIGN_SHIFT}; use crate::ff::WithSmallOrderMulGroup; use crate::ff::{Field, PrimeField}; use crate::group::Curve; @@ -30,7 +30,6 @@ new_curve_impl!( (pub), G1, G1Affine, - 2, Fq, Fr, (G1_GENERATOR_X, G1_GENERATOR_Y), @@ -93,6 +92,7 @@ impl G1 { #[cfg(test)] mod test { use super::*; + use group::UncompressedEncoding; crate::curve_testing_suite!(G1); crate::curve_testing_suite!(G1, "endo_consistency"); crate::curve_testing_suite!(G1, "endo"); diff --git a/src/pluto_eris/curve.rs b/src/pluto_eris/curve.rs index 43622ccf..a022247e 100644 --- a/src/pluto_eris/curve.rs +++ b/src/pluto_eris/curve.rs @@ -1,5 +1,5 @@ use super::fields::{fp::Fp, fp2::Fp2, fq::Fq}; -use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, NEG_Y_MASK, NEG_Y_SHIFT}; +use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, SIGN_MASK, SIGN_SHIFT}; use crate::ff::WithSmallOrderMulGroup; use crate::ff::{Field, PrimeField}; use crate::group::{prime::PrimeCurveAffine, Curve, Group as _, GroupEncoding}; @@ -125,7 +125,6 @@ new_curve_impl!( (pub), G1, G1Affine, - 2, Fp, Fq, (G1_GENERATOR_X,G1_GENERATOR_Y), @@ -161,7 +160,6 @@ new_curve_impl!( (pub), Eris, ErisAffine, - 2, Fq, Fp, (ERIS_GENERATOR_X,ERIS_GENERATOR_Y), @@ -233,7 +231,6 @@ new_curve_impl!( (pub), G2, G2Affine, - 2, Fp2, Fq, (G2_GENERATOR_X,G2_GENERATOR_Y), @@ -246,6 +243,7 @@ new_curve_impl!( #[cfg(test)] mod test { use super::*; + use group::UncompressedEncoding; crate::curve_testing_suite!(G1, Eris, G2); crate::curve_testing_suite!(G1, Eris, "hash_to_curve"); crate::curve_testing_suite!(G1, Eris, "endo_consistency"); diff --git a/src/secp256k1/curve.rs b/src/secp256k1/curve.rs index bc5b26c7..2fe74261 100644 --- a/src/secp256k1/curve.rs +++ b/src/secp256k1/curve.rs @@ -1,4 +1,4 @@ -use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, NEG_Y_MASK, NEG_Y_SHIFT}; +use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, SIGN_MASK, SIGN_SHIFT}; use crate::ff::WithSmallOrderMulGroup; use crate::ff::{Field, PrimeField}; use crate::group::{prime::PrimeCurveAffine, Curve, Group as _, GroupEncoding}; @@ -59,7 +59,6 @@ new_curve_impl!( (pub), Secp256k1, Secp256k1Affine, - 0, Fp, Fq, (SECP_GENERATOR_X,SECP_GENERATOR_Y), @@ -128,7 +127,6 @@ new_curve_impl!( (pub(crate)), IsoSecp256k1, IsoSecp256k1Affine, - 0, Fp, Fq, (ISO_SECP_GENERATOR_X, ISO_SECP_GENERATOR_Y), @@ -272,6 +270,7 @@ pub(crate) fn iso_map_secp256k1(rp: IsoSecp256k1) -> Secp256k1 { #[cfg(test)] mod test { use super::*; + use group::UncompressedEncoding; crate::curve_testing_suite!(Secp256k1); crate::curve_testing_suite!(Secp256k1, "endo_consistency"); crate::curve_testing_suite!(Secp256k1, "ecdsa_example"); diff --git a/src/secp256r1/curve.rs b/src/secp256r1/curve.rs index 210ad5c2..14ceaa78 100644 --- a/src/secp256r1/curve.rs +++ b/src/secp256r1/curve.rs @@ -1,4 +1,4 @@ -use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, NEG_Y_MASK, NEG_Y_SHIFT}; +use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, SIGN_MASK, SIGN_SHIFT}; use crate::ff::WithSmallOrderMulGroup; use crate::ff::{Field, PrimeField}; use crate::group::{prime::PrimeCurveAffine, Curve, Group as _, GroupEncoding}; @@ -70,7 +70,6 @@ new_curve_impl!( (pub), Secp256r1, Secp256r1Affine, - 0, Fp, Fq, (SECP_GENERATOR_X,SECP_GENERATOR_Y), @@ -95,6 +94,7 @@ impl Secp256r1 { #[cfg(test)] mod test { use super::*; + use group::UncompressedEncoding; crate::curve_testing_suite!(Secp256r1); crate::curve_testing_suite!(Secp256r1, "ecdsa_example"); crate::curve_testing_suite!( diff --git a/src/secq256k1/curve.rs b/src/secq256k1/curve.rs index 221c997f..f6c0be24 100644 --- a/src/secq256k1/curve.rs +++ b/src/secq256k1/curve.rs @@ -1,4 +1,4 @@ -use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, NEG_Y_MASK, NEG_Y_SHIFT}; +use crate::derive::curve::{IS_IDENTITY_MASK, IS_IDENTITY_SHIFT, SIGN_MASK, SIGN_SHIFT}; use crate::ff::WithSmallOrderMulGroup; use crate::ff::{Field, PrimeField}; use crate::group::Curve; @@ -42,7 +42,6 @@ new_curve_impl!( (pub), Secq256k1, Secq256k1Affine, - 0, Fq, Fp, (SECQ_GENERATOR_X, SECQ_GENERATOR_Y), @@ -75,6 +74,7 @@ impl Secq256k1 { #[cfg(test)] mod test { use super::*; + use group::UncompressedEncoding; crate::curve_testing_suite!(Secq256k1); crate::curve_testing_suite!(Secq256k1, "endo_consistency"); crate::curve_testing_suite!( diff --git a/src/tests/curve.rs b/src/tests/curve.rs index 7a634417..b89863cf 100644 --- a/src/tests/curve.rs +++ b/src/tests/curve.rs @@ -238,11 +238,19 @@ macro_rules! curve_testing_suite { .unwrap() .is_identity() )); + + assert!(bool::from( + <$c as CurveExt>::AffineExt::from_uncompressed(&<$c as CurveExt>::AffineExt::identity().to_uncompressed()) + .unwrap() + .is_identity() + )); + assert!(bool::from( <$c as CurveExt>::AffineExt::from_bytes(&<$c as CurveExt>::AffineExt::identity().to_bytes()) .unwrap() .is_identity() )); + for _ in 0..100 { let projective_point = $c::random(OsRng); let affine_point: <$c as CurveExt>::AffineExt = projective_point.into(); @@ -263,7 +271,6 @@ macro_rules! curve_testing_suite { assert_eq!(affine_point, affine_point_rec_unchecked); // Uncompressed format - use group::UncompressedEncoding; let affine_repr = affine_point.to_uncompressed(); let affine_point_rec = <$c as CurveExt>::AffineExt::from_uncompressed_unchecked(&affine_repr).unwrap(); @@ -271,8 +278,6 @@ macro_rules! curve_testing_suite { assert_eq!(affine_point, affine_point_rec); assert_eq!(affine_point, affine_point_rec_unchecked); - - } } } @@ -344,6 +349,7 @@ macro_rules! curve_testing_suite { use crate::{CurveAffine, CurveExt}; use rand_core::OsRng; + #[test] fn test_curve() { $(