Misc fixes of secp256r1 (#618)

* Sample scalars uniformly

* Ensure canonical byte representation

* Fix call to deserialize

* Add reduced x code path

* Simplify y_odd check

* Refactor

* Remove unused imports

* Check for reduction in Fq -> Fr conversion

* Delete vdf.rs

* Fix zeroize implementation

* Check that cache size is a power of two

* Fix window size = 1

* Prevent overflow

* Fix overflow in division

* Check recovery id

* Reduce z before used to generate nonce

* Clippy

* Remove borrow

* Clearer implementation of byte substring

* Fix recovery id

* Update comment

* Use associated types

* Use underlying cmp impl

* Refactor

* Fmt#

* Avoid unwrap

fastcrypto/src/groups/multiplier/integer_utils.rs

@@ -27,15 +27,26 @@ pub fn compute_base_2w_expansion<const N: usize>(
 }
 
 /// Get the integer represented by a given range of bits of a byte from start to end (exclusive).
+/// Both the start and end parameter may be greater than 8, in which case the remaining bits of the
+/// byte will be assumed to be zero.
 #[inline]
 fn get_lendian_from_substring(byte: &u8, start: usize, end: usize) -> u8 {
     assert!(start <= end);
+    if start > 7 {
+        return 0;
+    } else if end > 8 {
+        return get_lendian_from_substring(byte, start, 8);
+    }
     byte >> start & ((1 << (end - start)) - 1) as u8
 }
 
 /// Compute ceil(numerator / denominator).
 pub(crate) fn div_ceil(numerator: usize, denominator: usize) -> usize {
-    (numerator + denominator - 1) / denominator
+    assert!(denominator > 0);
+    if numerator == 0 {
+        return 0;
+    }
+    1 + ((numerator - 1) / denominator)
 }
 
 /// Get the integer represented by a given range of bits of a an integer represented by a little-endian
@@ -91,6 +102,14 @@ pub const fn log2(x: usize) -> usize {
     (usize::BITS - x.leading_zeros() - 1) as usize
 }
 
+/// Return true iff the given number is a power of 2.
+pub fn is_power_of_2(x: usize) -> bool {
+    if x == 0 {
+        return false;
+    }
+    x & (x - 1) == 0
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -118,6 +137,9 @@ mod tests {
         assert_eq!(129, get_lendian_from_substring(&byte, 0, 8));
         assert_eq!(64, get_lendian_from_substring(&byte, 1, 8));
         assert_eq!(16, get_lendian_from_substring(&byte, 3, 8));
+        assert_eq!(129, get_lendian_from_substring(&byte, 0, 100));
+        assert_eq!(1, get_lendian_from_substring(&byte, 7, 8));
+        assert_eq!(0, get_lendian_from_substring(&byte, 8, 8));
     }
 
     #[test]
@@ -153,4 +175,17 @@ mod tests {
         assert_eq!(0, get_bits_from_bytes(&bytes, 17, 23));
         assert_eq!(1, get_bits_from_bytes(&bytes, 23, 100));
     }
+
+    #[test]
+    fn test_is_power_of_two() {
+        assert!(!is_power_of_2(0));
+        assert!(is_power_of_2(1));
+        assert!(is_power_of_2(2));
+        assert!(!is_power_of_2(3));
+        assert!(is_power_of_2(4));
+        assert!(!is_power_of_2(511));
+        assert!(is_power_of_2(512));
+        assert!(!is_power_of_2(513));
+        assert!(is_power_of_2(4096));
+    }
 }

fastcrypto/src/groups/multiplier/windowed.rs

@@ -4,7 +4,7 @@
 use std::collections::HashMap;
 use std::iter::successors;
 
-use crate::groups::multiplier::integer_utils::{get_bits_from_bytes, test_bit};
+use crate::groups::multiplier::integer_utils::{get_bits_from_bytes, is_power_of_2, test_bit};
 use crate::groups::multiplier::{integer_utils, ScalarMultiplier};
 use crate::groups::GroupElement;
 use crate::serde_helpers::ToFromByteArray;
@@ -14,7 +14,7 @@ use crate::serde_helpers::ToFromByteArray;
 /// `double_mul`, is NOT constant time. However, the single multiplication method `mul` is constant
 /// time if the group operations for `G` are constant time.
 ///
-/// The `CACHE_SIZE` should be a power of two. The `SCALAR_SIZE` is the number of bytes in the byte
+/// The `CACHE_SIZE` should be a power of two > 1. The `SCALAR_SIZE` is the number of bytes in the byte
 /// representation of the scalar type `S`, and we assume that the `S::to_byte_array` method returns
 /// the scalar in little-endian format.
 ///
@@ -54,6 +54,9 @@ impl<
     for WindowedScalarMultiplier<G, S, CACHE_SIZE, SCALAR_SIZE, SLIDING_WINDOW_WIDTH>
 {
     fn new(base_element: G) -> Self {
+        if !is_power_of_2(CACHE_SIZE) || CACHE_SIZE <= 1 {
+            panic!("CACHE_SIZE must be a power of two greater than 1");
+        }
         let mut cache = [G::zero(); CACHE_SIZE];
         cache[1] = base_element;
         for i in 2..CACHE_SIZE {
@@ -212,8 +215,8 @@ pub fn multi_scalar_mul<
 /// Compute multiples <i>2<sup>w-1</sup> base_element, (2<sup>w-1</sup> + 1) base_element, ..., (2<sup>w</sup> - 1) base_element</i>.
 fn compute_multiples<G: GroupElement>(base_element: &G, window_size: usize) -> Vec<G> {
     assert!(window_size > 0, "Window size must be strictly positive.");
-    let mut smallest_multiple = base_element.double();
-    for _ in 2..window_size {
+    let mut smallest_multiple = *base_element;
+    for _ in 1..window_size {
         smallest_multiple = smallest_multiple.double();
     }
     successors(Some(smallest_multiple), |g| Some(*g + base_element))
@@ -278,7 +281,7 @@ mod tests {
         for scalar in scalars {
             let expected = ProjectivePoint::generator() * scalar;
 
-            let multiplier = WindowedScalarMultiplier::<ProjectivePoint, Scalar, 15, 32, 4>::new(
+            let multiplier = WindowedScalarMultiplier::<ProjectivePoint, Scalar, 2, 32, 4>::new(
                 ProjectivePoint::generator(),
             );
             let actual = multiplier.mul(&scalar);
@@ -290,12 +293,6 @@ mod tests {
             let actual = multiplier.mul(&scalar);
             assert_eq!(expected, actual);
 
-            let multiplier = WindowedScalarMultiplier::<ProjectivePoint, Scalar, 17, 32, 4>::new(
-                ProjectivePoint::generator(),
-            );
-            let actual = multiplier.mul(&scalar);
-            assert_eq!(expected, actual);
-
             let multiplier = WindowedScalarMultiplier::<ProjectivePoint, Scalar, 32, 32, 4>::new(
                 ProjectivePoint::generator(),
             );

fastcrypto/src/groups/secp256r1.rs

@@ -9,9 +9,9 @@ use crate::groups::{GroupElement, Scalar as ScalarTrait};
 use crate::serde_helpers::ToFromByteArray;
 use crate::traits::AllowedRng;
 use ark_ec::Group;
-use ark_ff::{Field, One, PrimeField, Zero};
+use ark_ff::{Field, One, UniformRand, Zero};
 use ark_secp256r1::{Fr, Projective};
-use ark_serialize::CanonicalSerialize;
+use ark_serialize::{CanonicalDeserialize, CanonicalSerialize};
 use derive_more::{Add, From, Neg, Sub};
 use fastcrypto_derive::GroupOpsExtend;
 use std::ops::{Div, Mul};
@@ -96,9 +96,7 @@ impl From<u64> for Scalar {
 
 impl ScalarTrait for Scalar {
     fn rand<R: AllowedRng>(rng: &mut R) -> Self {
-        let mut bytes = [0u8; SCALAR_SIZE_IN_BYTES];
-        rng.fill_bytes(&mut bytes);
-        Scalar(Fr::from_be_bytes_mod_order(&bytes))
+        Scalar(Fr::rand(rng))
     }
 
     fn inverse(&self) -> FastCryptoResult<Self> {
@@ -110,7 +108,10 @@ impl ScalarTrait for Scalar {
 
 impl ToFromByteArray<SCALAR_SIZE_IN_BYTES> for Scalar {
     fn from_byte_array(bytes: &[u8; SCALAR_SIZE_IN_BYTES]) -> Result<Self, FastCryptoError> {
-        Ok(Scalar(Fr::from_le_bytes_mod_order(bytes)))
+        Ok(Scalar(
+            Fr::deserialize_uncompressed(bytes.as_slice())
+                .map_err(|_| FastCryptoError::InvalidInput)?,
+        ))
     }
 
     fn to_byte_array(&self) -> [u8; SCALAR_SIZE_IN_BYTES] {

fastcrypto/src/secp256r1/conversion.rs

@@ -29,35 +29,40 @@ pub(crate) fn fr_arkworks_to_p256(scalar: &ark_secp256r1::Fr) -> p256::Scalar {
 }
 
 /// Convert an arkworks field element to a p256 field element.
-pub(crate) fn fq_arkworks_to_p256(scalar: &ark_secp256r1::Fq) -> p256::Scalar {
+pub(crate) fn fq_arkworks_to_p256(scalar: &ark_secp256r1::Fq) -> FieldBytes {
     // This implementation is taken from bls_fq_to_blst_fp in fastcrypto-zkp.
     let mut bytes = [0u8; 32];
     scalar.serialize_uncompressed(&mut bytes[..]).unwrap();
-    p256::Scalar::from_uint_unchecked(U256::from_le_byte_array(FieldBytes::from(bytes)))
+    bytes.reverse();
+    p256::FieldBytes::from(bytes)
 }
 
 /// Convert an p256 affine point to an arkworks affine point.
-pub(crate) fn affine_pt_p256_to_arkworks(point: &p256::AffinePoint) -> ark_secp256r1::Affine {
+pub(crate) fn affine_pt_p256_to_projective_arkworks(
+    point: &p256::AffinePoint,
+) -> ark_secp256r1::Projective {
     if point.is_identity().into() {
-        return ark_secp256r1::Affine::zero();
+        return ark_secp256r1::Projective::zero();
     }
     let encoded_point = point.to_encoded_point(false);
-    ark_secp256r1::Affine::new_unchecked(
+    ark_secp256r1::Projective::from(ark_secp256r1::Affine::new_unchecked(
         ark_secp256r1::Fq::from_be_bytes_mod_order(encoded_point.x().unwrap()),
         ark_secp256r1::Fq::from_be_bytes_mod_order(encoded_point.y().unwrap()),
-    )
+    ))
 }
 
 /// Reduce a big-endian integer representation modulo the subgroup order in arkworks representation.
 pub(crate) fn reduce_bytes(bytes: &[u8; 32]) -> ark_secp256r1::Fr {
     ark_secp256r1::Fr::from_be_bytes_mod_order(bytes)
 }
 
-/// Reduce an arkworks field element (modulo field size) to a scalar (modulo subgroup order)
-pub(crate) fn arkworks_fq_to_fr(scalar: &ark_secp256r1::Fq) -> ark_secp256r1::Fr {
+/// Reduce an arkworks field element (modulo field size) to a scalar (modulo subgroup order). This also
+/// returns a boolean indicating whether a modular reduction was performed.
+pub(crate) fn arkworks_fq_to_fr(scalar: &ark_secp256r1::Fq) -> (ark_secp256r1::Fr, bool) {
     let mut bytes = [0u8; 32];
     scalar.serialize_uncompressed(&mut bytes[..]).unwrap();
-    ark_secp256r1::Fr::from_le_bytes_mod_order(&bytes)
+    let output = ark_secp256r1::Fr::from_le_bytes_mod_order(&bytes);
+    (output, output.into_bigint() != scalar.into_bigint())
 }
 
 /// Converts an arkworks affine point to a p256 affine point.
@@ -66,8 +71,8 @@ pub(crate) fn affine_pt_arkworks_to_p256(point: &ark_secp256r1::Affine) -> p256:
         return p256::AffinePoint::IDENTITY;
     }
     let encoded_point = p256::EncodedPoint::from_affine_coordinates(
-        &fq_arkworks_to_p256(point.x().expect("The point should not be zero")).to_bytes(),
-        &fq_arkworks_to_p256(point.y().expect("The point should not be zero")).to_bytes(),
+        &fq_arkworks_to_p256(point.x().expect("The point should not be zero")),
+        &fq_arkworks_to_p256(point.y().expect("The point should not be zero")),
         false,
     );
     p256::AffinePoint::from_encoded_point(&encoded_point).unwrap()
@@ -147,46 +152,48 @@ mod tests {
     fn test_pt_p256_to_arkworks() {
         // 0
         assert_eq!(
-            ark_secp256r1::Affine::zero(),
-            affine_pt_p256_to_arkworks(&p256::AffinePoint::IDENTITY)
+            ark_secp256r1::Projective::zero(),
+            affine_pt_p256_to_projective_arkworks(&p256::AffinePoint::IDENTITY)
         );
 
         // G
         assert_eq!(
-            ark_secp256r1::Affine::generator() * ark_secp256r1::Fr::from(7u32),
-            affine_pt_p256_to_arkworks(
+            ark_secp256r1::Projective::generator() * ark_secp256r1::Fr::from(7u32),
+            affine_pt_p256_to_projective_arkworks(
                 &(p256::AffinePoint::generator() * p256::Scalar::from(7u32)).to_affine()
             )
         );
 
         // 7G
         assert_eq!(
-            ark_secp256r1::Affine::generator(),
-            affine_pt_p256_to_arkworks(&p256::AffinePoint::generator())
+            ark_secp256r1::Projective::generator(),
+            affine_pt_p256_to_projective_arkworks(&p256::AffinePoint::generator())
         );
 
         // sG, random s
         let random_s = p256::Scalar::random(&mut rand::thread_rng());
         assert_eq!(
-            (ark_secp256r1::Affine::generator() * fr_p256_to_arkworks(&random_s)).into_affine(),
-            affine_pt_p256_to_arkworks(&(p256::AffinePoint::generator() * random_s).to_affine())
+            (ark_secp256r1::Projective::generator() * fr_p256_to_arkworks(&random_s)).into_affine(),
+            affine_pt_p256_to_projective_arkworks(
+                &(p256::AffinePoint::generator() * random_s).to_affine()
+            )
         );
     }
 
     #[test]
     fn test_arkworks_fq_to_fr() {
         let s = ark_secp256r1::Fq::rand(&mut rand::thread_rng());
-        let s_fr = arkworks_fq_to_fr(&s);
+        let s_fr = arkworks_fq_to_fr(&s).0;
         let p256_s = fq_arkworks_to_p256(&s);
-        let reduced_s = p256::Scalar::reduce_bytes(&p256_s.to_bytes());
+        let reduced_s = p256::Scalar::reduce_bytes(&p256_s);
         assert_eq!(fr_arkworks_to_p256(&s_fr), reduced_s);
-        assert_eq!(reduce_bytes(&p256_s.to_bytes().try_into().unwrap()), s_fr);
+        assert_eq!(reduce_bytes(&p256_s.try_into().unwrap()), s_fr);
     }
 
     #[test]
     fn test_fq_arkworks_to_p256() {
         let arkworks_seven = ark_secp256r1::Fq::from(7u32);
-        let p256_seven = p256::Scalar::from(7u32);
+        let p256_seven = p256::FieldBytes::from(p256::Scalar::from(7u32));
 
         let actual_p256_seven = fq_arkworks_to_p256(&arkworks_seven);
         assert_eq!(actual_p256_seven, p256_seven);