From 1915634d2bfa2d54e00475722ec1f7118465d123 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 16 Jul 2020 15:09:22 +1000 Subject: [PATCH] Check for bad heights or hashes in checkpoint lists (#639) * Add MIN and MAX for BlockHeight and LockTime * Remove duplicate test cases * fix a comment about the minimum lock time The minimum LockTime::Time is 5 November 1985 00:53:20 UTC, so the first day that only contains valid times is 6 November 1985 (in all timezones). Similarly, the maximum LockTime::Time is 7 February 2106 06::28::15 UTC, so the last day that only contains valid times in all time zones is 5 February 2106. * fix: Reject checkpoint lists with bad hashes or heights Reject the all-zeroes hash, because it is the parent hash of the genesis block, and should never appear in a checkpoint list. Reject checkpoint heights that are greater than the maximum block height. --- zebra-chain/src/block/tests.rs | 20 +++--- zebra-chain/src/transaction/serialize.rs | 6 +- zebra-chain/src/types.rs | 79 +++++++++++++++++++++--- zebra-consensus/src/checkpoint/list.rs | 53 +++++++++++++++- 4 files changed, 134 insertions(+), 24 deletions(-) diff --git a/zebra-chain/src/block/tests.rs b/zebra-chain/src/block/tests.rs index 7556fad5f..776270abe 100644 --- a/zebra-chain/src/block/tests.rs +++ b/zebra-chain/src/block/tests.rs @@ -1,11 +1,14 @@ use super::*; + use crate::equihash_solution::EquihashSolution; use crate::merkle_tree::MerkleTreeRootHash; use crate::note_commitment_tree::SaplingNoteTreeRootHash; use crate::serialization::{ SerializationError, ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize, }; +use crate::types::LockTime; use crate::{sha256d_writer::Sha256dWriter, test::generate}; + use chrono::{DateTime, Duration, LocalResult, TimeZone, Utc}; use proptest::{ arbitrary::{any, Arbitrary}, @@ -299,20 +302,15 @@ static BLOCK_HEADER_VALID_TIMESTAMPS: &[i64] = &[ i64::MIN, i64::MIN + 1, // These times are valid DateTimes - (u32::MIN as i64) - 1, - (u32::MIN as i64), - (u32::MIN as i64) + 1, (i32::MIN as i64) - 1, (i32::MIN as i64), (i32::MIN as i64) + 1, -1, 0, 1, - // maximum nExpiryHeight or lock_time, in blocks - 499_999_999, - // minimum lock_time, in seconds - 500_000_000, - 500_000_001, + LockTime::MIN_TIMESTAMP - 1, + LockTime::MIN_TIMESTAMP, + LockTime::MIN_TIMESTAMP + 1, ]; /// Invalid unix epoch timestamps for blocks, in seconds @@ -320,9 +318,9 @@ static BLOCK_HEADER_INVALID_TIMESTAMPS: &[i64] = &[ (i32::MAX as i64) - 1, (i32::MAX as i64), (i32::MAX as i64) + 1, - (u32::MAX as i64) - 1, - (u32::MAX as i64), - (u32::MAX as i64) + 1, + LockTime::MAX_TIMESTAMP - 1, + LockTime::MAX_TIMESTAMP, + LockTime::MAX_TIMESTAMP + 1, // These times are currently invalid DateTimes, but they could // become valid in future chrono versions i64::MAX - 1, diff --git a/zebra-chain/src/transaction/serialize.rs b/zebra-chain/src/transaction/serialize.rs index f98bb5b6f..86ebbda0d 100644 --- a/zebra-chain/src/transaction/serialize.rs +++ b/zebra-chain/src/transaction/serialize.rs @@ -89,7 +89,7 @@ fn parse_coinbase_height( + ((data[2] as u32) << 8) + ((data[3] as u32) << 16) + ((data[4] as u32) << 24); - if h < 500_000_000 { + if h <= BlockHeight::MAX.0 { Ok((BlockHeight(h), CoinbaseData(data.split_off(5)))) } else { Err(SerializationError::Parse("Invalid block height")) @@ -114,7 +114,7 @@ fn coinbase_height_len(height: BlockHeight) -> usize { 3 } else if let _h @ 65536..=16_777_215 = height.0 { 4 - } else if let _h @ 16_777_216..=499_999_999 = height.0 { + } else if let _h @ 16_777_216..=BlockHeight::MAX_AS_U32 = height.0 { 5 } else { panic!("Invalid coinbase height"); @@ -139,7 +139,7 @@ fn write_coinbase_height(height: BlockHeight, mut w: W) -> Result< w.write_u8(h as u8)?; w.write_u8((h >> 8) as u8)?; w.write_u8((h >> 16) as u8)?; - } else if let h @ 16_777_216..=499_999_999 = height.0 { + } else if let h @ 16_777_216..=BlockHeight::MAX_AS_U32 = height.0 { w.write_u8(0x04)?; w.write_u32::(h)?; } else { diff --git a/zebra-chain/src/types.rs b/zebra-chain/src/types.rs index 2ba5d0907..8c69fdde1 100644 --- a/zebra-chain/src/types.rs +++ b/zebra-chain/src/types.rs @@ -16,7 +16,7 @@ pub mod amount; /// /// # Invariants /// -/// Users should not construct block heights greater than or equal to `500_000_000`. +/// Users should not construct block heights greater than `BlockHeight::MAX`. #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)] pub struct BlockHeight(pub u32); @@ -25,7 +25,9 @@ impl Arbitrary for BlockHeight { type Parameters = (); fn arbitrary_with(_args: ()) -> Self::Strategy { - (0u32..500_000_000_u32).prop_map(BlockHeight).boxed() + (BlockHeight::MIN.0..=BlockHeight::MAX.0) + .prop_map(BlockHeight) + .boxed() } type Strategy = BoxedStrategy; @@ -36,9 +38,12 @@ impl Arbitrary for BlockHeight { /// /// # Invariants /// -/// Users should not construct a `LockTime` with `BlockHeight` greater than or -/// equal to `500_000_000` or a timestamp before 4 November 1985 (Unix timestamp -/// less than `500_000_000`). +/// Users should not construct a `LockTime` with: +/// - a `BlockHeight` greater than MAX_BLOCK_HEIGHT, +/// - a timestamp before 6 November 1985 +/// (Unix timestamp less than MIN_LOCK_TIMESTAMP), or +/// - a timestamp after 5 February 2106 +/// (Unix timestamp greater than MAX_LOCK_TIMESTAMP). #[derive(Copy, Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] pub enum LockTime { /// Unlock at a particular block height. @@ -47,6 +52,61 @@ pub enum LockTime { Time(DateTime), } +impl BlockHeight { + /// The minimum BlockHeight. + /// + /// Due to the underlying type, it is impossible to construct block heights + /// less than `BlockHeight::MIN`. + /// + /// Style note: Sometimes, `BlockHeight::MIN` is less readable than + /// `BlockHeight(0)`. Use whichever makes sense in context. + pub const MIN: BlockHeight = BlockHeight(0); + + /// The maximum BlockHeight. + /// + /// Users should not construct block heights greater than `BlockHeight::MAX`. + pub const MAX: BlockHeight = BlockHeight(499_999_999); + + /// The maximum BlockHeight as a u32, for range patterns. + /// + /// `BlockHeight::MAX.0` can't be used in match range patterns, use this + /// alias instead. + pub const MAX_AS_U32: u32 = Self::MAX.0; +} + +impl LockTime { + /// The minimum LockTime::Time, as a timestamp in seconds. + /// + /// Users should not construct lock times less than `MIN_TIMESTAMP`. + pub const MIN_TIMESTAMP: i64 = 500_000_000; + + /// The maximum LockTime::Time, as a timestamp in seconds. + /// + /// Users should not construct lock times greater than `MAX_TIMESTAMP`. + /// LockTime is u32 in the spec, so times are limited to u32::MAX. + pub const MAX_TIMESTAMP: i64 = u32::MAX as i64; + + /// Returns the minimum LockTime::Time, as a LockTime. + /// + /// Users should not construct lock times less than `min_lock_timestamp`. + // + // When `Utc.timestamp` stabilises as a const function, we can make this a + // const function. + pub fn min_lock_time() -> LockTime { + LockTime::Time(Utc.timestamp(Self::MIN_TIMESTAMP, 0)) + } + + /// Returns the maximum LockTime::Time, as a LockTime. + /// + /// Users should not construct lock times greater than `max_lock_timestamp`. + // + // When `Utc.timestamp` stabilises as a const function, we can make this a + // const function. + pub fn max_lock_time() -> LockTime { + LockTime::Time(Utc.timestamp(Self::MAX_TIMESTAMP, 0)) + } +} + impl ZcashSerialize for LockTime { fn zcash_serialize(&self, mut writer: W) -> Result<(), io::Error> { // This implementation does not check the invariants on `LockTime` so that the @@ -64,7 +124,7 @@ impl ZcashSerialize for LockTime { impl ZcashDeserialize for LockTime { fn zcash_deserialize(mut reader: R) -> Result { let n = reader.read_u32::()?; - if n < 500_000_000 { + if n <= BlockHeight::MAX.0 { Ok(LockTime::Height(BlockHeight(n))) } else { Ok(LockTime::Time(Utc.timestamp(n as i64, 0))) @@ -78,9 +138,10 @@ impl Arbitrary for LockTime { fn arbitrary_with(_args: ()) -> Self::Strategy { prop_oneof![ - (0u32..500_000_000_u32).prop_map(|n| LockTime::Height(BlockHeight(n))), - // LockTime is u32 in the spec, so we are limited to u32::MAX here - (500_000_000..u32::MAX).prop_map(|n| { LockTime::Time(Utc.timestamp(n as i64, 0)) }) + (BlockHeight::MIN.0..=BlockHeight::MAX.0) + .prop_map(|n| LockTime::Height(BlockHeight(n))), + (LockTime::MIN_TIMESTAMP..=LockTime::MAX_TIMESTAMP) + .prop_map(|n| { LockTime::Time(Utc.timestamp(n as i64, 0)) }) ] .boxed() } diff --git a/zebra-consensus/src/checkpoint/list.rs b/zebra-consensus/src/checkpoint/list.rs index 4f7d07c78..72866ebea 100644 --- a/zebra-consensus/src/checkpoint/list.rs +++ b/zebra-consensus/src/checkpoint/list.rs @@ -64,7 +64,18 @@ impl CheckpointList { Err("checkpoint hashes must be unique")?; } - Ok(CheckpointList(checkpoints)) + // Make sure all the hashes are valid. In Bitcoin, [0; 32] is the null + // hash. It is also used as the parent hash of genesis blocks. + if block_hashes.contains(&BlockHeaderHash([0; 32])) { + Err("checkpoint list contains invalid checkpoint hash: found null hash")?; + } + + let checkpoints = CheckpointList(checkpoints); + if checkpoints.max_height() > BlockHeight::MAX { + Err("checkpoint list contains invalid checkpoint: checkpoint height is greater than the maximum block height")?; + } + + Ok(checkpoints) } /// Return true if there is a checkpoint at `height`. @@ -186,6 +197,46 @@ mod tests { Ok(()) } + /// Make sure a checkpoint list that contains a null hash fails + #[test] + fn checkpoint_list_null_hash_fail() -> Result<(), Error> { + let checkpoint_data = vec![(BlockHeight(0), BlockHeaderHash([0; 32]))]; + + // Make a checkpoint list containing the non-genesis block + let checkpoint_list: BTreeMap = + checkpoint_data.iter().cloned().collect(); + let _ = CheckpointList::new(checkpoint_list) + .expect_err("a checkpoint list with a null block hash should fail"); + + Ok(()) + } + + /// Make sure a checkpoint list that contains an invalid block height fails + #[test] + fn checkpoint_list_bad_height_fail() -> Result<(), Error> { + let checkpoint_data = vec![( + BlockHeight(BlockHeight::MAX.0 + 1), + BlockHeaderHash([1; 32]), + )]; + + // Make a checkpoint list containing the non-genesis block + let checkpoint_list: BTreeMap = + checkpoint_data.iter().cloned().collect(); + let _ = CheckpointList::new(checkpoint_list).expect_err( + "a checkpoint list with an invalid block height (BlockHeight::MAX + 1) should fail", + ); + + let checkpoint_data = vec![(BlockHeight(u32::MAX), BlockHeaderHash([1; 32]))]; + + // Make a checkpoint list containing the non-genesis block + let checkpoint_list: BTreeMap = + checkpoint_data.iter().cloned().collect(); + let _ = CheckpointList::new(checkpoint_list) + .expect_err("a checkpoint list with an invalid block height (u32::MAX) should fail"); + + Ok(()) + } + /// Make sure that a checkpoint list containing duplicate blocks fails #[test] fn checkpoint_list_duplicate_blocks_fail() -> Result<(), Error> {