Merge pull request #297 from dalek-cryptography/fix-serde

Fix serde data modeling.

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "curve25519-dalek"
-version = "1.2.3"
+version = "2.0.0-alpha.0"
 authors = ["Isis Lovecruft <isis@patternsinthevoid.net>",
            "Henry de Valence <hdevalence@hdevalence.ca>"]
 readme = "README.md"
@@ -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]

src/edwards.rs

@@ -217,7 +217,12 @@ impl Serialize for EdwardsPoint {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         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<E>(self, v: &[u8]) -> Result<EdwardsPoint, E>
-                where E: serde::de::Error
+            fn visit_seq<A>(self, mut seq: A) -> Result<EdwardsPoint, A::Error>
+                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<E>(self, v: &[u8]) -> Result<CompressedEdwardsY, E>
-                where E: serde::de::Error
+            fn visit_seq<A>(self, mut seq: A) -> Result<CompressedEdwardsY, A::Error>
+                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)
     }
 }
 
@@ -1410,10 +1418,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);
     }
 }

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() {

src/ristretto.rs

@@ -334,7 +334,12 @@ impl Serialize for RistrettoPoint {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         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<E>(self, v: &[u8]) -> Result<RistrettoPoint, E>
-                where E: serde::de::Error
+            fn visit_seq<A>(self, mut seq: A) -> Result<RistrettoPoint, A::Error>
+                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<E>(self, v: &[u8]) -> Result<CompressedRistretto, E>
-                where E: serde::de::Error
+            fn visit_seq<A>(self, mut seq: A) -> Result<CompressedRistretto, A::Error>
+                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)
     }
 }
 
@@ -1092,11 +1100,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]

src/scalar.rs

@@ -385,7 +385,12 @@ impl Serialize for Scalar {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         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<E>(self, v: &[u8]) -> Result<Scalar, E>
-                where E: serde::de::Error
+            fn visit_seq<A>(self, mut seq: A) -> Result<Scalar, A::Error>
+                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)
     }
 }
 
@@ -1649,9 +1647,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)]