From ee6316b26b0e2397ee2fbb617997ee20f724d227 Mon Sep 17 00:00:00 2001
From: Luke Parker <lukeparker5132@gmail.com>
Date: Mon, 29 Aug 2022 13:02:20 -0400
Subject: [PATCH] Use a Group::random which doesn't have a known DL

While Group::random shouldn't be used instead of a hash to curve, anyone
who did would've previously been insecure and now isn't.

Could've done a recover_x and a raw Point construction, followed by a
cofactor mul, to avoid the serialization, yet the serialization ensures
full validity under the standard from_bytes function. THis also doesn't
need to be micro-optimized.
---
 crypto/dalek-ff-group/src/lib.rs | 13 +++++++++----
 crypto/ed448/src/point.rs        | 19 +++++++++++++++----
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/crypto/dalek-ff-group/src/lib.rs b/crypto/dalek-ff-group/src/lib.rs
index 93628246..44efedfa 100644
--- a/crypto/dalek-ff-group/src/lib.rs
+++ b/crypto/dalek-ff-group/src/lib.rs
@@ -311,10 +311,15 @@ macro_rules! dalek_group {
 
     impl Group for $Point {
       type Scalar = Scalar;
-      // Ideally, this would be cryptographically secure, yet that's not a bound on the trait
-      // k256 also does this
-      fn random(rng: impl RngCore) -> Self {
-        &$BASEPOINT_TABLE * Scalar::random(rng)
+      fn random(mut rng: impl RngCore) -> Self {
+        loop {
+          let mut bytes = field::FieldElement::random(&mut rng).to_repr();
+          bytes[31] |= u8::try_from(rng.next_u32() % 2).unwrap() << 7;
+          let opt = Self::from_bytes(&bytes);
+          if opt.is_some().into() {
+            return opt.unwrap();
+          }
+        }
       }
       fn identity() -> Self {
         Self($DPoint::identity())
diff --git a/crypto/ed448/src/point.rs b/crypto/ed448/src/point.rs
index 02089b46..8f9d1b10 100644
--- a/crypto/ed448/src/point.rs
+++ b/crypto/ed448/src/point.rs
@@ -165,10 +165,16 @@ impl SubAssign<&Point> for Point {
 
 impl Group for Point {
   type Scalar = Scalar;
-  // Ideally, this would be cryptographically secure, yet that's not a bound on the trait
-  // k256 also does this
-  fn random(rng: impl RngCore) -> Self {
-    Self::generator() * Scalar::random(rng)
+  fn random(mut rng: impl RngCore) -> Self {
+    loop {
+      let mut bytes = FieldElement::random(&mut rng).to_repr();
+      let mut_ref: &mut [u8] = bytes.as_mut();
+      mut_ref[56] |= u8::try_from(rng.next_u32() % 2).unwrap() << 7;
+      let opt = Self::from_bytes(&bytes);
+      if opt.is_some().into() {
+        return opt.unwrap();
+      }
+    }
   }
   fn identity() -> Self {
     Point { x: FieldElement::zero(), y: FieldElement::one(), z: FieldElement::one() }
@@ -378,3 +384,8 @@ a401cd9df24632adfe6b418dc942d8a091817dd8bd70e1c72ba52f3c\
     .unwrap()
   );
 }
+
+#[test]
+fn random() {
+  Point::random(&mut rand_core::OsRng);
+}