From 29ce0d4fe96dd2f4815c0a6ba36fc87bd5c1247e Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Wed, 23 Oct 2019 14:52:48 -0700 Subject: [PATCH 1/3] Add length checks to serde-bincode tests. This ensures that the serde Serialize and Deserialize implementations use fixed-length Serde tuples, rather than variable-length byte arrays. This flaw in data modeling was pointed out by Trevor Perrin. --- src/edwards.rs | 8 ++++++++ src/ristretto.rs | 8 ++++++++ src/scalar.rs | 13 +++++++++++-- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/edwards.rs b/src/edwards.rs index ce6599f..d80392d 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -1410,10 +1410,18 @@ mod test { let enc_compressed = bincode::serialize(&constants::ED25519_BASEPOINT_COMPRESSED).unwrap(); assert_eq!(encoded, enc_compressed); + // Check that the encoding is 32 bytes exactly + assert_eq!(encoded.len(), 32); + let dec_uncompressed: EdwardsPoint = bincode::deserialize(&encoded).unwrap(); let dec_compressed: CompressedEdwardsY = bincode::deserialize(&encoded).unwrap(); assert_eq!(dec_uncompressed, constants::ED25519_BASEPOINT_POINT); assert_eq!(dec_compressed, constants::ED25519_BASEPOINT_COMPRESSED); + + // Check that the encoding itself matches the usual one + let raw_bytes = constants::ED25519_BASEPOINT_COMPRESSED.as_bytes(); + let bp: EdwardsPoint = bincode::deserialize(raw_bytes).unwrap(); + assert_eq!(bp, constants::ED25519_BASEPOINT_POINT); } } diff --git a/src/ristretto.rs b/src/ristretto.rs index e4c1e81..5d0be03 100644 --- a/src/ristretto.rs +++ b/src/ristretto.rs @@ -1092,11 +1092,19 @@ mod test { let enc_compressed = bincode::serialize(&constants::RISTRETTO_BASEPOINT_COMPRESSED).unwrap(); assert_eq!(encoded, enc_compressed); + // Check that the encoding is 32 bytes exactly + assert_eq!(encoded.len(), 32); + let dec_uncompressed: RistrettoPoint = bincode::deserialize(&encoded).unwrap(); let dec_compressed: CompressedRistretto = bincode::deserialize(&encoded).unwrap(); assert_eq!(dec_uncompressed, constants::RISTRETTO_BASEPOINT_POINT); assert_eq!(dec_compressed, constants::RISTRETTO_BASEPOINT_COMPRESSED); + + // Check that the encoding itself matches the usual one + let raw_bytes = constants::RISTRETTO_BASEPOINT_COMPRESSED.as_bytes(); + let bp: RistrettoPoint = bincode::deserialize(raw_bytes).unwrap(); + assert_eq!(bp, constants::RISTRETTO_BASEPOINT_POINT); } #[test] diff --git a/src/scalar.rs b/src/scalar.rs index 6f9d52b..86085ac 100644 --- a/src/scalar.rs +++ b/src/scalar.rs @@ -1649,9 +1649,18 @@ mod test { #[cfg(feature = "serde")] fn serde_bincode_scalar_roundtrip() { use bincode; - let output = bincode::serialize(&X).unwrap(); - let parsed: Scalar = bincode::deserialize(&output).unwrap(); + let encoded = bincode::serialize(&X).unwrap(); + let parsed: Scalar = bincode::deserialize(&encoded).unwrap(); assert_eq!(parsed, X); + + // Check that the encoding is 32 bytes exactly + assert_eq!(encoded.len(), 32); + + // Check that the encoding itself matches the usual one + assert_eq!( + X, + bincode::deserialize(X.as_bytes()).unwrap(), + ); } #[cfg(debug_assertions)] From 0fc534d98994630c3f4d91096677b4f9faf113be Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Wed, 23 Oct 2019 14:49:55 -0700 Subject: [PATCH 2/3] Use "tuples" instead of "bytes" in the Serde datamodel. This is a breaking change to the serialization format. It fixes it so that the Serde encoding can match the conventional encoding of each type of object, and so that Serde can be used with no overhead -- when using serde-bincode, the Serde encoding now matches the manual encoding. --- Cargo.toml | 2 +- src/edwards.rs | 52 ++++++++++++++++++++++++++++-------------------- src/ristretto.rs | 52 ++++++++++++++++++++++++++++-------------------- src/scalar.rs | 38 +++++++++++++++++------------------ 4 files changed, 79 insertions(+), 65 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 563998d..64aabe2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "curve25519-dalek" -version = "1.2.3" +version = "2.0.0-alpha.0" authors = ["Isis Lovecruft ", "Henry de Valence "] readme = "README.md" diff --git a/src/edwards.rs b/src/edwards.rs index d80392d..998af8d 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -217,7 +217,12 @@ impl Serialize for EdwardsPoint { fn serialize(&self, serializer: S) -> Result where S: Serializer { - serializer.serialize_bytes(self.compress().as_bytes()) + use serde::ser::SerializeTuple; + let mut tup = serializer.serialize_tuple(32)?; + for byte in self.compress().as_bytes().iter() { + tup.serialize_element(byte)?; + } + tup.end() } } @@ -226,7 +231,12 @@ impl Serialize for CompressedEdwardsY { fn serialize(&self, serializer: S) -> Result where S: Serializer { - serializer.serialize_bytes(self.as_bytes()) + use serde::ser::SerializeTuple; + let mut tup = serializer.serialize_tuple(32)?; + for byte in self.as_bytes().iter() { + tup.serialize_element(byte)?; + } + tup.end() } } @@ -244,22 +254,21 @@ impl<'de> Deserialize<'de> for EdwardsPoint { formatter.write_str("a valid point in Edwards y + sign format") } - fn visit_bytes(self, v: &[u8]) -> Result - where E: serde::de::Error + fn visit_seq(self, mut seq: A) -> Result + where A: serde::de::SeqAccess<'de> { - if v.len() == 32 { - let mut arr32 = [0u8; 32]; - arr32[0..32].copy_from_slice(v); - CompressedEdwardsY(arr32) - .decompress() - .ok_or(serde::de::Error::custom("decompression failed")) - } else { - Err(serde::de::Error::invalid_length(v.len(), &self)) + let mut bytes = [0u8; 32]; + for i in 0..32 { + bytes[i] = seq.next_element()? + .ok_or(serde::de::Error::invalid_length(i, &"expected 32 bytes"))?; } + CompressedEdwardsY(bytes) + .decompress() + .ok_or(serde::de::Error::custom("decompression failed")) } } - deserializer.deserialize_bytes(EdwardsPointVisitor) + deserializer.deserialize_tuple(32, EdwardsPointVisitor) } } @@ -277,20 +286,19 @@ impl<'de> Deserialize<'de> for CompressedEdwardsY { formatter.write_str("32 bytes of data") } - fn visit_bytes(self, v: &[u8]) -> Result - where E: serde::de::Error + fn visit_seq(self, mut seq: A) -> Result + where A: serde::de::SeqAccess<'de> { - if v.len() == 32 { - let mut arr32 = [0u8; 32]; - arr32[0..32].copy_from_slice(v); - Ok(CompressedEdwardsY(arr32)) - } else { - Err(serde::de::Error::invalid_length(v.len(), &self)) + let mut bytes = [0u8; 32]; + for i in 0..32 { + bytes[i] = seq.next_element()? + .ok_or(serde::de::Error::invalid_length(i, &"expected 32 bytes"))?; } + Ok(CompressedEdwardsY(bytes)) } } - deserializer.deserialize_bytes(CompressedEdwardsYVisitor) + deserializer.deserialize_tuple(32, CompressedEdwardsYVisitor) } } diff --git a/src/ristretto.rs b/src/ristretto.rs index 5d0be03..6d53e89 100644 --- a/src/ristretto.rs +++ b/src/ristretto.rs @@ -334,7 +334,12 @@ impl Serialize for RistrettoPoint { fn serialize(&self, serializer: S) -> Result where S: Serializer { - serializer.serialize_bytes(self.compress().as_bytes()) + use serde::ser::SerializeTuple; + let mut tup = serializer.serialize_tuple(32)?; + for byte in self.compress().as_bytes().iter() { + tup.serialize_element(byte)?; + } + tup.end() } } @@ -343,7 +348,12 @@ impl Serialize for CompressedRistretto { fn serialize(&self, serializer: S) -> Result where S: Serializer { - serializer.serialize_bytes(self.as_bytes()) + use serde::ser::SerializeTuple; + let mut tup = serializer.serialize_tuple(32)?; + for byte in self.as_bytes().iter() { + tup.serialize_element(byte)?; + } + tup.end() } } @@ -361,22 +371,21 @@ impl<'de> Deserialize<'de> for RistrettoPoint { formatter.write_str("a valid point in Ristretto format") } - fn visit_bytes(self, v: &[u8]) -> Result - where E: serde::de::Error + fn visit_seq(self, mut seq: A) -> Result + where A: serde::de::SeqAccess<'de> { - if v.len() == 32 { - let mut arr32 = [0u8; 32]; - arr32[0..32].copy_from_slice(v); - CompressedRistretto(arr32) - .decompress() - .ok_or(serde::de::Error::custom("decompression failed")) - } else { - Err(serde::de::Error::invalid_length(v.len(), &self)) + let mut bytes = [0u8; 32]; + for i in 0..32 { + bytes[i] = seq.next_element()? + .ok_or(serde::de::Error::invalid_length(i, &"expected 32 bytes"))?; } + CompressedRistretto(bytes) + .decompress() + .ok_or(serde::de::Error::custom("decompression failed")) } } - deserializer.deserialize_bytes(RistrettoPointVisitor) + deserializer.deserialize_tuple(32, RistrettoPointVisitor) } } @@ -394,20 +403,19 @@ impl<'de> Deserialize<'de> for CompressedRistretto { formatter.write_str("32 bytes of data") } - fn visit_bytes(self, v: &[u8]) -> Result - where E: serde::de::Error + fn visit_seq(self, mut seq: A) -> Result + where A: serde::de::SeqAccess<'de> { - if v.len() == 32 { - let mut arr32 = [0u8; 32]; - arr32[0..32].copy_from_slice(v); - Ok(CompressedRistretto(arr32)) - } else { - Err(serde::de::Error::invalid_length(v.len(), &self)) + let mut bytes = [0u8; 32]; + for i in 0..32 { + bytes[i] = seq.next_element()? + .ok_or(serde::de::Error::invalid_length(i, &"expected 32 bytes"))?; } + Ok(CompressedRistretto(bytes)) } } - deserializer.deserialize_bytes(CompressedRistrettoVisitor) + deserializer.deserialize_tuple(32, CompressedRistrettoVisitor) } } diff --git a/src/scalar.rs b/src/scalar.rs index 86085ac..3b252e2 100644 --- a/src/scalar.rs +++ b/src/scalar.rs @@ -385,7 +385,12 @@ impl Serialize for Scalar { fn serialize(&self, serializer: S) -> Result where S: Serializer { - serializer.serialize_bytes(self.reduce().as_bytes()) + use serde::ser::SerializeTuple; + let mut tup = serializer.serialize_tuple(32)?; + for byte in self.as_bytes().iter() { + tup.serialize_element(byte)?; + } + tup.end() } } @@ -400,32 +405,25 @@ impl<'de> Deserialize<'de> for Scalar { type Value = Scalar; fn expecting(&self, formatter: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { - formatter.write_str("a canonically-encoded 32-byte scalar value") + formatter.write_str("a valid point in Edwards y + sign format") } - fn visit_bytes(self, v: &[u8]) -> Result - where E: serde::de::Error + fn visit_seq(self, mut seq: A) -> Result + where A: serde::de::SeqAccess<'de> { - if v.len() == 32 { - let mut bytes = [0u8;32]; - bytes.copy_from_slice(v); - - static ERRMSG: &'static str = "encoding was not canonical"; - - Scalar::from_canonical_bytes(bytes) - .ok_or( - serde::de::Error::invalid_value( - serde::de::Unexpected::Bytes(v), - &ERRMSG, - ) - ) - } else { - Err(serde::de::Error::invalid_length(v.len(), &self)) + let mut bytes = [0u8; 32]; + for i in 0..32 { + bytes[i] = seq.next_element()? + .ok_or(serde::de::Error::invalid_length(i, &"expected 32 bytes"))?; } + Scalar::from_canonical_bytes(bytes) + .ok_or(serde::de::Error::custom( + &"scalar was not canonically encoded" + )) } } - deserializer.deserialize_bytes(ScalarVisitor) + deserializer.deserialize_tuple(32, ScalarVisitor) } } From 70e46c982655e2fbb97ad4f32c97ab0ced00685b Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Wed, 23 Oct 2019 15:55:03 -0700 Subject: [PATCH 3/3] Fill in missing Serde impl for MontgomeryPoint. --- Cargo.toml | 2 +- src/montgomery.rs | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 64aabe2..7274867 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,7 +42,7 @@ byteorder = { version = "^1.2.3", default-features = false, features = ["i128"] digest = { version = "0.8", default-features = false } clear_on_drop = "=0.2.3" subtle = { version = "2", default-features = false } -serde = { version = "1.0", default-features = false, optional = true } +serde = { version = "1.0", default-features = false, optional = true, features = ["derive"] } packed_simd = { version = "0.3.0", features = ["into_bits"], optional = true } [features] diff --git a/src/montgomery.rs b/src/montgomery.rs index 8b99bb5..a89de22 100644 --- a/src/montgomery.rs +++ b/src/montgomery.rs @@ -64,6 +64,7 @@ use subtle::ConstantTimeEq; /// Holds the \\(u\\)-coordinate of a point on the Montgomery form of /// Curve25519 or its twist. #[derive(Copy, Clone, Debug)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct MontgomeryPoint(pub [u8; 32]); /// Equality of `MontgomeryPoint`s is defined mod p. @@ -312,6 +313,22 @@ mod test { #[cfg(feature = "rand")] use rand_os::OsRng; + #[test] + #[cfg(feature = "serde")] + fn serde_bincode_basepoint_roundtrip() { + use bincode; + + let encoded = bincode::serialize(&constants::X25519_BASEPOINT).unwrap(); + let decoded: MontgomeryPoint = bincode::deserialize(&encoded).unwrap(); + + assert_eq!(encoded.len(), 32); + assert_eq!(decoded, constants::X25519_BASEPOINT); + + let raw_bytes = constants::X25519_BASEPOINT.as_bytes(); + let bp: MontgomeryPoint = bincode::deserialize(raw_bytes).unwrap(); + assert_eq!(bp, constants::X25519_BASEPOINT); + } + /// Test Montgomery -> Edwards on the X/Ed25519 basepoint #[test] fn basepoint_montgomery_to_edwards() {