From 757b0aa737b8cfe7e76f8c9098d10d80d4c12c48 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 28 Feb 2024 18:30:21 +0100 Subject: [PATCH 1/6] Remove unsafe cell from ark-secret-scalar --- ark-secret-scalar/Cargo.toml | 6 +- ark-secret-scalar/src/lib.rs | 155 +++++++++++++++++------------------ 2 files changed, 76 insertions(+), 85 deletions(-) diff --git a/ark-secret-scalar/Cargo.toml b/ark-secret-scalar/Cargo.toml index 45e7a42..400cdbc 100644 --- a/ark-secret-scalar/Cargo.toml +++ b/ark-secret-scalar/Cargo.toml @@ -2,7 +2,7 @@ name = "ark-secret-scalar" description = "Secret scalars for non-constant-time fields and curves" authors = ["Jeff Burdges "] -version = "0.0.2" +version = "0.0.3" repository = "https://github.com/w3f/ring-vrf/tree/master/ark-secret-scalar" edition = "2021" license = "MIT/Apache-2.0" @@ -27,9 +27,7 @@ ark-transcript = { version = "0.0.2", default-features = false, path = "../ark-t [dev-dependencies] - -# TODO: Tests -# ark-bls12-377 = { version = "0.4", default-features = false, features = [ "curve" ] } +ark-bls12-377 = { version = "0.4", default-features = false, features = [ "curve" ] } [features] diff --git a/ark-secret-scalar/src/lib.rs b/ark-secret-scalar/src/lib.rs index 84183f2..4047883 100644 --- a/ark-secret-scalar/src/lib.rs +++ b/ark-secret-scalar/src/lib.rs @@ -3,10 +3,7 @@ #![cfg_attr(not(feature = "std"), no_std)] #![doc = include_str!("../README.md")] -use core::{ - cell::UnsafeCell, - ops::{Add,AddAssign,Mul} -}; +use core::ops::{Add,AddAssign,Mul}; use ark_ff::{PrimeField}; // Field, Zero use ark_ec::{AffineRepr, Group}; // CurveGroup @@ -48,71 +45,50 @@ impl XofReader for Rng2Xof { // TODO: We split additively right now, but would a multiplicative splitting // help against rowhammer attacks on the secret key? #[repr(transparent)] -pub struct SecretScalar(UnsafeCell<[F; 2]>); +#[derive(Zeroize)] +pub struct SecretScalar([F; 2]); + +impl From for SecretScalar { + fn from(value: F) -> Self { + Self::from(&value) + } +} + +impl From<&F> for SecretScalar { + fn from(value: &F) -> Self { + let mut xof = Rng2Xof(getrandom_or_panic()); + let v1 = xof_read_reduced(&mut xof); + let v2 = value.clone() - &v1; + SecretScalar([v1, v2]) + } +} impl Clone for SecretScalar { fn clone(&self) -> SecretScalar { - let n = self.operate(|ss| ss.clone()); - self.resplit(); - SecretScalar(UnsafeCell::new(n) ) + let mut secret = SecretScalar(self.0.clone()); + secret.resplit(); + secret } } impl PartialEq for SecretScalar { fn eq(&self, rhs: &SecretScalar) -> bool { - let lhs = unsafe { &*self.0.get() }; - let rhs = unsafe { &*rhs.0.get() }; + let lhs = &self.0; + let rhs = &rhs.0; ( (lhs[0] - rhs[0]) + (lhs[1] - rhs[1]) ).is_zero() } } impl Eq for SecretScalar {} -impl Zeroize for SecretScalar { - fn zeroize(&mut self) { - self.0.get_mut().zeroize() - } -} impl Drop for SecretScalar { fn drop(&mut self) { self.zeroize() } } impl SecretScalar { - /// Do computations with an immutable borrow of the two scalars. - /// - /// At the module level, we keep this method private, never pass - /// these references into user's code, and never accept user's - /// closures, so our being `!Sync` ensures memory safety. - /// All other method ensure only the sum of the scalars is visible - /// outside this module too. - fn operate(&self, f : O) -> R - where O: FnOnce(&[F; 2]) -> R - { - f(unsafe { &*self.0.get() }) - } - - /// Internal clone which skips replit. - fn risky_clone(&self) -> SecretScalar { - let n = self.operate(|ss| ss.clone()); - SecretScalar(UnsafeCell::new(n) ) - } - - /// Immutably borrow `&self` but add opposite random values to its two scalars. - /// - /// We encapsulate exposed interior mutability of `SecretScalar` here, but - /// our other methods should never reveal references into the scalars, - /// or even their individual valus. - pub fn resplit(&self) { + pub fn resplit(&mut self) { let mut xof = Rng2Xof(getrandom_or_panic()); let x = xof_read_reduced(&mut xof); - let selfy = unsafe { &mut *self.0.get() }; - selfy[0] += &x; - selfy[1] -= &x; - } - - pub fn resplit_mut(&mut self) { - let mut xof = Rng2Xof(getrandom_or_panic()); - let x = xof_read_reduced(&mut xof); - let selfy = self.0.get_mut(); + let selfy = &mut self.0; selfy[0] += &x; selfy[1] -= &x; } @@ -120,73 +96,90 @@ impl SecretScalar { /// Initialize and unbiased `SecretScalar` from a `XofReaader`. pub fn from_xof(xof: &mut R) -> Self { let mut xof = || xof_read_reduced(&mut *xof); - let mut ss = SecretScalar(UnsafeCell::new([xof(), xof()]) ); - ss.resplit_mut(); + let mut ss = SecretScalar([xof(), xof()]); + ss.resplit(); ss } /// Multiply by a scalar. pub fn mul_by_challenge(&self, rhs: &F) -> F { - let o = self.operate(|ss| (ss[0] * rhs) + (ss[1] * rhs) ); - self.resplit(); - o + let selfy = &self.clone().0; + (selfy[0] * rhs) + (selfy[1] * rhs) + } + + pub fn scalar(&self) -> F { + self.0[0] + self.0[1] } /// Arkworks multiplies on the right since ark_ff is a dependency of ark_ec. /// but ark_ec being our dependency requires left multiplication here. fn mul_action>(&self, x: &mut G) { let mut y = x.clone(); - self.operate(|ss| { - *x *= ss[0]; - y *= ss[1]; - *x += y; - }); + let selfy = &self.0; + *x *= selfy[0]; + y *= selfy[1]; + *x += y; } } impl AddAssign<&SecretScalar> for SecretScalar { fn add_assign(&mut self, rhs: &SecretScalar) { - let lhs = self.0.get_mut(); - rhs.operate(|rhs| { - lhs[0] += rhs[0]; - lhs[1] += rhs[1]; - }); + let lhs = &mut self.0; + let rhs = &rhs.clone().0; + lhs[0] += rhs[0]; + lhs[1] += rhs[1]; } } impl Add<&SecretScalar> for &SecretScalar { type Output = SecretScalar; + fn add(self, rhs: &SecretScalar) -> SecretScalar { - let mut lhs = self.risky_clone(); + let mut lhs = self.clone(); lhs += rhs; - lhs.resplit_mut(); lhs } } -/* -impl Mul<&G> for &SecretScalar<::ScalarField> { - type Output = G; - /// Arkworks multiplies on the right since ark_ff is a dependency of ark_ec. - /// but ark_ec being our dependency requires left multiplication here. - fn mul(self, rhs: &G) -> G { - let mut rhs = rhs.clone(); - self.mul_action(&mut rhs); - rhs - } -} -*/ - impl Mul<&C> for &SecretScalar<::ScalarField> { type Output = ::Group; + /// Arkworks multiplies on the right since ark_ff is a dependency of ark_ec. /// but ark_ec being our dependency requires left multiplication here. fn mul(self, rhs: &C) -> Self::Output { - let o = self.operate(|lhs| rhs.mul(lhs[0]) + rhs.mul(lhs[1])); + let lhs = &self.clone().0; + let o = rhs.mul(lhs[0]) + rhs.mul(lhs[1]); use ark_ec::CurveGroup; debug_assert_eq!(o.into_affine(), { let mut t = rhs.into_group(); self.mul_action(&mut t); t }.into_affine() ); - self.resplit(); o } } +#[cfg(test)] +mod tests { + use super::*; + use ark_bls12_377::Fr; + use ark_ff::MontFp; + use ark_std::fmt::Debug; + + impl Debug for SecretScalar { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + let selfy = &self.0; + f.debug_tuple("SecretScalar") + .field(&selfy[0]) + .field(&selfy[1]) + .finish() + } + } + + #[test] + fn from_single_scalar_works() { + let value: Fr = MontFp!("12345678"); + + let mut secret = SecretScalar::from(value); + assert_eq!(value, secret.scalar()); + + secret.resplit(); + assert_eq!(value, secret.scalar()); + } +} From 4c7eba58c4d375635fc518f9eb743591b07f91c0 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 28 Feb 2024 18:55:28 +0100 Subject: [PATCH 2/6] Minor tweaks --- ark-secret-scalar/src/lib.rs | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/ark-secret-scalar/src/lib.rs b/ark-secret-scalar/src/lib.rs index 4047883..465117b 100644 --- a/ark-secret-scalar/src/lib.rs +++ b/ark-secret-scalar/src/lib.rs @@ -58,7 +58,7 @@ impl From<&F> for SecretScalar { fn from(value: &F) -> Self { let mut xof = Rng2Xof(getrandom_or_panic()); let v1 = xof_read_reduced(&mut xof); - let v2 = value.clone() - &v1; + let v2 = value.sub(v1); SecretScalar([v1, v2]) } } @@ -73,11 +73,10 @@ impl Clone for SecretScalar { impl PartialEq for SecretScalar { fn eq(&self, rhs: &SecretScalar) -> bool { - let lhs = &self.0; - let rhs = &rhs.0; - ( (lhs[0] - rhs[0]) + (lhs[1] - rhs[1]) ).is_zero() + self.scalar() == rhs.scalar() } } + impl Eq for SecretScalar {} impl Drop for SecretScalar { @@ -93,6 +92,11 @@ impl SecretScalar { selfy[1] -= &x; } + /// Internal clone which skips resplit. + fn risky_clone(&self) -> SecretScalar { + SecretScalar(self.0.clone()) + } + /// Initialize and unbiased `SecretScalar` from a `XofReaader`. pub fn from_xof(xof: &mut R) -> Self { let mut xof = || xof_read_reduced(&mut *xof); @@ -103,8 +107,7 @@ impl SecretScalar { /// Multiply by a scalar. pub fn mul_by_challenge(&self, rhs: &F) -> F { - let selfy = &self.clone().0; - (selfy[0] * rhs) + (selfy[1] * rhs) + self.scalar() * rhs } pub fn scalar(&self) -> F { @@ -135,7 +138,7 @@ impl Add<&SecretScalar> for &SecretScalar { type Output = SecretScalar; fn add(self, rhs: &SecretScalar) -> SecretScalar { - let mut lhs = self.clone(); + let mut lhs = self.risky_clone(); lhs += rhs; lhs } @@ -174,12 +177,27 @@ mod tests { #[test] fn from_single_scalar_works() { - let value: Fr = MontFp!("12345678"); + let value: Fr = MontFp!("123456789"); let mut secret = SecretScalar::from(value); assert_eq!(value, secret.scalar()); secret.resplit(); assert_eq!(value, secret.scalar()); + + let secret2 = secret.clone(); + assert_ne!(secret.0[0], secret2.0[0]); + assert_ne!(secret.0[1], secret2.0[1]); + assert_eq!(secret, secret2); + } + + #[test] + fn mul_my_challenge_works() { + let value: Fr = MontFp!("123456789"); + let secret = SecretScalar::from(value); + + let factor = Fr::from(3); + let result = secret.mul_by_challenge(&factor); + assert_eq!(result, value * factor); } } From 92d8f930e31cc11ab34ec25c10a12ab1fead7a39 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 28 Feb 2024 19:02:49 +0100 Subject: [PATCH 3/6] Extend modifications to dleq_vrfs --- dleq_vrf/Cargo.toml | 6 +++--- dleq_vrf/src/keys.rs | 12 ++++++------ dleq_vrf/src/pedersen.rs | 4 +++- dleq_vrf/src/thin.rs | 19 ++++++++----------- dleq_vrf/src/traits.rs | 4 +++- 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/dleq_vrf/Cargo.toml b/dleq_vrf/Cargo.toml index 1148e1d..cf37473 100644 --- a/dleq_vrf/Cargo.toml +++ b/dleq_vrf/Cargo.toml @@ -2,7 +2,7 @@ name = "dleq_vrf" description = "VRFs from Chaum-Pedersen DLEQ proofs, usable in Ring VRFs" authors = ["Sergey Vasilyev ", "Jeff Burdges ", "Syed Hosseini "] -version = "0.0.2" +version = "0.0.3" repository = "https://github.com/w3f/ring-vrf/tree/master/dleq_vrf" edition = "2021" license = "MIT/Apache-2.0" @@ -20,8 +20,8 @@ ark-ff.workspace = true ark-ec.workspace = true ark-serialize.workspace = true -ark-secret-scalar = { version = "0.0.2", default-features = false, path = "../ark-secret-scalar" } -ark-transcript = { version = "0.0.2", default-features = false, path = "../ark-transcript" } +ark-secret-scalar = { default-features = false, path = "../ark-secret-scalar" } +ark-transcript = { default-features = false, path = "../ark-transcript" } ark-scale = { workspace = true, optional = true } diff --git a/dleq_vrf/src/keys.rs b/dleq_vrf/src/keys.rs index 00e0642..72f63fd 100644 --- a/dleq_vrf/src/keys.rs +++ b/dleq_vrf/src/keys.rs @@ -11,12 +11,11 @@ use ark_std::{vec::Vec, io::{Read, Write}}; use ark_ec::{AffineRepr}; // Group, CurveGroup use ark_serialize::{CanonicalSerialize,CanonicalDeserialize,SerializationError}; +use ark_transcript::xof_read_reduced; #[cfg(feature = "getrandom")] use ark_secret_scalar::{rand_core, RngCore}; -use ark_secret_scalar::SecretScalar; - use crate::{ ThinVrf, transcript::digest::{Update,XofReader}, @@ -86,7 +85,7 @@ pub struct SecretKey { pub(crate) thin: ThinVrf, /// Secret key represented as a scalar. - pub(crate) key: SecretScalar<::ScalarField>, + pub(crate) key: K::ScalarField, /// Seed for deriving the nonces used in Schnorr proofs. /// @@ -146,8 +145,9 @@ impl ThinVrf { { let mut nonce_seed: [u8; 32] = [0u8; 32]; xof.read(&mut nonce_seed); - let mut key = SecretScalar::from_xof(&mut xof); - let public = self.make_public(&mut key); + + let key = xof_read_reduced(&mut xof); + let public = self.make_public(&key); SecretKey { thin: self, key, nonce_seed, public, #[cfg(debug_assertions)] test_vector_fake_rng: false, @@ -156,7 +156,7 @@ impl ThinVrf { /// Generate a `SecretKey` from a 32 byte seed. pub fn secretkey_from_seed(self, seed: &[u8; 32]) -> SecretKey { - use crate::transcript::digest::{ExtendableOutput}; + use crate::transcript::digest::ExtendableOutput; let mut xof = crate::transcript::Shake128::default(); xof.update(b"VrfSecretSeed"); xof.update(seed.as_ref()); diff --git a/dleq_vrf/src/pedersen.rs b/dleq_vrf/src/pedersen.rs index ad897bf..8dec67f 100644 --- a/dleq_vrf/src/pedersen.rs +++ b/dleq_vrf/src/pedersen.rs @@ -9,6 +9,7 @@ use ark_ff::PrimeField; use ark_ec::{AffineRepr, CurveGroup}; +use ark_secret_scalar::SecretScalar; use ark_serialize::{CanonicalSerialize,CanonicalDeserialize}; use ark_std::{borrow::BorrowMut, vec::Vec}; @@ -322,8 +323,9 @@ where K: AffineRepr, H: AffineRepr, for i in 0..B { blindings.push( k.blindings[i] + c * secret_blindings.0[i] ); } + let secret = SecretScalar::from(&secret.key); let s = Scalars { - keying: k.keying + secret.key.mul_by_challenge(&c), + keying: k.keying + secret.mul_by_challenge(&c), blindings: blindings.into_inner().unwrap(), }; k.zeroize(); diff --git a/dleq_vrf/src/thin.rs b/dleq_vrf/src/thin.rs index 69fa43d..2ba927e 100644 --- a/dleq_vrf/src/thin.rs +++ b/dleq_vrf/src/thin.rs @@ -7,6 +7,7 @@ use ark_std::{borrow::{Borrow,BorrowMut}, vec::Vec}; use ark_ec::{AffineRepr, CurveGroup}; +use ark_secret_scalar::SecretScalar; use crate::{ Transcript, IntoTranscript, @@ -108,8 +109,9 @@ impl Witness> { t.label(b"Thin R"); t.append(&r); let c: ::ScalarField = t.challenge(b"ThinVrfChallenge").read_reduce(); - let s = k + secret.key.mul_by_challenge(&c); - k.zeroize(); + let secret = SecretScalar::from(secret.key); + let s = k + secret.mul_by_challenge(&c); + k.zeroize(); Batchable { compk: (), r, s } // TODO: Add some verify_final for additional rowhammer defenses? // We already hash the public key though, so no issues like Ed25519. @@ -135,15 +137,10 @@ impl Valid for Batchable> { */ impl ThinVrf { - pub(crate) fn make_public( - &self, - secret: &ark_secret_scalar::SecretScalar<::ScalarField> - ) -> PublicKey { - // #[cfg(feature = "getrandom")] - let p = secret * self.keying_base(); - // #[cfg(not(feature = "getrandom"))] - // let p = self.keying_base().mul(secret.0[0]) + self.keying_base().mul(secret.0[1]) ; - PublicKey(p.into_affine()) + pub(crate) fn make_public(&self, secret: &::ScalarField) -> PublicKey { + let secret = SecretScalar::from(secret); + let public = &secret * self.keying_base(); + PublicKey(public.into_affine()) } /// Verify thin VRF signature diff --git a/dleq_vrf/src/traits.rs b/dleq_vrf/src/traits.rs index d3c1d95..2ae2c47 100644 --- a/dleq_vrf/src/traits.rs +++ b/dleq_vrf/src/traits.rs @@ -23,6 +23,7 @@ //! a remote signer. +use ark_secret_scalar::SecretScalar; use ark_std::{borrow::Borrow, fmt, vec::Vec}; use ark_serialize::{CanonicalSerialize,CanonicalDeserialize}; // Valid @@ -73,7 +74,8 @@ impl EcVrfSecret for SecretKey where K: AffineRepr, H: AffineRepr, { fn vrf_preout(&self, input: &VrfInput) -> VrfPreOut { - VrfPreOut( (&self.key * &input.0).into_affine() ) + let secret = SecretScalar::from(&self.key); + VrfPreOut( (&secret * &input.0).into_affine() ) } } From 3d890fa0f21f83cafc5c12f934e54bc99252aebe Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 28 Feb 2024 19:06:50 +0100 Subject: [PATCH 4/6] Fix internal deps --- bandersnatch_vrfs/Cargo.toml | 2 +- nugget_bls/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bandersnatch_vrfs/Cargo.toml b/bandersnatch_vrfs/Cargo.toml index b604c48..c90a87e 100644 --- a/bandersnatch_vrfs/Cargo.toml +++ b/bandersnatch_vrfs/Cargo.toml @@ -9,7 +9,7 @@ license = "MIT/Apache-2.0" keywords = ["crypto", "cryptography", "vrf", "signature", "privacy"] [dependencies] -dleq_vrf = { version = "0.0.2", default-features = false, path = "../dleq_vrf", features = [ "scale" ] } +dleq_vrf = { default-features = false, path = "../dleq_vrf", features = [ "scale" ] } rand_core.workspace = true zeroize.workspace = true diff --git a/nugget_bls/Cargo.toml b/nugget_bls/Cargo.toml index 6c1e479..f99fbed 100644 --- a/nugget_bls/Cargo.toml +++ b/nugget_bls/Cargo.toml @@ -10,7 +10,7 @@ keywords = ["crypto", "cryptography", "bls", "signature"] [dependencies] -dleq_vrf = { version = "0.0.2", default-features = false, path = "../dleq_vrf" } +dleq_vrf = { default-features = false, path = "../dleq_vrf" } rand_core.workspace = true zeroize.workspace = true From 2af5bd27225f0e4516a1348692b9b1488b998a75 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 28 Feb 2024 23:00:11 +0100 Subject: [PATCH 5/6] Don't leak the secret --- ark-secret-scalar/src/lib.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/ark-secret-scalar/src/lib.rs b/ark-secret-scalar/src/lib.rs index 465117b..4a70549 100644 --- a/ark-secret-scalar/src/lib.rs +++ b/ark-secret-scalar/src/lib.rs @@ -16,8 +16,6 @@ use zeroize::Zeroize; // TODO: Remove ark-transcript dependency once https://github.com/arkworks-rs/algebra/pull/643 lands use ark_transcript::xof_read_reduced; - - pub struct Rng2Xof(pub R); impl XofReader for Rng2Xof { fn read(&mut self, dest: &mut [u8]) { @@ -25,7 +23,6 @@ impl XofReader for Rng2Xof { } } - /// Secret scalar split into the sum of two scalars, which randomly /// mutate but retain the same sum. Incurs 2x penalty in scalar /// multiplications, but provides side channel defenses. @@ -73,7 +70,9 @@ impl Clone for SecretScalar { impl PartialEq for SecretScalar { fn eq(&self, rhs: &SecretScalar) -> bool { - self.scalar() == rhs.scalar() + let lhs = &self.0; + let rhs = &rhs.0; + ((lhs[0] - rhs[0]) + (lhs[1] - rhs[1])).is_zero() } } @@ -84,6 +83,11 @@ impl Drop for SecretScalar { } impl SecretScalar { + /// Internal clone which skips resplit. + fn risky_clone(&self) -> SecretScalar { + SecretScalar(self.0.clone()) + } + pub fn resplit(&mut self) { let mut xof = Rng2Xof(getrandom_or_panic()); let x = xof_read_reduced(&mut xof); @@ -92,11 +96,6 @@ impl SecretScalar { selfy[1] -= &x; } - /// Internal clone which skips resplit. - fn risky_clone(&self) -> SecretScalar { - SecretScalar(self.0.clone()) - } - /// Initialize and unbiased `SecretScalar` from a `XofReaader`. pub fn from_xof(xof: &mut R) -> Self { let mut xof = || xof_read_reduced(&mut *xof); @@ -107,7 +106,8 @@ impl SecretScalar { /// Multiply by a scalar. pub fn mul_by_challenge(&self, rhs: &F) -> F { - self.scalar() * rhs + let lhs = &self.clone().0; + (lhs[0] * rhs) + (lhs[1] * rhs) } pub fn scalar(&self) -> F { From 2799d46b5120d09a00ce5c4778bdd3a8a2e3655a Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 28 Feb 2024 23:09:02 +0100 Subject: [PATCH 6/6] Docs --- ark-secret-scalar/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ark-secret-scalar/src/lib.rs b/ark-secret-scalar/src/lib.rs index 4a70549..33cb73b 100644 --- a/ark-secret-scalar/src/lib.rs +++ b/ark-secret-scalar/src/lib.rs @@ -88,6 +88,7 @@ impl SecretScalar { SecretScalar(self.0.clone()) } + /// Randomply resplit the secret in two components. pub fn resplit(&mut self) { let mut xof = Rng2Xof(getrandom_or_panic()); let x = xof_read_reduced(&mut xof); @@ -110,6 +111,7 @@ impl SecretScalar { (lhs[0] * rhs) + (lhs[1] * rhs) } + /// Get the secret scalar value by joining the two components. pub fn scalar(&self) -> F { self.0[0] + self.0[1] }