From 681010f422c41b562ba2d614a39f369210cbd6a7 Mon Sep 17 00:00:00 2001
From: Luke Parker <lukeparker5132@gmail.com>
Date: Sun, 28 Jul 2024 01:35:11 -0400
Subject: [PATCH] Ban zero ECDH keys, document non-zero requirements

---
 crypto/dkg/src/evrf/mod.rs   | 11 ++++++++++-
 crypto/dkg/src/evrf/proof.rs | 15 +++++++++++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/crypto/dkg/src/evrf/mod.rs b/crypto/dkg/src/evrf/mod.rs
index 729c7406..37ca7955 100644
--- a/crypto/dkg/src/evrf/mod.rs
+++ b/crypto/dkg/src/evrf/mod.rs
@@ -208,6 +208,8 @@ pub enum EvrfError {
   TooManyParticipants,
   #[error("the threshold t wasn't in range 1 <= t <= n")]
   InvalidThreshold,
+  #[error("a public key was the identity point")]
+  PublicKeyWasIdentity,
   #[error("participating in a DKG we aren't a participant in")]
   NotAParticipant,
   #[error("a participant with an unrecognized ID participated")]
@@ -244,6 +246,8 @@ where
   ///
   /// The context MUST be unique across invocations. Reuse of context will lead to sharing
   /// prior-shared secrets.
+  ///
+  /// Public keys are not allowed to be the identity point. This will error if any are.
   pub fn participate(
     rng: &mut (impl RngCore + CryptoRng),
     generators: &EvrfGenerators<C>,
@@ -257,6 +261,9 @@ where
     if (t == 0) || (t > n) {
       Err(EvrfError::InvalidThreshold)?;
     }
+    if evrf_public_keys.iter().any(|key| bool::from(key.is_identity())) {
+      Err(EvrfError::PublicKeyWasIdentity)?;
+    };
     if !evrf_public_keys.iter().any(|key| *key == evrf_public_key) {
       Err(EvrfError::NotAParticipant)?;
     };
@@ -314,6 +321,9 @@ where
     if (t == 0) || (t > n) {
       Err(EvrfError::InvalidThreshold)?;
     }
+    if evrf_public_keys.iter().any(|key| bool::from(key.is_identity())) {
+      Err(EvrfError::PublicKeyWasIdentity)?;
+    };
     for i in participations.keys() {
       if u16::from(*i) > n {
         Err(EvrfError::NonExistentParticipant)?;
@@ -492,7 +502,6 @@ where
 
         let mut ecdh = Zeroizing::new(C::F::ZERO);
         for point in ecdh_keys {
-          // TODO: Explicitly ban 0-ECDH commitments, 0-eVRF public keys, and gen non-zero keys
           let (mut x, mut y) =
             <C::EmbeddedCurve as Ciphersuite>::G::to_xy(point * evrf_private_key.deref()).unwrap();
           *ecdh += x;
diff --git a/crypto/dkg/src/evrf/proof.rs b/crypto/dkg/src/evrf/proof.rs
index da5e4259..52c72d75 100644
--- a/crypto/dkg/src/evrf/proof.rs
+++ b/crypto/dkg/src/evrf/proof.rs
@@ -38,7 +38,9 @@ fn sample_point<C: Ciphersuite>(rng: &mut (impl RngCore + CryptoRng)) -> C::G {
   loop {
     rng.fill_bytes(repr.as_mut());
     if let Ok(point) = C::read_G(&mut repr.as_ref()) {
-      return point;
+      if bool::from(!point.is_identity()) {
+        return point;
+      }
     }
   }
 }
@@ -595,7 +597,15 @@ where
 
       let mut res = Zeroizing::new(C::F::ZERO);
       for j in 0 .. 2 {
-        let mut ecdh_private_key = <C::EmbeddedCurve as Ciphersuite>::F::random(&mut *rng);
+        let mut ecdh_private_key;
+        loop {
+          ecdh_private_key = <C::EmbeddedCurve as Ciphersuite>::F::random(&mut *rng);
+          // Generate a non-0 ECDH private key, as necessary to not produce an identity output
+          // Identity isn't representable with the divisors, hence the explicit effort
+          if bool::from(!ecdh_private_key.is_zero()) {
+            break;
+          }
+        }
         let mut dlog = Self::scalar_to_bits(&ecdh_private_key);
         let ecdh_commitment = <C::EmbeddedCurve as Ciphersuite>::generator() * ecdh_private_key;
         ecdh_commitments.push(ecdh_commitment);
@@ -798,6 +808,7 @@ where
         transcript.read_point::<C::EmbeddedCurve>().map_err(|_| ())?,
       ];
       ecdh_keys.push(ecdh_keys_i);
+      // This bans zero ECDH keys
       ecdh_keys_xy.push([
         <<C::EmbeddedCurve as Ciphersuite>::G as DivisorCurve>::to_xy(ecdh_keys_i[0]).ok_or(())?,
         <<C::EmbeddedCurve as Ciphersuite>::G as DivisorCurve>::to_xy(ecdh_keys_i[1]).ok_or(())?,