serdect: safer bytes handling (#1112)

See #1111 for the discussion leading to this PR.

serdect/Cargo.toml

@@ -26,6 +26,7 @@ bincode = "1"
 ciborium = "0.2"
 hex-literal = "0.4"
 proptest = "1"
+rmp-serde = "1"
 serde = { version = "1.0.184", default-features = false, features = ["derive"] }
 serde_json = "1"
 serde-json-core = { version = "0.5", default-features = false, features = ["std"] }

serdect/README.md

@@ -30,6 +30,15 @@ other kinds of data-dependent branching on the contents of the serialized data,
 using a constant-time hex serialization with human-readable formats should
 help reduce the overall timing variability.
 
+`serdect` is tested against the following crates:
+- [`bincode`](https://crates.io/crates/bincode) v1
+- [`ciborium`](https://crates.io/crates/ciborium) v0.2
+- [`rmp-serde`](https://crates.io/crates/rmp-serde) v1
+- [`serde-json-core`](https://crates.io/crates/serde-json-core) v0.5
+- [`serde-json`](https://crates.io/crates/serde-json) v1
+- [`toml`](https://crates.io/crates/toml) v0.7
+
+
 ## Minimum Supported Rust Version
 
 Rust **1.60** or newer.

serdect/src/array.rs

@@ -1,11 +1,21 @@
 //! Serialization primitives for arrays.
 
+// Unfortunately, we currently cannot tell `serde` in a uniform fashion that we are serializing
+// a fixed-size byte array.
+// See https://github.com/serde-rs/serde/issues/2120 for the discussion.
+// Therefore we have to fall back to the slice methods,
+// which will add the size information in the binary formats.
+// The only difference is that for the arrays we require the size of the data
+// to be exactly equal to the size of the buffer during deserialization,
+// while for slices the buffer can be larger than the deserialized data.
+
 use core::fmt;
+use core::marker::PhantomData;
 
-use serde::de::{Error, SeqAccess, Visitor};
-use serde::ser::SerializeTuple;
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 
+use crate::common::{self, LengthCheck, SliceVisitor, StrIntoBufVisitor};
+
 #[cfg(feature = "zeroize")]
 use zeroize::Zeroize;
 
@@ -16,17 +26,7 @@ where
     S: Serializer,
     T: AsRef<[u8]>,
 {
-    if serializer.is_human_readable() {
-        crate::serialize_hex::<_, _, false>(value, serializer)
-    } else {
-        let mut seq = serializer.serialize_tuple(value.as_ref().len())?;
-
-        for byte in value.as_ref() {
-            seq.serialize_element(byte)?;
-        }
-
-        seq.end()
-    }
+    common::serialize_hex_lower_or_bin(value, serializer)
 }
 
 /// Serialize the given type as upper case hex when using human-readable
@@ -36,16 +36,21 @@ where
     S: Serializer,
     T: AsRef<[u8]>,
 {
-    if serializer.is_human_readable() {
-        crate::serialize_hex::<_, _, true>(value, serializer)
-    } else {
-        let mut seq = serializer.serialize_tuple(value.as_ref().len())?;
+    common::serialize_hex_upper_or_bin(value, serializer)
+}
 
-        for byte in value.as_ref() {
-            seq.serialize_element(byte)?;
-        }
+struct ExactLength;
 
-        seq.end()
+impl LengthCheck for ExactLength {
+    fn length_check(buffer_length: usize, data_length: usize) -> bool {
+        buffer_length == data_length
+    }
+    fn expecting(
+        formatter: &mut fmt::Formatter<'_>,
+        data_type: &str,
+        data_length: usize,
+    ) -> fmt::Result {
+        write!(formatter, "{} of length {}", data_type, data_length)
     }
 }
 
@@ -57,56 +62,9 @@ where
     D: Deserializer<'de>,
 {
     if deserializer.is_human_readable() {
-        struct StrVisitor<'b>(&'b mut [u8]);
-
-        impl<'de> Visitor<'de> for StrVisitor<'_> {
-            type Value = ();
-
-            fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
-                write!(formatter, "a string of length {}", self.0.len() * 2)
-            }
-
-            fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
-            where
-                E: Error,
-            {
-                if v.len() != self.0.len() * 2 {
-                    return Err(Error::invalid_length(v.len(), &self));
-                }
-
-                base16ct::mixed::decode(v, self.0).map_err(E::custom)?;
-
-                Ok(())
-            }
-        }
-
-        deserializer.deserialize_str(StrVisitor(buffer))
+        deserializer.deserialize_str(StrIntoBufVisitor::<ExactLength>(buffer, PhantomData))
     } else {
-        struct ArrayVisitor<'b>(&'b mut [u8]);
-
-        impl<'de> Visitor<'de> for ArrayVisitor<'_> {
-            type Value = ();
-
-            fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
-                write!(formatter, "an array of length {}", self.0.len())
-            }
-
-            fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
-            where
-                A: SeqAccess<'de>,
-            {
-                for (index, byte) in self.0.iter_mut().enumerate() {
-                    *byte = match seq.next_element()? {
-                        Some(byte) => byte,
-                        None => return Err(Error::invalid_length(index, &self)),
-                    };
-                }
-
-                Ok(())
-            }
-        }
-
-        deserializer.deserialize_tuple(buffer.len(), ArrayVisitor(buffer))
+        deserializer.deserialize_byte_buf(SliceVisitor::<ExactLength>(buffer, PhantomData))
     }
 }
 

serdect/src/common.rs

@@ -0,0 +1,181 @@
+use core::fmt;
+use core::marker::PhantomData;
+
+use serde::{
+    de::{Error, Visitor},
+    Serializer,
+};
+
+#[cfg(feature = "alloc")]
+use ::{alloc::vec::Vec, serde::Serialize};
+
+#[cfg(not(feature = "alloc"))]
+use serde::ser::Error as SerError;
+
+pub(crate) fn serialize_hex<S, T, const UPPERCASE: bool>(
+    value: &T,
+    serializer: S,
+) -> Result<S::Ok, S::Error>
+where
+    S: Serializer,
+    T: AsRef<[u8]>,
+{
+    #[cfg(feature = "alloc")]
+    if UPPERCASE {
+        return base16ct::upper::encode_string(value.as_ref()).serialize(serializer);
+    } else {
+        return base16ct::lower::encode_string(value.as_ref()).serialize(serializer);
+    }
+    #[cfg(not(feature = "alloc"))]
+    {
+        let _ = value;
+        let _ = serializer;
+        return Err(S::Error::custom(
+            "serializer is human readable, which requires the `alloc` crate feature",
+        ));
+    }
+}
+
+pub(crate) fn serialize_hex_lower_or_bin<S, T>(value: &T, serializer: S) -> Result<S::Ok, S::Error>
+where
+    S: Serializer,
+    T: AsRef<[u8]>,
+{
+    if serializer.is_human_readable() {
+        serialize_hex::<_, _, false>(value, serializer)
+    } else {
+        serializer.serialize_bytes(value.as_ref())
+    }
+}
+
+/// Serialize the given type as upper case hex when using human-readable
+/// formats or binary if the format is binary.
+pub(crate) fn serialize_hex_upper_or_bin<S, T>(value: &T, serializer: S) -> Result<S::Ok, S::Error>
+where
+    S: Serializer,
+    T: AsRef<[u8]>,
+{
+    if serializer.is_human_readable() {
+        serialize_hex::<_, _, true>(value, serializer)
+    } else {
+        serializer.serialize_bytes(value.as_ref())
+    }
+}
+
+pub(crate) trait LengthCheck {
+    fn length_check(buffer_length: usize, data_length: usize) -> bool;
+    fn expecting(
+        formatter: &mut fmt::Formatter<'_>,
+        data_type: &str,
+        data_length: usize,
+    ) -> fmt::Result;
+}
+
+pub(crate) struct StrIntoBufVisitor<'b, T: LengthCheck>(pub &'b mut [u8], pub PhantomData<T>);
+
+impl<'de, 'b, T: LengthCheck> Visitor<'de> for StrIntoBufVisitor<'b, T> {
+    type Value = ();
+
+    fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+        T::expecting(formatter, "a string", self.0.len() * 2)
+    }
+
+    fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+    where
+        E: Error,
+    {
+        if !T::length_check(self.0.len() * 2, v.len()) {
+            return Err(Error::invalid_length(v.len(), &self));
+        }
+        // TODO: Map `base16ct::Error::InvalidLength` to `Error::invalid_length`.
+        base16ct::mixed::decode(v, self.0)
+            .map(|_| ())
+            .map_err(E::custom)
+    }
+}
+
+#[cfg(feature = "alloc")]
+pub(crate) struct StrIntoVecVisitor;
+
+#[cfg(feature = "alloc")]
+impl<'de> Visitor<'de> for StrIntoVecVisitor {
+    type Value = Vec<u8>;
+
+    fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(formatter, "a string")
+    }
+
+    fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+    where
+        E: Error,
+    {
+        base16ct::mixed::decode_vec(v).map_err(E::custom)
+    }
+}
+
+pub(crate) struct SliceVisitor<'b, T: LengthCheck>(pub &'b mut [u8], pub PhantomData<T>);
+
+impl<'de, 'b, T: LengthCheck> Visitor<'de> for SliceVisitor<'b, T> {
+    type Value = ();
+
+    fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+        T::expecting(formatter, "an array", self.0.len())
+    }
+
+    fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
+    where
+        E: Error,
+    {
+        // Workaround for
+        // https://github.com/rust-lang/rfcs/blob/b1de05846d9bc5591d753f611ab8ee84a01fa500/text/2094-nll.md#problem-case-3-conditional-control-flow-across-functions
+        if T::length_check(self.0.len(), v.len()) {
+            let buffer = &mut self.0[..v.len()];
+            buffer.copy_from_slice(v);
+            return Ok(());
+        }
+
+        Err(E::invalid_length(v.len(), &self))
+    }
+
+    #[cfg(feature = "alloc")]
+    fn visit_byte_buf<E>(self, mut v: Vec<u8>) -> Result<Self::Value, E>
+    where
+        E: Error,
+    {
+        // Workaround for
+        // https://github.com/rust-lang/rfcs/blob/b1de05846d9bc5591d753f611ab8ee84a01fa500/text/2094-nll.md#problem-case-3-conditional-control-flow-across-functions
+        if T::length_check(self.0.len(), v.len()) {
+            let buffer = &mut self.0[..v.len()];
+            buffer.swap_with_slice(&mut v);
+            return Ok(());
+        }
+
+        Err(E::invalid_length(v.len(), &self))
+    }
+}
+
+#[cfg(feature = "alloc")]
+pub(crate) struct VecVisitor;
+
+#[cfg(feature = "alloc")]
+impl<'de> Visitor<'de> for VecVisitor {
+    type Value = Vec<u8>;
+
+    fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(formatter, "a bytestring")
+    }
+
+    fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
+    where
+        E: Error,
+    {
+        Ok(v.into())
+    }
+
+    fn visit_byte_buf<E>(self, v: Vec<u8>) -> Result<Self::Value, E>
+    where
+        E: Error,
+    {
+        Ok(v)
+    }
+}