From 244d1b6b6832bdd4ab99508f69c659de81ddc704 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 28 May 2022 20:34:44 -0400 Subject: [PATCH] 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 | 10 ++++++--- crypto/frost/src/key_gen.rs | 17 ++++++--------- crypto/frost/src/lib.rs | 24 ++++++++++++--------- crypto/frost/src/sign.rs | 2 +- crypto/frost/src/tests/literal/secp256k1.rs | 8 ++++--- 5 files changed, 33 insertions(+), 28 deletions(-) diff --git a/coins/monero/src/frost.rs b/coins/monero/src/frost.rs index 265a86eb..ad31c193 100644 --- a/coins/monero/src/frost.rs +++ b/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 { 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)) } diff --git a/crypto/frost/src/key_gen.rs b/crypto/frost/src/key_gen.rs index b63c5d22..ae9f4148 100644 --- a/crypto/frost/src/key_gen.rs +++ b/crypto/frost/src/key_gen.rs @@ -14,15 +14,10 @@ use crate::{ }; #[allow(non_snake_case)] -fn challenge(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(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( // general obsession with canonicity and determinism though r, challenge::( - params.i(), context, + params.i(), &C::G_to_bytes(&(C::generator_table() * r)), &serialized ) @@ -116,7 +111,7 @@ fn verify_r1( signatures.push(( l, these_commitments[0], - challenge::(l, context, R_bytes(l), Am(l)), + challenge::(context, l, R_bytes(l), Am(l)), SchnorrSignature:: { R: R(l)?, s: s(l)? } )); } diff --git a/crypto/frost/src/lib.rs b/crypto/frost/src/lib.rs index 2efb5041..5d2c5f19 100644 --- a/crypto/frost/src/lib.rs +++ b/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; - /// 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 diff --git a/crypto/frost/src/sign.rs b/crypto/frost/src/sign.rs index 05c12181..9b35b935 100644 --- a/crypto/frost/src/sign.rs +++ b/crypto/frost/src/sign.rs @@ -161,7 +161,7 @@ fn sign_with_share>( 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 diff --git a/crypto/frost/src/tests/literal/secp256k1.rs b/crypto/frost/src/tests/literal/secp256k1.rs index 4d40d689..c1f9223a 100644 --- a/crypto/frost/src/tests/literal/secp256k1.rs +++ b/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 {