Stop untrusted preallocation during deserialization

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/transparent/script.rs

@@ -1,11 +1,10 @@
+//! Bitcoin script for Zebra
+
 #![allow(clippy::unit_arg)]
-use crate::serialization::{
-    ReadZcashExt, SerializationError, WriteZcashExt, ZcashDeserialize, ZcashSerialize,
-};
-use std::{
-    fmt,
-    io::{self, Read},
-};
+
+use crate::serialization::{SerializationError, WriteZcashExt, ZcashDeserialize, ZcashSerialize};
+
+use std::{fmt, io};
 
 /// An encoding of a Bitcoin script.
 #[derive(Clone, Eq, PartialEq, Serialize, Deserialize, Hash)]
@@ -32,12 +31,8 @@ impl ZcashSerialize for Script {
 }
 
 impl ZcashDeserialize for Script {
-    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
-        // XXX what is the max length of a script?
-        let len = reader.read_compactsize()?;
-        let mut bytes = Vec::new();
-        reader.take(len).read_to_end(&mut bytes)?;
-        Ok(Script(bytes))
+    fn zcash_deserialize<R: io::Read>(reader: R) -> Result<Self, SerializationError> {
+        Ok(Script(Vec::zcash_deserialize(reader)?))
     }
 }
 

zebra-chain/src/transparent/serialize.rs

@@ -1,4 +1,4 @@
-use std::io::{self, Read};
+use std::io;
 
 use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt};
 
@@ -196,8 +196,9 @@ impl ZcashDeserialize for Input {
             if len > 100 {
                 return Err(SerializationError::Parse("coinbase has too much data"));
             }
-            let mut data = Vec::with_capacity(len as usize);
-            (&mut reader).take(len).read_to_end(&mut data)?;
+            // Memory Denial of Service: this length has just been checked
+            let mut data = vec![0; len as usize];
+            reader.read_exact(&mut data[..])?;
             let (height, data) = parse_coinbase_height(data)?;
             let sequence = reader.read_u32::<LittleEndian>()?;
             Ok(Input::Coinbase {

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

@@ -1,7 +1,10 @@
 //! A Tokio codec mapping byte streams to Bitcoin message streams.
 
 use std::fmt;
-use std::io::{Cursor, Read, Write};
+use std::{
+    cmp::min,
+    io::{Cursor, Read, Write},
+};
 
 use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt};
 use bytes::{BufMut, BytesMut};
@@ -423,7 +426,7 @@ impl Decoder for Codec {
                     b"tx\0\0\0\0\0\0\0\0\0\0" => self.read_tx(&mut body_reader),
                     b"mempool\0\0\0\0\0" => self.read_mempool(&mut body_reader),
                     b"filterload\0\0" => self.read_filterload(&mut body_reader, body_len),
-                    b"filteradd\0\0\0" => self.read_filteradd(&mut body_reader),
+                    b"filteradd\0\0\0" => self.read_filteradd(&mut body_reader, body_len),
                     b"filterclear\0" => self.read_filterclear(&mut body_reader),
                     _ => return Err(Parse("unknown command")),
                 }
@@ -464,7 +467,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 +491,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 +504,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
@@ -585,15 +588,15 @@ impl Codec {
     }
 
     fn read_filterload<R: Read>(&self, mut reader: R, body_len: usize) -> Result<Message, Error> {
+        const MAX_FILTERLOAD_LENGTH: usize = 36000;
+        const FILTERLOAD_REMAINDER_LENGTH: usize = 4 + 4 + 1;
+
         if !(FILTERLOAD_REMAINDER_LENGTH <= body_len
-            && body_len <= FILTERLOAD_REMAINDER_LENGTH + MAX_FILTER_LENGTH)
+            && body_len <= FILTERLOAD_REMAINDER_LENGTH + MAX_FILTERLOAD_LENGTH)
         {
             return Err(Error::Parse("Invalid filterload message body length."));
         }
 
-        const MAX_FILTER_LENGTH: usize = 36000;
-        const FILTERLOAD_REMAINDER_LENGTH: usize = 4 + 4 + 1;
-
         let filter_length: usize = body_len - FILTERLOAD_REMAINDER_LENGTH;
 
         let mut filter_bytes = vec![0; filter_length];
@@ -607,13 +610,16 @@ impl Codec {
         })
     }
 
-    fn read_filteradd<R: Read>(&self, reader: R) -> Result<Message, Error> {
-        let mut bytes = Vec::new();
+    fn read_filteradd<R: Read>(&self, mut reader: R, body_len: usize) -> Result<Message, Error> {
+        const MAX_FILTERADD_LENGTH: usize = 520;
+
+        let filter_length: usize = min(body_len, MAX_FILTERADD_LENGTH);
 
-        // Maximum size of data is 520 bytes.
-        reader.take(520).read_exact(&mut bytes)?;
+        // Memory Denial of Service: this length has just been bounded
+        let mut filter_bytes = vec![0; filter_length];
+        reader.read_exact(&mut filter_bytes)?;
 
-        Ok(Message::FilterAdd { data: bytes })
+        Ok(Message::FilterAdd { data: filter_bytes })
     }
 
     fn read_filterclear<R: Read>(&self, mut _reader: R) -> Result<Message, Error> {