From c548749b77d1f0627879037f3e7e4b0e46fccf08 Mon Sep 17 00:00:00 2001 From: Bogdan Opanchuk Date: Thu, 17 Oct 2024 15:41:24 -0700 Subject: [PATCH] Add a SerializedMap type to use anywhere where we would serialize a BTreeMap --- CHANGELOG.md | 21 ++++ clippy.toml | 1 + example/Cargo.toml | 2 +- example/src/simple.rs | 5 +- manul/Cargo.toml | 2 + manul/src/lib.rs | 1 + manul/src/session/echo.rs | 17 ++- manul/src/session/evidence.rs | 21 ++-- manul/src/utils.rs | 5 + manul/src/utils/serializable_map.rs | 186 ++++++++++++++++++++++++++++ 10 files changed, 242 insertions(+), 19 deletions(-) create mode 100644 CHANGELOG.md create mode 100644 clippy.toml create mode 100644 manul/src/utils.rs create mode 100644 manul/src/utils/serializable_map.rs diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..ea08318 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,21 @@ +# Changelog + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [0.0.2] - In development + +### Added + +- `SerializableMap` wrapper for `BTreeMap` supporting more formats and providing some safety features. (#[32]) + + +[#32]: https://github.com/entropyxyz/manul/pull/32 + + +## [0.0.1] - 2024-10-12 + +Initial release. + + +[0.0.1]: https://github.com/entropyxyz/manul/releases/tag/v0.0.1 diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 0000000..154626e --- /dev/null +++ b/clippy.toml @@ -0,0 +1 @@ +allow-unwrap-in-tests = true diff --git a/example/Cargo.toml b/example/Cargo.toml index 61bb1ad..d5522f7 100644 --- a/example/Cargo.toml +++ b/example/Cargo.toml @@ -10,8 +10,8 @@ readme = "README.md" [dependencies] manul = { path = "../manul" } -bincode = { version = "2.0.0-rc.3", default-features = false, features = ["alloc", "serde"] } serde = "1" +serde_asn1_der = "0.8" sha3 = "0.10" rand_core = "0.6" tracing-subscriber = { version = "0.3", features = ["env-filter"] } diff --git a/example/src/simple.rs b/example/src/simple.rs index 4cbaf80..602ce86 100644 --- a/example/src/simple.rs +++ b/example/src/simple.rs @@ -76,14 +76,13 @@ impl Protocol for SimpleProtocol { type Digest = Sha3_256; fn serialize(value: T) -> Result, LocalError> { - bincode::serde::encode_to_vec(value, bincode::config::standard()) + serde_asn1_der::to_vec(&value) .map(|vec| vec.into()) .map_err(|err| LocalError::new(err.to_string())) } fn deserialize<'de, T: Deserialize<'de>>(bytes: &'de [u8]) -> Result { - bincode::serde::decode_borrowed_from_slice(bytes, bincode::config::standard()) - .map_err(|err| DeserializationError::new(err.to_string())) + serde_asn1_der::from_bytes(bytes).map_err(|err| DeserializationError::new(err.to_string())) } fn verify_direct_message_is_invalid( diff --git a/manul/Cargo.toml b/manul/Cargo.toml index dd9396d..d76761f 100644 --- a/manul/Cargo.toml +++ b/manul/Cargo.toml @@ -24,6 +24,8 @@ rand = { version = "0.8", default-features = false, optional = true } impls = "1" sha3 = "0.10" rand = { version = "0.8", default-features = false } +serde_asn1_der = "0.8" +serde_json = "1" [features] testing = ["rand"] diff --git a/manul/src/lib.rs b/manul/src/lib.rs index f8bc102..824e78a 100644 --- a/manul/src/lib.rs +++ b/manul/src/lib.rs @@ -17,6 +17,7 @@ extern crate alloc; pub mod protocol; pub mod session; +pub mod utils; #[cfg(any(test, feature = "testing"))] pub mod testing; diff --git a/manul/src/session/echo.rs b/manul/src/session/echo.rs index e1d9959..7b5c8dc 100644 --- a/manul/src/session/echo.rs +++ b/manul/src/session/echo.rs @@ -15,9 +15,12 @@ use super::{ message::{MessageVerificationError, SignedMessage}, LocalError, }; -use crate::protocol::{ - Artifact, DirectMessage, EchoBroadcast, FinalizeError, FinalizeOutcome, ObjectSafeRound, Payload, Protocol, - ReceiveError, Round, RoundId, +use crate::{ + protocol::{ + Artifact, DirectMessage, EchoBroadcast, FinalizeError, FinalizeOutcome, ObjectSafeRound, Payload, Protocol, + ReceiveError, Round, RoundId, + }, + utils::SerializableMap, }; #[derive(Debug)] @@ -27,8 +30,8 @@ pub(crate) enum EchoRoundError { } #[derive(Debug, Clone, Serialize, Deserialize)] -pub struct EchoRoundMessage { - pub(crate) echo_messages: BTreeMap>, +pub struct EchoRoundMessage { + pub(crate) echo_messages: SerializableMap>, } pub struct EchoRound { @@ -121,7 +124,9 @@ where ))); } - let message = EchoRoundMessage { echo_messages }; + let message = EchoRoundMessage { + echo_messages: echo_messages.into(), + }; let dm = DirectMessage::new::(&message)?; Ok((dm, Artifact::empty())) } diff --git a/manul/src/session/evidence.rs b/manul/src/session/evidence.rs index ad3bca6..786cdb0 100644 --- a/manul/src/session/evidence.rs +++ b/manul/src/session/evidence.rs @@ -10,9 +10,12 @@ use super::{ transcript::Transcript, LocalError, }; -use crate::protocol::{ - DirectMessage, DirectMessageError, EchoBroadcast, EchoBroadcastError, MessageValidationError, Protocol, - ProtocolError, ProtocolValidationError, RoundId, +use crate::{ + protocol::{ + DirectMessage, DirectMessageError, EchoBroadcast, EchoBroadcastError, MessageValidationError, Protocol, + ProtocolError, ProtocolValidationError, RoundId, + }, + utils::SerializableMap, }; #[derive(Debug, Clone)] @@ -113,9 +116,9 @@ where error, direct_message, echo_broadcast, - direct_messages, - echo_broadcasts, - combined_echos, + direct_messages: direct_messages.into(), + echo_broadcasts: echo_broadcasts.into(), + combined_echos: combined_echos.into(), }), }) } @@ -352,9 +355,9 @@ struct ProtocolEvidence { error: P::ProtocolError, direct_message: SignedMessage, echo_broadcast: Option>, - direct_messages: BTreeMap>, - echo_broadcasts: BTreeMap>, - combined_echos: BTreeMap>, + direct_messages: SerializableMap>, + echo_broadcasts: SerializableMap>, + combined_echos: SerializableMap>, } impl ProtocolEvidence diff --git a/manul/src/utils.rs b/manul/src/utils.rs new file mode 100644 index 0000000..856394a --- /dev/null +++ b/manul/src/utils.rs @@ -0,0 +1,5 @@ +//! Assorted utilities. + +mod serializable_map; + +pub use serializable_map::SerializableMap; diff --git a/manul/src/utils/serializable_map.rs b/manul/src/utils/serializable_map.rs new file mode 100644 index 0000000..399bd00 --- /dev/null +++ b/manul/src/utils/serializable_map.rs @@ -0,0 +1,186 @@ +use alloc::{collections::BTreeMap, format}; +use core::{ + fmt::{self, Debug}, + marker::PhantomData, + ops::Deref, +}; + +use serde::{ + de::{self, SeqAccess, Visitor}, + ser::SerializeSeq, + Deserialize, Deserializer, Serialize, Serializer, +}; + +/// A wrapper for [`BTreeMap`](`alloc::collections::BTreeMap`) that is safe to serialize. +/// +/// It solves two issues: +/// - some serialization formats/implementations (e.g. `serde_asn1_der`) +/// do not support serializing maps at all; +/// - some serialization formats/implementations (e.g. `bincode`) serialize maps as sequences of key/value pairs, +/// but on deserialization do not check if the keys repeat. +/// This can make an arbitrarily long message still "valid", leading to DoS attacks. +/// +/// This implementation serializes maps as sequences of key/value pairs, +/// but checks for duplicate keys on deserialization. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SerializableMap(BTreeMap); + +impl From> for SerializableMap { + fn from(source: BTreeMap) -> Self { + Self(source) + } +} + +impl Deref for SerializableMap { + type Target = BTreeMap; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl Serialize for SerializableMap +where + K: Serialize, + V: Serialize, +{ + fn serialize(&self, serializer: S) -> Result { + // TODO: an error here can be covered by a custom `Serializer`, + // but that's a lot of extra code to test just one line. + // Is there an easier way? + // Alternatively, we wait until `#[coverage]` is stabilized. + let mut seq = serializer.serialize_seq(Some(self.0.len()))?; + for e in self.0.iter() { + seq.serialize_element(&e)?; + } + seq.end() + } +} + +struct MapVisitor(PhantomData<(K, V)>); + +impl<'de, K, V> Visitor<'de> for MapVisitor +where + K: Debug + Clone + Ord + Deserialize<'de>, + V: Deserialize<'de>, +{ + type Value = SerializableMap; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("A map serialized as a list of pairs") + } + + fn visit_seq(self, mut access: M) -> Result + where + M: SeqAccess<'de>, + { + let mut map = SerializableMap(BTreeMap::new()); + + while let Some((key, value)) = access.next_element::<(K, V)>()? { + // This clone, and the consequent `Debug` bound on the impl can be removed + // when `BTreeMap::try_insert()` is stabilized. + // Or we could call `BTreeMap::contains()` first, but it's more expensive than cloning a key + // (which will be short). + let key_clone = key.clone(); + if map.0.insert(key, value).is_some() { + return Err(de::Error::custom(format!("Duplicate key: {key_clone:?}"))); + } + } + + Ok(map) + } +} + +impl<'de, K, V> Deserialize<'de> for SerializableMap +where + K: Debug + Clone + Ord + Deserialize<'de>, + V: Deserialize<'de>, +{ + fn deserialize>(deserializer: D) -> Result { + deserializer.deserialize_seq(MapVisitor::(PhantomData)) + } +} + +#[cfg(test)] +mod tests { + use alloc::collections::BTreeMap; + use alloc::string::{String, ToString}; + use alloc::{vec, vec::Vec}; + + use serde::{Deserialize, Serialize}; + + use super::SerializableMap; + + fn asn1_serialize(value: &T) -> Result, String> { + serde_asn1_der::to_vec(value).map_err(|err| err.to_string()) + } + + fn asn1_deserialize<'de, T: Deserialize<'de>>(bytes: &'de [u8]) -> Result { + serde_asn1_der::from_bytes(bytes).map_err(|err| err.to_string()) + } + + fn json_serialize(value: &T) -> String { + serde_json::to_string(value).unwrap() + } + + fn json_deserialize<'de, T: Deserialize<'de>>(string: &'de str) -> Result { + serde_json::from_str::(string).map_err(|err| err.to_string()) + } + + #[test] + fn roundtrip() { + let map = SerializableMap::(BTreeMap::from([(120, 130), (140, 150)])); + let map_serialized = asn1_serialize(&map).unwrap(); + let map_back = asn1_deserialize(&map_serialized).unwrap(); + assert_eq!(map, map_back); + } + + #[test] + fn representation() { + // Test that the map is represented identically to a vector of tuples in the serialized data. + let map = SerializableMap::(BTreeMap::from([(120, 130), (140, 150)])); + let map_as_vec = vec![(120u8, 130u8), (140, 150)]; + let map_serialized = asn1_serialize(&map).unwrap(); + let map_as_vec_serialized = asn1_serialize(&map_as_vec).unwrap(); + assert_eq!(map_serialized, map_as_vec_serialized); + } + + #[test] + fn duplicate_key() { + let map_as_vec = vec![(120u8, 130u8), (120, 150)]; + let map_serialized = asn1_serialize(&map_as_vec).unwrap(); + assert_eq!( + asn1_deserialize::>(&map_serialized).unwrap_err(), + "Serde error: Duplicate key: 120" + ); + } + + #[test] + fn serialize_error() { + // Coverage for possible errors during serialization. + // ASN.1 cannot serialize BTreeMap, so we will use it to trigger an error. + let map = SerializableMap(BTreeMap::from([(1u8, BTreeMap::from([(2u8, 3u8)]))])); + assert!(asn1_serialize(&map) + .unwrap_err() + .starts_with("Unsupported Maps variants are not supported by this implementation")); + } + + #[test] + fn unexpected_sequence_element() { + // The deserializer will encounter an integer where it expects a tuple. + let not_map_serialized = asn1_serialize(&[1u64, 2u64]).unwrap(); + assert!(asn1_deserialize::>(¬_map_serialized) + .unwrap_err() + .contains("Invalid encoding DER object is not a valid sequence"),); + } + + #[test] + fn unexpected_type() { + // Have to use JSON and not ASN1 here because `serde_asn1_der` doesn't seem to trigger `Visitor::expecting()`. + let not_map_serialized = json_serialize(&1); + assert_eq!( + json_deserialize::>(¬_map_serialized).unwrap_err(), + "invalid type: integer `1`, expected A map serialized as a list of pairs at line 1 column 1" + ); + } +}