From b600e82d6e6f54656f05e36ce586dec679214348 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 18 May 2021 06:53:10 +1000 Subject: [PATCH] Security: Avoid silently corrupting invalid times during serialization (#2149) * Security: panic if an internally generated time is out of range If Zebra has a bug where it generates blocks, transactions, or meta addresses with bad times, panic. This avoids sending bad data onto the network. (Previously, Zebra would truncate some of these times, silently corrupting the underlying data.) Make it clear that deserialization of these objects is infalliable. --- zebra-chain/src/block/serialize.rs | 12 +++++++----- zebra-chain/src/transaction/lock_time.rs | 8 +++++--- zebra-network/src/meta_addr.rs | 11 +++++++++-- zebra-network/src/meta_addr/arbitrary.rs | 10 +++++----- 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/zebra-chain/src/block/serialize.rs b/zebra-chain/src/block/serialize.rs index 0bb1f6b4..f05ec609 100644 --- a/zebra-chain/src/block/serialize.rs +++ b/zebra-chain/src/block/serialize.rs @@ -27,10 +27,12 @@ impl ZcashSerialize for Header { self.previous_block_hash.zcash_serialize(&mut writer)?; writer.write_all(&self.merkle_root.0[..])?; writer.write_all(&self.commitment_bytes[..])?; - // this is a truncating cast, rather than a saturating cast - // but u32 times are valid until 2106, and our block verification time - // checks should detect any truncation. - writer.write_u32::(self.time.timestamp() as u32)?; + writer.write_u32::( + self.time + .timestamp() + .try_into() + .expect("deserialized and generated timestamps are u32 values"), + )?; writer.write_u32::(self.difficulty_threshold.0)?; writer.write_all(&self.nonce[..])?; self.solution.zcash_serialize(&mut writer)?; @@ -76,7 +78,7 @@ impl ZcashDeserialize for Header { merkle_root: merkle::Root(reader.read_32_bytes()?), commitment_bytes: reader.read_32_bytes()?, // This can't panic, because all u32 values are valid `Utc.timestamp`s - time: Utc.timestamp(reader.read_u32::()? as i64, 0), + time: Utc.timestamp(reader.read_u32::()?.into(), 0), difficulty_threshold: CompactDifficulty(reader.read_u32::()?), nonce: reader.read_32_bytes()?, solution: equihash::Solution::zcash_deserialize(reader)?, diff --git a/zebra-chain/src/transaction/lock_time.rs b/zebra-chain/src/transaction/lock_time.rs index 3c0db3a0..160e8c25 100644 --- a/zebra-chain/src/transaction/lock_time.rs +++ b/zebra-chain/src/transaction/lock_time.rs @@ -1,4 +1,4 @@ -use std::io; +use std::{convert::TryInto, io}; use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt}; use chrono::{DateTime, TimeZone, Utc}; @@ -65,7 +65,8 @@ impl ZcashSerialize for LockTime { // we can always compute a hash of a transaction object. match self { LockTime::Height(block::Height(n)) => writer.write_u32::(*n)?, - LockTime::Time(t) => writer.write_u32::(t.timestamp() as u32)?, + LockTime::Time(t) => writer + .write_u32::(t.timestamp().try_into().expect("time is in range"))?, } Ok(()) } @@ -77,7 +78,8 @@ impl ZcashDeserialize for LockTime { if n <= block::Height::MAX.0 { Ok(LockTime::Height(block::Height(n))) } else { - Ok(LockTime::Time(Utc.timestamp(n as i64, 0))) + // This can't panic, because all u32 values are valid `Utc.timestamp`s + Ok(LockTime::Time(Utc.timestamp(n.into(), 0))) } } } diff --git a/zebra-network/src/meta_addr.rs b/zebra-network/src/meta_addr.rs index 5bbbd302..39b258fb 100644 --- a/zebra-network/src/meta_addr.rs +++ b/zebra-network/src/meta_addr.rs @@ -2,6 +2,7 @@ use std::{ cmp::{Ord, Ordering}, + convert::TryInto, io::{Read, Write}, net::SocketAddr, }; @@ -309,7 +310,12 @@ impl PartialOrd for MetaAddr { impl ZcashSerialize for MetaAddr { fn zcash_serialize(&self, mut writer: W) -> Result<(), std::io::Error> { - writer.write_u32::(self.get_last_seen().timestamp() as u32)?; + writer.write_u32::( + self.get_last_seen() + .timestamp() + .try_into() + .expect("time is in range"), + )?; writer.write_u64::(self.services.bits())?; writer.write_socket_addr(self.addr)?; Ok(()) @@ -318,7 +324,8 @@ impl ZcashSerialize for MetaAddr { impl ZcashDeserialize for MetaAddr { fn zcash_deserialize(mut reader: R) -> Result { - let last_seen = Utc.timestamp(reader.read_u32::()? as i64, 0); + // This can't panic, because all u32 values are valid `Utc.timestamp`s + let last_seen = Utc.timestamp(reader.read_u32::()?.into(), 0); let services = PeerServices::from_bits_truncate(reader.read_u64::()?); let addr = reader.read_socket_addr()?; diff --git a/zebra-network/src/meta_addr/arbitrary.rs b/zebra-network/src/meta_addr/arbitrary.rs index 6352d83e..3c2231af 100644 --- a/zebra-network/src/meta_addr/arbitrary.rs +++ b/zebra-network/src/meta_addr/arbitrary.rs @@ -2,7 +2,8 @@ use proptest::{arbitrary::any, arbitrary::Arbitrary, prelude::*}; use super::{MetaAddr, PeerAddrState, PeerServices}; -use std::{net::SocketAddr, time::SystemTime}; +use chrono::{TimeZone, Utc}; +use std::net::SocketAddr; impl Arbitrary for MetaAddr { type Parameters = (); @@ -11,16 +12,15 @@ impl Arbitrary for MetaAddr { ( any::(), any::(), - any::(), + any::(), any::(), ) .prop_map( |(addr, services, last_seen, last_connection_state)| MetaAddr { addr, services, - // TODO: implement constraints on last_seen as part of the - // last_connection_state refactor in #1849 - last_seen: last_seen.into(), + // This can't panic, because all u32 values are valid `Utc.timestamp`s + last_seen: Utc.timestamp(last_seen.into(), 0), last_connection_state, }, )