3.5.2 Add more tests to ff-group-tests

The audit recommends checking failure cases for from_bytes,
from_bytes_unechecked, and from_repr. This isn't feasible.

from_bytes is allowed to have non-canonical values. [0xff; 32] may accordingly
be a valid point for non-SEC1-encoded curves.

from_bytes_unchecked doesn't have a defined failure mode, and by name,
unchecked, shouldn't necessarily fail. The audit acknowledges the tests should
test for whatever result is 'appropriate', yet any result which isn't a failure
on a valid element is appropriate.

from_repr must be canonical, yet for a binary field of 2^n where n % 8 == 0, a
[0xff; n / 8] repr would be valid.

Cargo.lock

@@ -2342,6 +2342,7 @@ dependencies = [
  "group",
  "k256",
  "p256",
+ "rand_core 0.6.4",
 ]
 
 [[package]]

crypto/ciphersuite/README.md

@@ -27,8 +27,8 @@ The domain-separation tag is naively prefixed to the message.
 ### Ed448
 
 Ed448 is offered via [minimal-ed448](https://crates.io/crates/minimal-ed448), an
-explicitly not recommended, unaudited Ed448 implementation, limited to its
+explicitly not recommended, unaudited, incomplete Ed448 implementation, limited
-prime-order subgroup.
+to its prime-order subgroup.
 
 Its `hash_to_F` is the wide reduction of SHAKE256, with a 114-byte output, as
 used in [RFC-8032](https://www.rfc-editor.org/rfc/rfc8032). The

crypto/ciphersuite/src/dalek.rs

@@ -48,7 +48,7 @@ dalek_curve!("ristretto", Ristretto, RistrettoPoint, b"ristretto");
 #[cfg(any(test, feature = "ristretto"))]
 #[test]
 fn test_ristretto() {
-  ff_group_tests::group::test_prime_group_bits::<RistrettoPoint>();
+  ff_group_tests::group::test_prime_group_bits::<_, RistrettoPoint>(&mut rand_core::OsRng);
 
   assert_eq!(
     Ristretto::hash_to_F(
@@ -79,7 +79,7 @@ dalek_curve!("ed25519", Ed25519, EdwardsPoint, b"edwards25519");
 #[cfg(feature = "ed25519")]
 #[test]
 fn test_ed25519() {
-  ff_group_tests::group::test_prime_group_bits::<EdwardsPoint>();
+  ff_group_tests::group::test_prime_group_bits::<_, EdwardsPoint>(&mut rand_core::OsRng);
 
   // Ideally, a test vector from RFC-8032 (not FROST) would be here
   // Unfortunately, the IETF draft doesn't provide any vectors for the derived challenges

crypto/ciphersuite/src/kp256.rs

@@ -114,7 +114,7 @@ kp_curve!("secp256k1", k256, Secp256k1, b"secp256k1");
 #[cfg(feature = "secp256k1")]
 #[test]
 fn test_secp256k1() {
-  ff_group_tests::group::test_prime_group_bits::<k256::ProjectivePoint>();
+  ff_group_tests::group::test_prime_group_bits::<_, k256::ProjectivePoint>(&mut rand_core::OsRng);
 
   // Ideally, a test vector from hash_to_field (not FROST) would be here
   // Unfortunately, the IETF draft only provides vectors for field elements, not scalars
@@ -152,7 +152,7 @@ kp_curve!("p256", p256, P256, b"P-256");
 #[cfg(feature = "p256")]
 #[test]
 fn test_p256() {
-  ff_group_tests::group::test_prime_group_bits::<p256::ProjectivePoint>();
+  ff_group_tests::group::test_prime_group_bits::<_, p256::ProjectivePoint>(&mut rand_core::OsRng);
 
   assert_eq!(
     P256::hash_to_F(

crypto/dalek-ff-group/src/field.rs

@@ -281,5 +281,5 @@ fn test_sqrt_m1() {
 
 #[test]
 fn test_field() {
-  ff_group_tests::prime_field::test_prime_field_bits::<FieldElement>();
+  ff_group_tests::prime_field::test_prime_field_bits::<_, FieldElement>(&mut rand_core::OsRng);
 }

crypto/dalek-ff-group/src/lib.rs

@@ -448,10 +448,10 @@ fn test_scalar_modulus() {
 
 #[test]
 fn test_ed25519_group() {
-  ff_group_tests::group::test_prime_group_bits::<EdwardsPoint>();
+  ff_group_tests::group::test_prime_group_bits::<_, EdwardsPoint>(&mut rand_core::OsRng);
 }
 
 #[test]
 fn test_ristretto_group() {
-  ff_group_tests::group::test_prime_group_bits::<RistrettoPoint>();
+  ff_group_tests::group::test_prime_group_bits::<_, RistrettoPoint>(&mut rand_core::OsRng);
 }

crypto/ed448/src/field.rs

@@ -32,5 +32,5 @@ field!(FieldElement, MODULUS, WIDE_MODULUS, 448);
 #[test]
 fn test_field() {
   // TODO: Move to test_prime_field_bits once the impl is finished
-  ff_group_tests::prime_field::test_prime_field::<FieldElement>();
+  ff_group_tests::prime_field::test_prime_field::<_, FieldElement>(&mut rand_core::OsRng);
 }

crypto/ed448/src/point.rs

@@ -323,6 +323,7 @@ fn test_group() {
   test_sub::<Point>();
   test_mul::<Point>();
   test_order::<Point>();
+  test_random::<_, Point>(&mut rand_core::OsRng);
 
   test_encoding::<Point>();
 }

crypto/ed448/src/scalar.rs

@@ -35,5 +35,5 @@ impl Scalar {
 #[test]
 fn test_scalar_field() {
   // TODO: Move to test_prime_field_bits once the impl is finished
-  ff_group_tests::prime_field::test_prime_field::<Scalar>();
+  ff_group_tests::prime_field::test_prime_field::<_, Scalar>(&mut rand_core::OsRng);
 }

crypto/ff-group-tests/Cargo.toml

@@ -13,6 +13,7 @@ all-features = true
 rustdoc-args = ["--cfg", "docsrs"]
 
 [dependencies]
+rand_core = "0.6"
 group = "0.12"
 
 [dev-dependencies]

crypto/ff-group-tests/src/field.rs

@@ -1,3 +1,4 @@
+use rand_core::RngCore;
 use group::ff::Field;
 
 /// Perform basic tests on equality.
@@ -106,8 +107,27 @@ pub fn test_cube<F: Field>() {
   assert_eq!(two.cube(), two * two * two, "2^3 != 8");
 }
 
+/// Test random.
+pub fn test_random<R: RngCore, F: Field>(rng: &mut R) {
+  let a = F::random(&mut *rng);
+
+  // Run up to 128 times so small fields, which may occasionally return the same element twice,
+  // are statistically unlikely to fail
+  // Field of order 1 will always fail this test due to lack of distinct elements to sample
+  // from
+  let mut pass = false;
+  for _ in 0 .. 128 {
+    let b = F::random(&mut *rng);
+    // This test passes if a distinct element is returned at least once
+    if b != a {
+      pass = true;
+    }
+  }
+  assert!(pass, "random always returned the same value");
+}
+
 /// Run all tests on fields implementing Field.
-pub fn test_field<F: Field>() {
+pub fn test_field<R: RngCore, F: Field>(rng: &mut R) {
   test_eq::<F>();
   test_conditional_select::<F>();
   test_add::<F>();
@@ -119,4 +139,5 @@ pub fn test_field<F: Field>() {
   test_sqrt::<F>();
   test_is_zero::<F>();
   test_cube::<F>();
+  test_random::<R, F>(rng);
 }

crypto/ff-group-tests/src/group.rs

@@ -1,3 +1,4 @@
+use rand_core::RngCore;
 use group::{
   ff::{Field, PrimeFieldBits},
   Group,
@@ -69,6 +70,11 @@ pub fn test_sum<G: Group>() {
     G::generator().double(),
     "[generator, generator].sum() != two"
   );
+  assert_eq!(
+    [G::generator().double(), G::generator()].iter().sum::<G>(),
+    G::generator().double() + G::generator(),
+    "[generator.double(), generator].sum() != three"
+  );
 }
 
 /// Test negation.
@@ -107,9 +113,31 @@ pub fn test_order<G: Group>() {
   assert_eq!(minus_one + G::generator(), G::identity(), "((modulus - 1) * G) + G wasn't identity");
 }
 
+/// Test random.
+pub fn test_random<R: RngCore, G: Group>(rng: &mut R) {
+  let a = G::random(&mut *rng);
+  assert!(!bool::from(a.is_identity()), "random returned identity");
+
+  // Run up to 128 times so small groups, which may occasionally return the same element twice,
+  // are statistically unlikely to fail
+  // Groups of order <= 2 will always fail this test due to lack of distinct elements to sample
+  // from
+  let mut pass = false;
+  for _ in 0 .. 128 {
+    let b = G::random(&mut *rng);
+    assert!(!bool::from(b.is_identity()), "random returned identity");
+
+    // This test passes if a distinct element is returned at least once
+    if b != a {
+      pass = true;
+    }
+  }
+  assert!(pass, "random always returned the same value");
+}
+
 /// Run all tests on groups implementing Group.
-pub fn test_group<G: Group>() {
+pub fn test_group<R: RngCore, G: Group>(rng: &mut R) {
-  test_prime_field::<G::Scalar>();
+  test_prime_field::<R, G::Scalar>(rng);
 
   test_eq::<G>();
   test_identity::<G>();
@@ -121,6 +149,7 @@ pub fn test_group<G: Group>() {
   test_sub::<G>();
   test_mul::<G>();
   test_order::<G>();
+  test_random::<R, G>(rng);
 }
 
 /// Test encoding and decoding of group elements.
@@ -142,19 +171,19 @@ pub fn test_encoding<G: PrimeGroup>() {
 }
 
 /// Run all tests on groups implementing PrimeGroup (Group + GroupEncoding).
-pub fn test_prime_group<G: PrimeGroup>() {
+pub fn test_prime_group<R: RngCore, G: PrimeGroup>(rng: &mut R) {
-  test_group::<G>();
+  test_group::<R, G>(rng);
 
   test_encoding::<G>();
 }
 
 /// Run all tests offered by this crate on the group.
-pub fn test_prime_group_bits<G: PrimeGroup>()
+pub fn test_prime_group_bits<R: RngCore, G: PrimeGroup>(rng: &mut R)
 where
   G::Scalar: PrimeFieldBits,
 {
-  test_prime_field_bits::<G::Scalar>();
+  test_prime_field_bits::<R, G::Scalar>(rng);
-  test_prime_group::<G>();
+  test_prime_group::<R, G>(rng);
 }
 
 // Run these tests against k256/p256
@@ -167,10 +196,10 @@ where
 
 #[test]
 fn test_k256() {
-  test_prime_group_bits::<k256::ProjectivePoint>();
+  test_prime_group_bits::<_, k256::ProjectivePoint>(&mut rand_core::OsRng);
 }
 
 #[test]
 fn test_p256() {
-  test_prime_group_bits::<p256::ProjectivePoint>();
+  test_prime_group_bits::<_, p256::ProjectivePoint>(&mut rand_core::OsRng);
 }

crypto/ff-group-tests/src/prime_field.rs

@@ -1,3 +1,4 @@
+use rand_core::RngCore;
 use group::ff::{PrimeField, PrimeFieldBits};
 
 use crate::field::test_field;
@@ -29,6 +30,16 @@ pub fn test_is_odd<F: PrimeField>() {
   assert_eq!(F::one().is_odd().unwrap_u8(), 1, "1 was even");
   assert_eq!(F::one().is_even().unwrap_u8(), 0, "1 wasn't odd");
 
+  // Make sure an odd value added to an odd value is even
+  let two = F::one().double();
+  assert_eq!(two.is_odd().unwrap_u8(), 0, "2 was odd");
+  assert_eq!(two.is_even().unwrap_u8(), 1, "2 wasn't even");
+
+  // Make sure an even value added to an even value is even
+  let four = two.double();
+  assert_eq!(four.is_odd().unwrap_u8(), 0, "4 was odd");
+  assert_eq!(four.is_even().unwrap_u8(), 1, "4 wasn't even");
+
   let neg_one = -F::one();
   assert_eq!(neg_one.is_odd().unwrap_u8(), 0, "-1 was odd");
   assert_eq!(neg_one.is_even().unwrap_u8(), 1, "-1 wasn't even");
@@ -49,6 +60,11 @@ pub fn test_encoding<F: PrimeField>() {
       F::from_repr_vartime(repr).unwrap(),
       "{msg} couldn't be encoded and decoded",
     );
+    assert_eq!(
+      bytes.as_ref(),
+      F::from_repr(repr).unwrap().to_repr().as_ref(),
+      "canonical encoding decoded produced distinct encoding"
+    );
   };
   test(F::zero(), "0");
   test(F::one(), "1");
@@ -57,8 +73,8 @@ pub fn test_encoding<F: PrimeField>() {
 }
 
 /// Run all tests on fields implementing PrimeField.
-pub fn test_prime_field<F: PrimeField>() {
+pub fn test_prime_field<R: RngCore, F: PrimeField>(rng: &mut R) {
-  test_field::<F>();
+  test_field::<R, F>(rng);
 
   test_zero::<F>();
   test_one::<F>();
@@ -265,6 +281,7 @@ pub fn test_root_of_unity<F: PrimeFieldBits>() {
     }
     bit = bit.double();
   }
+  assert!(bool::from(t.is_odd()), "t wasn't odd");
 
   assert_eq!(pow(F::multiplicative_generator(), t), F::root_of_unity(), "incorrect root of unity");
   assert_eq!(
@@ -275,8 +292,8 @@ pub fn test_root_of_unity<F: PrimeFieldBits>() {
 }
 
 /// Run all tests on fields implementing PrimeFieldBits.
-pub fn test_prime_field_bits<F: PrimeFieldBits>() {
+pub fn test_prime_field_bits<R: RngCore, F: PrimeFieldBits>(rng: &mut R) {
-  test_prime_field::<F>();
+  test_prime_field::<R, F>(rng);
 
   test_to_le_bits::<F>();
   test_char_le_bits::<F>();