From eb2e58ba53a70b09ba047f928bf26db9b861e44c Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 18 May 2021 08:23:06 +1000 Subject: [PATCH] Security: reject compact sizes greater than the protocol message limit (#2155) These sizes should be impossible in valid messages. So they likely represent a memory preallocation attack. --- zebra-chain/src/block/tests/preallocate.rs | 4 +- zebra-chain/src/serialization/read_zcash.rs | 49 ++++++++++++++------ zebra-chain/src/serialization/write_zcash.rs | 38 +++++++++++---- zebra-chain/src/work/equihash.rs | 5 +- 4 files changed, 69 insertions(+), 27 deletions(-) diff --git a/zebra-chain/src/block/tests/preallocate.rs b/zebra-chain/src/block/tests/preallocate.rs index f6b18b5be..ad35db6d1 100644 --- a/zebra-chain/src/block/tests/preallocate.rs +++ b/zebra-chain/src/block/tests/preallocate.rs @@ -3,7 +3,7 @@ use crate::{ block::{ header::MIN_COUNTED_HEADER_LEN, CountedHeader, Hash, Header, BLOCK_HASH_SIZE, - MAX_PROTOCOL_MESSAGE_LEN, + MAX_BLOCK_BYTES, MAX_PROTOCOL_MESSAGE_LEN, }, serialization::{TrustedPreallocate, ZcashSerialize}, }; @@ -51,7 +51,7 @@ proptest! { /// Confirm that each counted header takes at least COUNTED_HEADER_LEN bytes when serialized. /// This verifies that our calculated `TrustedPreallocate::max_allocation()` is indeed an upper bound. #[test] - fn counted_header_min_length(header in Header::arbitrary_with(()), transaction_count in (0..std::u32::MAX)) { + fn counted_header_min_length(header in Header::arbitrary_with(()), transaction_count in (0..MAX_BLOCK_BYTES)) { let header = CountedHeader { header, transaction_count: transaction_count.try_into().expect("Must run test on platform with at least 32 bit address space"), diff --git a/zebra-chain/src/serialization/read_zcash.rs b/zebra-chain/src/serialization/read_zcash.rs index 0527ace8c..4c1b26b29 100644 --- a/zebra-chain/src/serialization/read_zcash.rs +++ b/zebra-chain/src/serialization/read_zcash.rs @@ -1,9 +1,12 @@ use std::io; -use std::net::{IpAddr, SocketAddr}; +use std::{ + convert::TryInto, + net::{IpAddr, SocketAddr}, +}; use byteorder::{BigEndian, LittleEndian, ReadBytesExt}; -use super::SerializationError; +use super::{SerializationError, MAX_PROTOCOL_MESSAGE_LEN}; /// Extends [`Read`] with methods for writing Zcash/Bitcoin types. /// @@ -17,12 +20,17 @@ pub trait ReadZcashExt: io::Read { /// /// 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 + /// `MAX_PROTOCOL_MESSAGE_LEN / min_serialized_item_bytes` (or a lower limit /// specified by the Zcash consensus rules or Bitcoin network protocol). /// + /// As a defence-in-depth for memory preallocation attacks, + /// Zebra rejects sizes greater than the protocol message length limit. + /// (These sizes should be impossible, because each array items takes at + /// least one byte.) + /// /// # Examples /// - /// ```rust + /// ``` /// use zebra_chain::serialization::ReadZcashExt; /// /// use std::io::Cursor; @@ -41,22 +49,21 @@ pub trait ReadZcashExt: io::Read { /// Cursor::new(b"\xfd\xfd\xaa") /// .read_compactsize().unwrap() /// ); - /// assert_eq!( - /// 0xbbaafd, - /// Cursor::new(b"\xfe\xfd\xaa\xbb\x00") - /// .read_compactsize().unwrap() - /// ); - /// assert_eq!( - /// 0x22ccbbaafd, - /// Cursor::new(b"\xff\xfd\xaa\xbb\xcc\x22\x00\x00\x00") - /// .read_compactsize().unwrap() - /// ); + /// ``` + /// + /// Sizes greater than the maximum network message length are invalid, + /// they return a `Parse` error: + /// ``` + /// # use zebra_chain::serialization::ReadZcashExt; + /// # use std::io::Cursor; + /// Cursor::new(b"\xfe\xfd\xaa\xbb\x00").read_compactsize().unwrap_err(); + /// Cursor::new(b"\xff\xfd\xaa\xbb\xcc\x22\x00\x00\x00").read_compactsize().unwrap_err(); /// ``` #[inline] fn read_compactsize(&mut self) -> Result { use SerializationError::Parse; let flag_byte = self.read_u8()?; - match flag_byte { + let size = match flag_byte { n @ 0x00..=0xfc => Ok(n as u64), 0xfd => match self.read_u16::()? { n @ 0x0000_00fd..=0x0000_ffff => Ok(n as u64), @@ -70,7 +77,19 @@ pub trait ReadZcashExt: io::Read { n @ 0x1_0000_0000..=0xffff_ffff_ffff_ffff => Ok(n), _ => Err(Parse("non-canonical compactsize")), }, + }?; + + // # Security + // Defence-in-depth for memory DoS via preallocation. + if size + > MAX_PROTOCOL_MESSAGE_LEN + .try_into() + .expect("usize fits in u64") + { + Err(Parse("compactsize larger than protocol message limit"))?; } + + Ok(size) } /// Read an IP address in Bitcoin format. diff --git a/zebra-chain/src/serialization/write_zcash.rs b/zebra-chain/src/serialization/write_zcash.rs index ad53aeedc..40013eaf6 100644 --- a/zebra-chain/src/serialization/write_zcash.rs +++ b/zebra-chain/src/serialization/write_zcash.rs @@ -1,6 +1,10 @@ use std::io; -use std::net::{IpAddr, SocketAddr}; +use std::{ + convert::TryInto, + net::{IpAddr, SocketAddr}, +}; +use super::MAX_PROTOCOL_MESSAGE_LEN; use byteorder::{BigEndian, LittleEndian, WriteBytesExt}; /// Extends [`Write`] with methods for writing Zcash/Bitcoin types. @@ -9,9 +13,16 @@ use byteorder::{BigEndian, LittleEndian, WriteBytesExt}; pub trait WriteZcashExt: io::Write { /// Writes a `u64` using the Bitcoin `CompactSize` encoding. /// + /// # Panics + /// + /// Zebra panics on sizes greater than the protocol message length limit. + /// This is a defence-in-depth for memory preallocation attacks. + /// (These sizes should be impossible, because each array item takes at + /// least one byte.) + /// /// # Examples /// - /// ```rust + /// ``` /// use zebra_chain::serialization::WriteZcashExt; /// /// let mut buf = Vec::new(); @@ -25,17 +36,28 @@ pub trait WriteZcashExt: io::Write { /// let mut buf = Vec::new(); /// buf.write_compactsize(0xaafd).unwrap(); /// assert_eq!(buf, b"\xfd\xfd\xaa"); + /// ``` /// + /// Sizes greater than the maximum network message length are invalid + /// and cause a panic: + /// ```should_panic + /// # use zebra_chain::serialization::WriteZcashExt; /// let mut buf = Vec::new(); - /// buf.write_compactsize(0xbbaafd).unwrap(); - /// assert_eq!(buf, b"\xfe\xfd\xaa\xbb\x00"); - /// - /// let mut buf = Vec::new(); - /// buf.write_compactsize(0x22ccbbaafd).unwrap(); - /// assert_eq!(buf, b"\xff\xfd\xaa\xbb\xcc\x22\x00\x00\x00"); + /// buf.write_compactsize(0xbbaafd).unwrap_err(); + /// buf.write_compactsize(0x22ccbbaafd).unwrap_err(); /// ``` #[inline] fn write_compactsize(&mut self, n: u64) -> io::Result<()> { + // # Security + // Defence-in-depth for memory DoS via preallocation. + // + if n > MAX_PROTOCOL_MESSAGE_LEN + .try_into() + .expect("usize fits in u64") + { + panic!("compactsize larger than protocol message limit"); + } + match n { 0x0000_0000..=0x0000_00fc => self.write_u8(n as u8), 0x0000_00fd..=0x0000_ffff => { diff --git a/zebra-chain/src/work/equihash.rs b/zebra-chain/src/work/equihash.rs index ce6cea601..9497f8d4f 100644 --- a/zebra-chain/src/work/equihash.rs +++ b/zebra-chain/src/work/equihash.rs @@ -108,6 +108,7 @@ impl ZcashDeserialize for Solution { #[cfg(test)] mod tests { use super::*; + use crate::block::MAX_BLOCK_BYTES; static EQUIHASH_SIZE_TESTS: &[u64] = &[ 0, @@ -115,8 +116,8 @@ mod tests { (SOLUTION_SIZE - 1) as u64, SOLUTION_SIZE as u64, (SOLUTION_SIZE + 1) as u64, - u64::MAX - 1, - u64::MAX, + MAX_BLOCK_BYTES - 1, + MAX_BLOCK_BYTES, ]; #[test]