Clarify FROST's hash functions

Updates the keygen challenge to a format not vulnerable to collisions 
due to having multiple variable length elements.

coins/monero/src/frost.rs

@@ -59,13 +59,17 @@ impl Curve for Ed25519 {
     true
   }
 
-  // This, as used by CLSAG, will already be a keccak256 hash
-  // The only necessity is for this to be unique, which means skipping a hash here should be fine accordingly
-  // TODO: Decide
+  // This will already be a keccak256 hash in the case of CLSAG signing, making it fine to simply
+  // return as-is, yet this ensures it's fixed size (a security requirement) and unique regardless
+  // of how it's called/what it's called with
   fn hash_msg(msg: &[u8]) -> Vec<u8> {
     Blake2b512::digest(msg).to_vec()
   }
 
+  fn hash_binding_factor(binding: &[u8]) -> Self::F {
+    Self::hash_to_F(&[b"rho", binding].concat())
+  }
+
   fn hash_to_F(data: &[u8]) -> Self::F {
     dfg::Scalar::from_hash(Blake2b512::new().chain(data))
   }

crypto/frost/src/key_gen.rs

@@ -14,15 +14,10 @@ use crate::{
 };
 
 #[allow(non_snake_case)]
-fn challenge<C: Curve>(l: u16, context: &str, R: &[u8], Am: &[u8]) -> C::F {
-  let mut c = Vec::with_capacity(2 + context.len() + R.len() + Am.len());
-  c.extend(l.to_be_bytes());
-  c.extend(context.as_bytes());
-  c.extend(R);  // R
-  c.extend(Am); // A of the first commitment, which is what we're proving we have the private key
-                // for
-                // m of the rest of the commitments, authenticating them
-  C::hash_to_F(&c)
+fn challenge<C: Curve>(context: &str, l: u16, R: &[u8], Am: &[u8]) -> C::F {
+  const DST: &'static [u8] = b"FROST Schnorr Proof of Knowledge";
+  // Uses hash_msg to get a fixed size value out of the context string
+  C::hash_to_F(&[DST, &C::hash_msg(context.as_bytes()), &l.to_be_bytes(), R, Am].concat())
 }
 
 // Implements steps 1 through 3 of round 1 of FROST DKG. Returns the coefficients, commitments, and
@@ -57,8 +52,8 @@ fn generate_key_r1<R: RngCore + CryptoRng, C: Curve>(
       // general obsession with canonicity and determinism though
       r,
       challenge::<C>(
-        params.i(),
         context,
+        params.i(),
         &C::G_to_bytes(&(C::generator_table() * r)),
         &serialized
       )
@@ -116,7 +111,7 @@ fn verify_r1<R: RngCore + CryptoRng, C: Curve>(
       signatures.push((
         l,
         these_commitments[0],
-        challenge::<C>(l, context, R_bytes(l), Am(l)),
+        challenge::<C>(context, l, R_bytes(l), Am(l)),
         SchnorrSignature::<C> { R: R(l)?, s: s(l)? }
       ));
     }

crypto/frost/src/lib.rs

@@ -58,25 +58,29 @@ pub trait Curve: Clone + Copy + PartialEq + Eq + Debug {
   /// If little endian is used for the scalar field's Repr
   fn little_endian() -> bool;
 
-  /// Hash the message as needed to calculate the binding factor
-  /// H3 from the IETF draft
+  /// Hash the message for the binding factor. H3 from the IETF draft
   // This doesn't actually need to be part of Curve as it does nothing with the curve
   // This also solely relates to FROST and with a proper Algorithm/HRAM, all projects using
-  // aggregatable signatures over this curve will work without issue, albeit potentially with
-  // incompatibilities between FROST implementations
-  // It is kept here as Curve + HRAM is effectively a ciphersuite according to the IETF draft
+  // aggregatable signatures over this curve will work without issue
+  // It is kept here as Curve + H{1, 2, 3} is effectively a ciphersuite according to the IETF draft
   // and moving it to Schnorr would force all of them into being ciphersuite-specific
+  // H2 is left to the Schnorr Algorithm as H2 is the H used in HRAM, which Schnorr further
+  // modularizes
   fn hash_msg(msg: &[u8]) -> Vec<u8>;
 
-  /// Field element from hash, used in key generation and to calculate the binding factor
-  /// H1 from the IETF draft
-  /// Key generation uses it as if it's H2 to generate a challenge for a Proof of Knowledge
-  #[allow(non_snake_case)]
-  fn hash_to_F(data: &[u8]) -> Self::F;
+  /// Hash the commitments and message to calculate the binding factor. H1 from the IETF draft
+  fn hash_binding_factor(binding: &[u8]) -> Self::F;
 
   // The following methods would optimally be F:: and G:: yet developers can't control F/G
   // They can control a trait they pass into this library
 
+  /// Field element from hash. Used during key gen and by other crates under Serai as a general
+  /// utility
+  // Not parameterized by Digest as it's fine for it to use its own hash function as relevant to
+  // hash_msg and hash_binding_factor
+  #[allow(non_snake_case)]
+  fn hash_to_F(data: &[u8]) -> Self::F;
+
   /// Constant size of a serialized field element
   // The alternative way to grab this would be either serializing a junk element and getting its
   // length or doing a naive division of its BITS property by 8 and assuming a lack of padding

crypto/frost/src/sign.rs

@@ -161,7 +161,7 @@ fn sign_with_share<C: Curve, A: Algorithm<C>>(
     transcript.append_message(b"message", &C::hash_msg(&msg));
 
     // Calculate the binding factor
-    C::hash_to_F(&transcript.challenge(b"binding"))
+    C::hash_binding_factor(&transcript.challenge(b"binding"))
   };
 
   // Process the addendums

crypto/frost/src/tests/literal/secp256k1.rs

@@ -48,11 +48,13 @@ impl Curve for Secp256k1 {
     (&Sha256::digest(msg)).to_vec()
   }
 
+  fn hash_binding_factor(binding: &[u8]) -> Self::F {
+    Self::hash_to_F(&[b"rho", binding].concat())
+  }
+
   // Use wide reduction for security
   fn hash_to_F(data: &[u8]) -> Self::F {
-    Scalar::from_uint_reduced(
-      U512::from_be_byte_array(Sha512::new().chain_update("rho").chain_update(data).finalize())
-    )
+    Scalar::from_uint_reduced(U512::from_be_byte_array(Sha512::digest(data)))
   }
 
   fn F_len() -> usize {