From 823609b53ea5923e774f31bf26fa0b3e7f4d85ca Mon Sep 17 00:00:00 2001 From: Sebastian Geisler Date: Thu, 17 Jan 2019 15:07:05 -0800 Subject: [PATCH 1/5] Add SerdeSecret wrapper type and SerializeSecret trait --- src/serde_impl.rs | 55 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/serde_impl.rs b/src/serde_impl.rs index 34fcc55..8fdad40 100644 --- a/src/serde_impl.rs +++ b/src/serde_impl.rs @@ -3,6 +3,7 @@ pub use self::field_vec::FieldWrap; use std::borrow::Cow; +use std::ops::Deref; use crate::G1; use serde::de::Error as DeserializeError; @@ -10,9 +11,63 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; use serde_derive::{Deserialize, Serialize}; use crate::poly::{coeff_pos, BivarCommitment}; +use crate::serde_impl::serialize_secret_internal::SerializeSecret; const ERR_DEG: &str = "commitment degree does not match coefficients"; +pub(crate) mod serialize_secret_internal { + use serde::Serializer; + + /// To avoid deriving [`Serialize`] automatically for structs containing secret keys this trait + /// should be implemented instead. It only enables explicit serialization through + /// [`::serde_impls::SerdeSecret`]. + pub trait SerializeSecret { + fn serialize_secret(&self, serializer: S) -> Result; + } +} + +/// `SerdeSecret` is a wrapper struct for serializing and deserializing secret keys. Due to security +/// concerns serialize shouldn't be implemented for secret keys to avoid accidental leakage. +/// +/// Whenever this struct is used the integrity of security boundaries should be checked carefully. +pub struct SerdeSecret(T); + +impl Deref for SerdeSecret { + type Target = T; + + fn deref(&self) -> &Self::Target { + self.inner() + } +} + +impl SerdeSecret { + /// Returns the actual secret from the wrapper + pub fn into_inner(self) -> T { + self.0 + } + + /// Returns a reference to the actual secret contained in the wrapper + pub fn inner(&self) -> &T { + &self.0 + } +} + +impl<'de, T: Deserialize<'de>> Deserialize<'de> for SerdeSecret { + fn deserialize(deserializer: D) -> Result where + D: Deserializer<'de> + { + Ok(SerdeSecret(Deserialize::deserialize(deserializer)?)) + } +} + +impl<'de, T: SerializeSecret> Serialize for SerdeSecret { + fn serialize(&self, serializer: S) -> Result where + S: Serializer + { + self.0.serialize_secret(serializer) + } +} + /// A type with the same content as `BivarCommitment`, but that has not been validated yet. #[derive(Serialize, Deserialize)] struct WireBivarCommitment<'a> { From a17cff2041c0cde508523c579364dc043d600578 Mon Sep 17 00:00:00 2001 From: Sebastian Geisler Date: Fri, 18 Jan 2019 11:53:29 -0800 Subject: [PATCH 2/5] Implement ser/de for SecretKey --- src/serde_impl.rs | 111 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 107 insertions(+), 4 deletions(-) diff --git a/src/serde_impl.rs b/src/serde_impl.rs index 8fdad40..da634fe 100644 --- a/src/serde_impl.rs +++ b/src/serde_impl.rs @@ -24,6 +24,15 @@ pub(crate) mod serialize_secret_internal { pub trait SerializeSecret { fn serialize_secret(&self, serializer: S) -> Result; } + + impl SerializeSecret for &T { + fn serialize_secret( + &self, + serializer: S, + ) -> Result<::Ok, ::Error> { + (*self).serialize_secret(serializer) + } + } } /// `SerdeSecret` is a wrapper struct for serializing and deserializing secret keys. Due to security @@ -53,21 +62,91 @@ impl SerdeSecret { } impl<'de, T: Deserialize<'de>> Deserialize<'de> for SerdeSecret { - fn deserialize(deserializer: D) -> Result where - D: Deserializer<'de> + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, { Ok(SerdeSecret(Deserialize::deserialize(deserializer)?)) } } impl<'de, T: SerializeSecret> Serialize for SerdeSecret { - fn serialize(&self, serializer: S) -> Result where - S: Serializer + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, { self.0.serialize_secret(serializer) } } +#[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] +impl<'de> Deserialize<'de> for crate::SecretKey { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + use pairing::bls12_381; + use pairing::PrimeField; + + use serde::de; + use std::fmt; + + struct ReprVisitor; + impl<'de> serde::de::Visitor<'de> for ReprVisitor { + type Value = [u64; 4]; + + fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + write!(formatter, "A tuple of four u64 integers.") + } + + fn visit_seq(self, mut seq: A) -> Result + where + A: de::SeqAccess<'de>, + { + let mut repr = [0u64; 4]; + for (num, part) in repr.iter_mut().enumerate() { + *part = match seq.next_element()? { + Some(x) => x, + None => return Err(de::Error::invalid_length(num + 1, &"Expected 4 u64s")), + }; + } + Ok(repr) + } + } + + let repr = deserializer.deserialize_tuple(4, ReprVisitor)?; + let mut fr = match bls12_381::Fr::from_repr(bls12_381::FrRepr(repr)) { + Ok(x) => x, + Err(pairing::PrimeFieldDecodingError::NotInField(_)) => { + return Err(de::Error::invalid_value( + de::Unexpected::Other(&"Number outside of prime field."), + &"Valid prime field element.", + )) + } + }; + + Ok(crate::SecretKey::from_mut(&mut fr)) + } +} + +#[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] +impl SerializeSecret for crate::SecretKey { + fn serialize_secret(&self, serializer: S) -> Result { + use pairing::PrimeField; + use serde::ser::SerializeTuple; + + let repr: &[u64] = &self.0.into_repr().0; + debug_assert_eq!(repr.len(), 4); + + let mut serialize_tuple = serializer.serialize_tuple(4)?; + for part in repr { + serialize_tuple.serialize_element(part)?; + } + + serialize_tuple.end() + } +} + /// A type with the same content as `BivarCommitment`, but that has not been validated yet. #[derive(Serialize, Deserialize)] struct WireBivarCommitment<'a> { @@ -305,4 +384,28 @@ mod tests { assert_eq!(comm, de_comm); } } + + #[test] + #[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] + fn serde_secret_key() { + use crate::serde_impl::SerdeSecret; + use crate::SecretKey; + use rand::{thread_rng, Rng}; + + let mut rng = thread_rng(); + for _ in 0..2048 { + let sk: SecretKey = rng.gen(); + let ser_ref = bincode::serialize(&SerdeSecret(&sk)).expect("serialize secret key"); + + let de = bincode::deserialize(&ser_ref).expect("deserialize secret key"); + assert_eq!(sk, de); + + let de_serde_secret: SerdeSecret = + bincode::deserialize(&ser_ref).expect("deserialize secret key"); + assert_eq!(sk, de_serde_secret.into_inner()); + + let ser_val = bincode::serialize(&SerdeSecret(sk)).expect("serialize secret key"); + assert_eq!(ser_ref, ser_val); + } + } } From cd490bcdee4235b2e3ab09f82dc3463e4f119187 Mon Sep 17 00:00:00 2001 From: Sebastian Geisler Date: Fri, 18 Jan 2019 14:09:09 -0800 Subject: [PATCH 3/5] Implement ser/de for SecretKeyShare --- src/serde_impl.rs | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/serde_impl.rs b/src/serde_impl.rs index da634fe..8d60d25 100644 --- a/src/serde_impl.rs +++ b/src/serde_impl.rs @@ -147,6 +147,25 @@ impl SerializeSecret for crate::SecretKey { } } +#[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] +impl<'de> Deserialize<'de> for crate::SecretKeyShare { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + Ok(crate::SecretKeyShare(Deserialize::deserialize( + deserializer, + )?)) + } +} + +#[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] +impl SerializeSecret for crate::SecretKeyShare { + fn serialize_secret(&self, serializer: S) -> Result { + self.0.serialize_secret(serializer) + } +} + /// A type with the same content as `BivarCommitment`, but that has not been validated yet. #[derive(Serialize, Deserialize)] struct WireBivarCommitment<'a> { @@ -408,4 +427,28 @@ mod tests { assert_eq!(ser_ref, ser_val); } } + + #[test] + #[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] + fn serde_secret_key_share() { + use crate::serde_impl::SerdeSecret; + use crate::SecretKeyShare; + use rand::{thread_rng, Rng}; + + let mut rng = thread_rng(); + for _ in 0..2048 { + let sk: SecretKeyShare = rng.gen(); + let ser_ref = bincode::serialize(&SerdeSecret(&sk)).expect("serialize secret key"); + + let de = bincode::deserialize(&ser_ref).expect("deserialize secret key"); + assert_eq!(sk, de); + + let de_serde_secret: SerdeSecret = + bincode::deserialize(&ser_ref).expect("deserialize secret key"); + assert_eq!(sk, de_serde_secret.into_inner()); + + let ser_val = bincode::serialize(&SerdeSecret(sk)).expect("serialize secret key"); + assert_eq!(ser_ref, ser_val); + } + } } From ce1c30f9b4a720397b18d1022d65289ece44ee47 Mon Sep 17 00:00:00 2001 From: Sebastian Geisler Date: Mon, 21 Jan 2019 16:40:02 -0800 Subject: [PATCH 4/5] Address @afck's comments * make SerializeSecret private * simplify serde impls --- src/serde_impl.rs | 53 +++++++---------------------------------------- 1 file changed, 8 insertions(+), 45 deletions(-) diff --git a/src/serde_impl.rs b/src/serde_impl.rs index 8d60d25..d440218 100644 --- a/src/serde_impl.rs +++ b/src/serde_impl.rs @@ -15,7 +15,7 @@ use crate::serde_impl::serialize_secret_internal::SerializeSecret; const ERR_DEG: &str = "commitment degree does not match coefficients"; -pub(crate) mod serialize_secret_internal { +mod serialize_secret_internal { use serde::Serializer; /// To avoid deriving [`Serialize`] automatically for structs containing secret keys this trait @@ -70,7 +70,7 @@ impl<'de, T: Deserialize<'de>> Deserialize<'de> for SerdeSecret { } } -impl<'de, T: SerializeSecret> Serialize for SerdeSecret { +impl Serialize for SerdeSecret { fn serialize(&self, serializer: S) -> Result where S: Serializer, @@ -79,43 +79,16 @@ impl<'de, T: SerializeSecret> Serialize for SerdeSecret { } } -#[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] impl<'de> Deserialize<'de> for crate::SecretKey { fn deserialize(deserializer: D) -> Result where D: Deserializer<'de>, { - use pairing::bls12_381; + use crate::{Fr, FrRepr}; use pairing::PrimeField; - use serde::de; - use std::fmt; - - struct ReprVisitor; - impl<'de> serde::de::Visitor<'de> for ReprVisitor { - type Value = [u64; 4]; - - fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - write!(formatter, "A tuple of four u64 integers.") - } - fn visit_seq(self, mut seq: A) -> Result - where - A: de::SeqAccess<'de>, - { - let mut repr = [0u64; 4]; - for (num, part) in repr.iter_mut().enumerate() { - *part = match seq.next_element()? { - Some(x) => x, - None => return Err(de::Error::invalid_length(num + 1, &"Expected 4 u64s")), - }; - } - Ok(repr) - } - } - - let repr = deserializer.deserialize_tuple(4, ReprVisitor)?; - let mut fr = match bls12_381::Fr::from_repr(bls12_381::FrRepr(repr)) { + let mut fr = match Fr::from_repr(FrRepr(Deserialize::deserialize(deserializer)?)) { Ok(x) => x, Err(pairing::PrimeFieldDecodingError::NotInField(_)) => { return Err(de::Error::invalid_value( @@ -129,25 +102,14 @@ impl<'de> Deserialize<'de> for crate::SecretKey { } } -#[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] impl SerializeSecret for crate::SecretKey { fn serialize_secret(&self, serializer: S) -> Result { use pairing::PrimeField; - use serde::ser::SerializeTuple; - - let repr: &[u64] = &self.0.into_repr().0; - debug_assert_eq!(repr.len(), 4); - - let mut serialize_tuple = serializer.serialize_tuple(4)?; - for part in repr { - serialize_tuple.serialize_element(part)?; - } - serialize_tuple.end() + Serialize::serialize(&self.0.into_repr().0, serializer) } } -#[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] impl<'de> Deserialize<'de> for crate::SecretKeyShare { fn deserialize(deserializer: D) -> Result where @@ -159,7 +121,6 @@ impl<'de> Deserialize<'de> for crate::SecretKeyShare { } } -#[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] impl SerializeSecret for crate::SecretKeyShare { fn serialize_secret(&self, serializer: S) -> Result { self.0.serialize_secret(serializer) @@ -429,7 +390,6 @@ mod tests { } #[test] - #[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] fn serde_secret_key_share() { use crate::serde_impl::SerdeSecret; use crate::SecretKeyShare; @@ -449,6 +409,9 @@ mod tests { let ser_val = bincode::serialize(&SerdeSecret(sk)).expect("serialize secret key"); assert_eq!(ser_ref, ser_val); + + #[cfg(not(feature = "use-insecure-test-only-mock-crypto"))] + assert_eq!(ser_val.len(), 32); } } } From 7a077846c9e626e485001811da7c5fae700947d9 Mon Sep 17 00:00:00 2001 From: Sebastian Geisler Date: Tue, 22 Jan 2019 15:06:38 -0800 Subject: [PATCH 5/5] Add documentation to secret keys pointing to SerdeSecret --- src/lib.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 2f806a0..9f146e4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -254,6 +254,11 @@ impl SignatureShare { /// A secret key; wraps a single prime field element. The field element is /// heap allocated to avoid any stack copying that result when passing /// `SecretKey`s between stack frames. +/// +/// # Serde integration +/// `SecretKey` implements `Deserialize` but not `Serialize` to avoid accidental +/// serialization in insecure contexts. To enable both use the `::serde_impl::SerdeSecret` +/// wrapper which implements both `Deserialize` and `Serialize`. #[derive(PartialEq, Eq)] pub struct SecretKey(Box); @@ -364,6 +369,11 @@ impl SecretKey { } /// A secret key share. +/// +/// # Serde integration +/// `SecretKeyShare` implements `Deserialize` but not `Serialize` to avoid accidental +/// serialization in insecure contexts. To enable both use the `::serde_impl::SerdeSecret` +/// wrapper which implements both `Deserialize` and `Serialize`. #[derive(Clone, PartialEq, Eq, Default)] pub struct SecretKeyShare(SecretKey);