From 7d972e2b16fe57f4dfa4a9549d2fbb9306411704 Mon Sep 17 00:00:00 2001
From: Luke Parker <lukeparker5132@gmail.com>
Date: Mon, 6 Jan 2025 15:39:19 -0500
Subject: [PATCH] Don't have the Helioselene Ciphersuite use the provided
 read_G

The provided read_G compares the point's serialization against its
reserialization to check if the encoding was malleated. helioselene already
checks the point's encoded x coordinate is reduced and the point's sign bit
isn't for the representation of identity, preventing malleation. We accordingly
don't need this additional check which is expensive.
---
 crypto/ciphersuite/src/helioselene.rs | 32 +++++++++++++++++++++++++--
 crypto/ciphersuite/src/lib.rs         |  3 +++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/crypto/ciphersuite/src/helioselene.rs b/crypto/ciphersuite/src/helioselene.rs
index 9b74e9e5..f64ff28e 100644
--- a/crypto/ciphersuite/src/helioselene.rs
+++ b/crypto/ciphersuite/src/helioselene.rs
@@ -1,9 +1,11 @@
+#[cfg(any(feature = "alloc", feature = "std"))]
+use std_shims::io::{self, Read};
+
 use zeroize::Zeroize;
 
 use blake2::{Digest, Blake2b512};
 
-use group::Group;
-
+use group::{Group, GroupEncoding};
 use helioselene::{Field25519, HelioseleneField, HeliosPoint, SelenePoint};
 
 use crate::Ciphersuite;
@@ -37,6 +39,19 @@ impl Ciphersuite for Helios {
     uniform.zeroize();
     res
   }
+
+  // We override the provided impl, which compares against the reserialization, because
+  // Helios::G::from_bytes already enforces canonically encoded points
+  #[cfg(any(feature = "alloc", feature = "std"))]
+  #[allow(non_snake_case)]
+  fn read_G<R: Read>(reader: &mut R) -> io::Result<Self::G> {
+    let mut encoding = <Self::G as GroupEncoding>::Repr::default();
+    reader.read_exact(encoding.as_mut())?;
+
+    let point = Option::<Self::G>::from(Self::G::from_bytes(&encoding))
+      .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "invalid point"))?;
+    Ok(point)
+  }
 }
 
 #[derive(Clone, Copy, PartialEq, Eq, Debug, Zeroize)]
@@ -68,6 +83,19 @@ impl Ciphersuite for Selene {
     uniform.zeroize();
     res
   }
+
+  // We override the provided impl, which compares against the reserialization, because
+  // Selene::G::from_bytes already enforces canonically encoded points
+  #[cfg(any(feature = "alloc", feature = "std"))]
+  #[allow(non_snake_case)]
+  fn read_G<R: Read>(reader: &mut R) -> io::Result<Self::G> {
+    let mut encoding = <Self::G as GroupEncoding>::Repr::default();
+    reader.read_exact(encoding.as_mut())?;
+
+    let point = Option::<Self::G>::from(Self::G::from_bytes(&encoding))
+      .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "invalid point"))?;
+    Ok(point)
+  }
 }
 
 #[test]
diff --git a/crypto/ciphersuite/src/lib.rs b/crypto/ciphersuite/src/lib.rs
index d338c8ba..25b8b2f6 100644
--- a/crypto/ciphersuite/src/lib.rs
+++ b/crypto/ciphersuite/src/lib.rs
@@ -111,6 +111,9 @@ pub trait Ciphersuite:
   }
 
   /// Read a canonical point from something implementing std::io::Read.
+  ///
+  /// The provided implementation is safe so long as `GroupEncoding::to_bytes` always returns a
+  /// canonical serialization.
   #[cfg(any(feature = "alloc", feature = "std"))]
   #[allow(non_snake_case)]
   fn read_G<R: Read>(reader: &mut R) -> io::Result<Self::G> {