From da8e7e73e08b7191a777460133ca2e7d37698a47 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 24 Dec 2022 17:08:22 -0500 Subject: [PATCH] Re-organize testing strategy and document Ciphersuite::hash_to_F. --- crypto/ciphersuite/Cargo.toml | 3 +++ crypto/ciphersuite/README.md | 32 +++++++++++++++++++++++++++++++ crypto/ciphersuite/src/dalek.rs | 10 ++++++++++ crypto/ciphersuite/src/ed448.rs | 6 ++++++ crypto/ciphersuite/src/kp256.rs | 10 ++++++++++ crypto/ciphersuite/src/lib.rs | 9 ++++++++- crypto/dkg/Cargo.toml | 3 +++ crypto/dkg/src/tests/mod.rs | 5 +++++ crypto/dleq/src/tests/mod.rs | 32 +++++++++++++++++++++++++++++++ crypto/ed448/README.md | 5 +++-- crypto/frost/src/tests/curve.rs | 27 -------------------------- crypto/frost/src/tests/mod.rs | 2 -- crypto/frost/src/tests/vectors.rs | 10 ++-------- 13 files changed, 114 insertions(+), 40 deletions(-) delete mode 100644 crypto/frost/src/tests/curve.rs diff --git a/crypto/ciphersuite/Cargo.toml b/crypto/ciphersuite/Cargo.toml index b0746f9e..54176d17 100644 --- a/crypto/ciphersuite/Cargo.toml +++ b/crypto/ciphersuite/Cargo.toml @@ -33,6 +33,9 @@ k256 = { version = "0.11", features = ["arithmetic", "bits", "hash2curve"], opti minimal-ed448 = { path = "../ed448", version = "^0.1.2", optional = true } +[dev-dependencies] +ff-group-tests = { version = "0.12", path = "../ff-group-tests" } + [features] std = [] diff --git a/crypto/ciphersuite/README.md b/crypto/ciphersuite/README.md index db85fda2..a5c28dd3 100644 --- a/crypto/ciphersuite/README.md +++ b/crypto/ciphersuite/README.md @@ -1,3 +1,35 @@ # Ciphersuite Ciphersuites for elliptic curves premised on ff/group. + +### Secp256k1/P-256 + +Secp256k1 and P-256 are offered via [k256](https://crates.io/crates/k256) and +[p256](https://crates.io/crates/p256), two libraries maintained by +[RustCrypto](https://github.com/RustCrypto). + +Their `hash_to_F` is the +[IETF's hash to curve](https://www.ietf.org/archive/id/draft-irtf-cfrg-hash-to-curve-16.html), +yet applied to their scalar field. + +### Ed25519/Ristretto + +Ed25519/Ristretto are offered via +[dalek-ff-group](https://crates.io/crates/dalek-ff-group), an ff/group wrapper +around [curve25519-dalek](https://crates.io/crates/curve25519-dalek). + +Their `hash_to_F` is the wide reduction of SHA2-512, as used in +[RFC 8032](https://www.rfc-editor.org/rfc/rfc8032). This is also compliant with +the draft +[RFC RISTRETTO](https://www.ietf.org/archive/id/draft-rtf-cfrg-ristretto255-decaf448-05.html). +The domain-separation tag is naively prefixed to the message. + +### Ed448 + +Ed448 is offered via [minimal-ed448](https://crates.io/crates/minimal-ed448), an +explicitly not recommended Ed448 implementation, limited to its prime-order +subgroup. + +Its `hash_to_F` is the wide reduction of SHAKE256, with a 114-byte output, as +used in [RFC 8032](https://www.rfc-editor.org/rfc/rfc8032). The +domain-separation tag is naively prefixed to the message. diff --git a/crypto/ciphersuite/src/dalek.rs b/crypto/ciphersuite/src/dalek.rs index 48b968eb..e94837d1 100644 --- a/crypto/ciphersuite/src/dalek.rs +++ b/crypto/ciphersuite/src/dalek.rs @@ -39,6 +39,16 @@ macro_rules! dalek_curve { #[cfg(any(test, feature = "ristretto"))] dalek_curve!("ristretto", Ristretto, RistrettoPoint, b"ristretto"); +#[cfg(any(test, feature = "ristretto"))] +#[test] +fn test_ristretto() { + ff_group_tests::group::test_prime_group_bits::(); +} #[cfg(feature = "ed25519")] dalek_curve!("ed25519", Ed25519, EdwardsPoint, b"edwards25519"); +#[cfg(feature = "ed25519")] +#[test] +fn test_ed25519() { + ff_group_tests::group::test_prime_group_bits::(); +} diff --git a/crypto/ciphersuite/src/ed448.rs b/crypto/ciphersuite/src/ed448.rs index 43b88ef3..9300a4b7 100644 --- a/crypto/ciphersuite/src/ed448.rs +++ b/crypto/ciphersuite/src/ed448.rs @@ -65,3 +65,9 @@ impl Ciphersuite for Ed448 { Scalar::wide_reduce(Self::H::digest([dst, data].concat()).as_ref().try_into().unwrap()) } } + +#[test] +fn test_ed448() { + // TODO: Enable once ed448 passes these tests + //ff_group_tests::group::test_prime_group_bits::(); +} diff --git a/crypto/ciphersuite/src/kp256.rs b/crypto/ciphersuite/src/kp256.rs index 26b16bc9..41a7c16a 100644 --- a/crypto/ciphersuite/src/kp256.rs +++ b/crypto/ciphersuite/src/kp256.rs @@ -67,6 +67,16 @@ macro_rules! kp_curve { #[cfg(feature = "p256")] kp_curve!("p256", p256, P256, b"P-256"); +#[cfg(feature = "p256")] +#[test] +fn test_p256() { + ff_group_tests::group::test_prime_group_bits::(); +} #[cfg(feature = "secp256k1")] kp_curve!("secp256k1", k256, Secp256k1, b"secp256k1"); +#[cfg(feature = "secp256k1")] +#[test] +fn test_secp256k1() { + ff_group_tests::group::test_prime_group_bits::(); +} diff --git a/crypto/ciphersuite/src/lib.rs b/crypto/ciphersuite/src/lib.rs index 68680d66..099f38b8 100644 --- a/crypto/ciphersuite/src/lib.rs +++ b/crypto/ciphersuite/src/lib.rs @@ -58,7 +58,14 @@ pub trait Ciphersuite: Clone + Copy + PartialEq + Eq + Debug + Zeroize { // While group does provide this in its API, privacy coins may want to use a custom basepoint fn generator() -> Self::G; - /// Hash the provided dst and message to a scalar. + /// Hash the provided domain-separation tag and message to a scalar. Ciphersuites MAY naively + /// prefix the tag to the message, enabling transpotion between the two. Accordingly, this + /// function should NOT be used in any scheme where one tag is a valid substring of another + /// UNLESS the specific Ciphersuite is verified to handle the DST securely. + /// + /// Verifying specific ciphersuites have secure tag handling is not recommended, due to it + /// breaking the intended modularity of ciphersuites. Instead, component-specific tags with + /// further purpose tags are recommended ("Schnorr-nonce", "Schnorr-chal"). #[allow(non_snake_case)] fn hash_to_F(dst: &[u8], msg: &[u8]) -> Self::F; diff --git a/crypto/dkg/Cargo.toml b/crypto/dkg/Cargo.toml index a63d5bb1..fbdcba75 100644 --- a/crypto/dkg/Cargo.toml +++ b/crypto/dkg/Cargo.toml @@ -32,5 +32,8 @@ ciphersuite = { path = "../ciphersuite", version = "0.1", features = ["std"] } schnorr = { package = "schnorr-signatures", path = "../schnorr", version = "0.2" } dleq = { path = "../dleq", version = "0.2", features = ["serialize"] } +[dev-dependencies] +ciphersuite = { path = "../ciphersuite", version = "0.1", features = ["std", "ristretto"] } + [features] tests = [] diff --git a/crypto/dkg/src/tests/mod.rs b/crypto/dkg/src/tests/mod.rs index 999af228..89957315 100644 --- a/crypto/dkg/src/tests/mod.rs +++ b/crypto/dkg/src/tests/mod.rs @@ -68,3 +68,8 @@ pub fn test_ciphersuite(rng: &mut R) { key_gen::<_, C>(rng); test_generator_promotion::<_, C>(rng); } + +#[test] +fn test_with_ristretto() { + test_ciphersuite::<_, ciphersuite::Ristretto>(&mut rand_core::OsRng); +} diff --git a/crypto/dleq/src/tests/mod.rs b/crypto/dleq/src/tests/mod.rs index 099c0d4e..ac3ff11e 100644 --- a/crypto/dleq/src/tests/mod.rs +++ b/crypto/dleq/src/tests/mod.rs @@ -52,6 +52,38 @@ fn test_dleq() { keys[k] = generators[k] * key.deref(); } proof.verify(&mut transcript(), &generators[.. i], &keys[.. i]).unwrap(); + // Different challenge + assert!(proof + .verify( + &mut RecommendedTranscript::new(b"different challenge"), + &generators[.. i], + &keys[.. i] + ) + .is_err()); + + // We could edit these tests to always test with at least two generators + // Then we don't test proofs with zero/one generator(s) + // While those are stupid, and pointless, and potentially point to a failure in the caller, + // it could also be part of a dynamic system which deals with variable amounts of generators + // Not panicking in such use cases, even if they're inefficient, provides seamless behavior + if i >= 2 { + // Different generators + assert!(proof + .verify( + &mut transcript(), + generators[.. i].iter().cloned().rev().collect::>().as_ref(), + &keys[.. i] + ) + .is_err()); + // Different keys + assert!(proof + .verify( + &mut transcript(), + &generators[.. i], + keys[.. i].iter().cloned().rev().collect::>().as_ref() + ) + .is_err()); + } #[cfg(feature = "serialize")] { diff --git a/crypto/ed448/README.md b/crypto/ed448/README.md index ac9270e6..1d9d248a 100644 --- a/crypto/ed448/README.md +++ b/crypto/ed448/README.md @@ -3,7 +3,8 @@ Inefficient, barebones implementation of Ed448 bound to the ff/group API, rejecting torsion to achieve a PrimeGroup definition. This likely should not be used and was only done so another library under Serai could confirm its -completion. It is minimally tested, yet should be correct for what it has. -Multiple functions remain unimplemented. +completion. It is minimally tested, yet should be correct for what it has. The +functions it doesn't have are marked `unimplemented!()`. This has not undergone +auditing. constant time and no_std. diff --git a/crypto/frost/src/tests/curve.rs b/crypto/frost/src/tests/curve.rs deleted file mode 100644 index 980f2435..00000000 --- a/crypto/frost/src/tests/curve.rs +++ /dev/null @@ -1,27 +0,0 @@ -use rand_core::{RngCore, CryptoRng}; - -use group::Group; - -use crate::Curve; - -// Test successful multiexp, with enough pairs to trigger its variety of algorithms -// Multiexp has its own tests, yet only against k256 and Ed25519 (which should be sufficient -// as-is to prove multiexp), and this doesn't hurt -pub fn test_multiexp(rng: &mut R) { - let mut pairs = Vec::with_capacity(1000); - let mut sum = C::G::identity(); - for _ in 0 .. 10 { - for _ in 0 .. 100 { - pairs.push((C::random_nonzero_F(&mut *rng), C::generator() * C::random_nonzero_F(&mut *rng))); - sum += pairs[pairs.len() - 1].1 * pairs[pairs.len() - 1].0; - } - assert_eq!(multiexp::multiexp(&pairs), sum); - assert_eq!(multiexp::multiexp_vartime(&pairs), sum); - } -} - -pub fn test_curve(rng: &mut R) { - // TODO: Test the Curve functions themselves - - test_multiexp::<_, C>(rng); -} diff --git a/crypto/frost/src/tests/mod.rs b/crypto/frost/src/tests/mod.rs index 927864fa..3e3cfd48 100644 --- a/crypto/frost/src/tests/mod.rs +++ b/crypto/frost/src/tests/mod.rs @@ -10,8 +10,6 @@ use crate::{ sign::{Writable, PreprocessMachine, SignMachine, SignatureMachine, AlgorithmMachine}, }; -/// Curve tests. -pub mod curve; /// Vectorized test suite to ensure consistency. pub mod vectors; diff --git a/crypto/frost/src/tests/vectors.rs b/crypto/frost/src/tests/vectors.rs index f81ec7cc..3f6223c8 100644 --- a/crypto/frost/src/tests/vectors.rs +++ b/crypto/frost/src/tests/vectors.rs @@ -9,7 +9,7 @@ use rand_core::{RngCore, CryptoRng}; use group::{ff::PrimeField, GroupEncoding}; -use dkg::tests::{key_gen, test_ciphersuite as test_dkg}; +use dkg::tests::key_gen; use crate::{ curve::Curve, @@ -19,7 +19,7 @@ use crate::{ Nonce, GeneratorCommitments, NonceCommitments, Commitments, Writable, Preprocess, SignMachine, SignatureMachine, AlgorithmMachine, }, - tests::{clone_without, recover_key, algorithm_machines, sign, curve::test_curve}, + tests::{clone_without, recover_key, algorithm_machines, sign}, }; pub struct Vectors { @@ -118,12 +118,6 @@ pub fn test_with_vectors>( rng: &mut R, vectors: Vectors, ) { - // Do basic tests before trying the vectors - test_curve::<_, C>(&mut *rng); - - // Test the DKG - test_dkg::<_, C>(&mut *rng); - // Test a basic Schnorr signature { let keys = key_gen(&mut *rng);