Stop untrusted preallocation during string deserialization

This is an easy memory denial of service attack.

zebra-chain/src/block.rs

@@ -21,6 +21,7 @@ pub use header::BlockTimeError;
 pub use header::{CountedHeader, Header};
 pub use height::Height;
 pub use root_hash::RootHash;
+pub use serialize::MAX_BLOCK_BYTES;
 
 use serde::{Deserialize, Serialize};
 

zebra-chain/src/serialization/read_zcash.rs

@@ -11,6 +11,15 @@ use super::SerializationError;
 pub trait ReadZcashExt: io::Read {
     /// Reads a `u64` using the Bitcoin `CompactSize` encoding.
     ///
+    /// # Security
+    ///
+    /// Deserialized sizes must be validated before being used.
+    ///
+    /// Preallocating vectors using untrusted `CompactSize`s allows memory
+    /// denial of service attacks. Valid sizes must be less than
+    /// `MAX_BLOCK_BYTES / min_serialized_item_bytes` (or a lower limit
+    /// specified by the Zcash consensus rules or Bitcoin network protocol).
+    ///
     /// # Examples
     ///
     /// ```rust
@@ -87,15 +96,6 @@ pub trait ReadZcashExt: io::Read {
         Ok(SocketAddr::new(ip_addr, port))
     }
 
-    /// Read a Bitcoin-encoded UTF-8 string.
-    #[inline]
-    fn read_string(&mut self) -> Result<String, SerializationError> {
-        let len = self.read_compactsize()?;
-        let mut buf = vec![0; len as usize];
-        self.read_exact(&mut buf)?;
-        String::from_utf8(buf).map_err(|_| SerializationError::Parse("invalid utf-8"))
-    }
-
     /// Convenience method to read a `[u8; 4]`.
     #[inline]
     fn read_4_bytes(&mut self) -> io::Result<[u8; 4]> {

zebra-chain/src/serialization/zcash_deserialize.rs

@@ -1,6 +1,7 @@
 use std::io;
 
 use super::{ReadZcashExt, SerializationError};
+use byteorder::ReadBytesExt;
 
 /// Consensus-critical serialization for Zcash.
 ///
@@ -33,6 +34,21 @@ impl<T: ZcashDeserialize> ZcashDeserialize for Vec<T> {
     }
 }
 
+/// Read a byte.
+impl ZcashDeserialize for u8 {
+    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
+        Ok(reader.read_u8()?)
+    }
+}
+
+/// Read a Bitcoin-encoded UTF-8 string.
+impl ZcashDeserialize for String {
+    fn zcash_deserialize<R: io::Read>(reader: R) -> Result<Self, SerializationError> {
+        let bytes: Vec<_> = Vec::zcash_deserialize(reader)?;
+        String::from_utf8(bytes).map_err(|_| SerializationError::Parse("invalid utf-8"))
+    }
+}
+
 /// Helper for deserializing more succinctly via type inference
 pub trait ZcashDeserializeInto {
     /// Deserialize based on type inference

zebra-chain/src/transaction/serialize.rs

@@ -44,22 +44,24 @@ impl<P: ZkSnarkProof> ZcashSerialize for JoinSplitData<P> {
 
 impl<P: ZkSnarkProof> ZcashDeserialize for Option<JoinSplitData<P>> {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
-        let joinsplits: Vec<sprout::JoinSplit<P>> = Vec::zcash_deserialize(&mut reader)?;
+        let num_joinsplits = reader.read_compactsize()?;
-
+        match num_joinsplits {
-        if joinsplits.is_empty() {
+            0 => Ok(None),
-            Ok(None)
+            n => {
-        } else {
+                let first = sprout::JoinSplit::zcash_deserialize(&mut reader)?;
-            let (first, rest) = joinsplits
+                let mut rest = Vec::with_capacity((n - 1) as usize);
-                .split_first()
+                for _ in 0..(n - 1) {
-                .expect("a non-empty Vec must have at least one entry");
+                    rest.push(sprout::JoinSplit::zcash_deserialize(&mut reader)?);
-            let pub_key = reader.read_32_bytes()?.into();
+                }
-            let sig = reader.read_64_bytes()?.into();
+                let pub_key = reader.read_32_bytes()?.into();
-            Ok(Some(JoinSplitData {
+                let sig = reader.read_64_bytes()?.into();
-                first: first.clone(),
+                Ok(Some(JoinSplitData {
-                rest: rest.to_vec(),
+                    first,
-                pub_key,
+                    rest,
-                sig,
+                    pub_key,
-            }))
+                    sig,
+                }))
+            }
         }
     }
 }

zebra-network/src/protocol/external/codec.rs

@@ -464,7 +464,7 @@ impl Codec {
                 reader.read_socket_addr()?,
             ),
             nonce: Nonce(reader.read_u64::<LittleEndian>()?),
-            user_agent: reader.read_string()?,
+            user_agent: String::zcash_deserialize(&mut reader)?,
             start_height: block::Height(reader.read_u32::<LittleEndian>()?),
             relay: match reader.read_u8()? {
                 0 => false,
@@ -488,7 +488,7 @@ impl Codec {
 
     fn read_reject<R: Read>(&self, mut reader: R) -> Result<Message, Error> {
         Ok(Message::Reject {
-            message: reader.read_string()?,
+            message: String::zcash_deserialize(&mut reader)?,
             ccode: match reader.read_u8()? {
                 0x01 => RejectReason::Malformed,
                 0x10 => RejectReason::Invalid,
@@ -501,7 +501,7 @@ impl Codec {
                 0x50 => RejectReason::Other,
                 _ => return Err(Error::Parse("invalid RejectReason value in ccode field")),
             },
-            reason: reader.read_string()?,
+            reason: String::zcash_deserialize(&mut reader)?,
             // Sometimes there's data, sometimes there isn't. There's no length
             // field, this is just implicitly encoded by the body_len.
             // Apparently all existing implementations only supply 32 bytes of