From 403c0745021c59e8c945de7b0bafa22d5b4c21bb Mon Sep 17 00:00:00 2001 From: Arya Date: Wed, 12 Apr 2023 00:54:02 -0400 Subject: [PATCH] fix(chain): Validate header versions when serializing blocks (#6475) * checks block header version when serializing * fixes test panic * adds docs * test block header (de)serialization --- zebra-chain/src/block/serialize.rs | 77 ++++++++++++++----------- zebra-chain/src/block/tests/vectors.rs | 62 +++++++++++++++++++- zebra-consensus/src/checkpoint/tests.rs | 2 +- 3 files changed, 102 insertions(+), 39 deletions(-) diff --git a/zebra-chain/src/block/serialize.rs b/zebra-chain/src/block/serialize.rs index 4082df023e3..e763915e499 100644 --- a/zebra-chain/src/block/serialize.rs +++ b/zebra-chain/src/block/serialize.rs @@ -22,9 +22,49 @@ use crate::{ /// transaction in the chain is approximately 1.5 kB smaller.) pub const MAX_BLOCK_BYTES: u64 = 2_000_000; +/// Checks if a block header version is valid. +/// +/// Zebra could encounter a [`Header`] with an invalid version when serializing a block header constructed +/// in memory with the wrong version in tests or the getblocktemplate RPC. +/// +/// The getblocktemplate RPC generates a template with version 4. The miner generates the actual block, +/// and then we deserialize it and do this check. +/// +/// All other blocks are deserialized when we receive them, and never modified, +/// so the deserialisation would pick up any errors. +fn check_version(version: u32) -> Result<(), &'static str> { + match version { + // The Zcash specification says that: + // "The current and only defined block version number for Zcash is 4." + // but this is not actually part of the consensus rules, and in fact + // broken mining software created blocks that do not have version 4. + // There are approximately 4,000 blocks with version 536870912; this + // is the bit-reversal of the value 4, indicating that that mining pool + // reversed bit-ordering of the version field. Because the version field + // was not properly validated, these blocks were added to the chain. + // + // The only possible way to work around this is to do a similar hack + // as the overwintered field in transaction parsing, which we do here: + // treat the high bit (which zcashd interprets as a sign bit) as an + // indicator that the version field is meaningful. + version if version >> 31 != 0 => Err("high bit was set in version field"), + + // # Consensus + // + // > The block version number MUST be greater than or equal to 4. + // + // https://zips.z.cash/protocol/protocol.pdf#blockheader + version if version < ZCASH_BLOCK_VERSION => Err("version must be at least 4"), + + _ => Ok(()), + } +} + impl ZcashSerialize for Header { #[allow(clippy::unwrap_in_result)] fn zcash_serialize(&self, mut writer: W) -> Result<(), io::Error> { + check_version(self.version).map_err(|msg| io::Error::new(io::ErrorKind::Other, msg))?; + writer.write_u32::(self.version)?; self.previous_block_hash.zcash_serialize(&mut writer)?; writer.write_all(&self.merkle_root.0[..])?; @@ -44,41 +84,8 @@ impl ZcashSerialize for Header { impl ZcashDeserialize for Header { fn zcash_deserialize(mut reader: R) -> Result { - // The Zcash specification says that - // "The current and only defined block version number for Zcash is 4." - // but this is not actually part of the consensus rules, and in fact - // broken mining software created blocks that do not have version 4. - // There are approximately 4,000 blocks with version 536870912; this - // is the bit-reversal of the value 4, indicating that that mining pool - // reversed bit-ordering of the version field. Because the version field - // was not properly validated, these blocks were added to the chain. - // - // The only possible way to work around this is to do a similar hack - // as the overwintered field in transaction parsing, which we do here: - // treat the high bit (which zcashd interprets as a sign bit) as an - // indicator that the version field is meaningful. - // - // - let (version, future_version_flag) = { - const LOW_31_BITS: u32 = (1 << 31) - 1; - let raw_version = reader.read_u32::()?; - (raw_version & LOW_31_BITS, raw_version >> 31 != 0) - }; - - if future_version_flag { - return Err(SerializationError::Parse( - "high bit was set in version field", - )); - } - - // # Consensus - // - // > The block version number MUST be greater than or equal to 4. - // - // https://zips.z.cash/protocol/protocol.pdf#blockheader - if version < ZCASH_BLOCK_VERSION { - return Err(SerializationError::Parse("version must be at least 4")); - } + let version = reader.read_u32::()?; + check_version(version).map_err(SerializationError::Parse)?; Ok(Header { version, diff --git a/zebra-chain/src/block/tests/vectors.rs b/zebra-chain/src/block/tests/vectors.rs index 2d05cc39777..257614becf1 100644 --- a/zebra-chain/src/block/tests/vectors.rs +++ b/zebra-chain/src/block/tests/vectors.rs @@ -13,7 +13,9 @@ use crate::{ Network::{self, *}, NetworkUpgrade::*, }, - serialization::{sha256d, ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize}, + serialization::{ + sha256d, SerializationError, ZcashDeserialize, ZcashDeserializeInto, ZcashSerialize, + }, transaction::LockTime, }; @@ -63,7 +65,7 @@ fn blockheaderhash_from_blockheader() { } #[test] -fn deserialize_blockheader() { +fn blockheader_serialization() { let _init_guard = zebra_test::init(); // Includes the 32-byte nonce and 3-byte equihash length field. @@ -73,11 +75,65 @@ fn deserialize_blockheader() { + crate::work::equihash::SOLUTION_SIZE; for block in zebra_test::vectors::BLOCKS.iter() { + // successful deserialization + let header_bytes = &block[..BLOCK_HEADER_LENGTH]; - let _header = header_bytes + let mut header = header_bytes .zcash_deserialize_into::
() .expect("blockheader test vector should deserialize"); + + // successful serialization + + let _serialized_header = header + .zcash_serialize_to_vec() + .expect("blockheader test vector should serialize"); + + // deserialiation errors + + let header_bytes = [&[255; 4], &header_bytes[4..]].concat(); + + let deserialization_err = header_bytes + .zcash_deserialize_into::
() + .expect_err("blockheader test vector should fail to deserialize"); + + let SerializationError::Parse(err_msg) = deserialization_err else { + panic!("SerializationError variant should be Parse") + }; + + assert_eq!(err_msg, "high bit was set in version field"); + + let header_bytes = [&[0; 4], &header_bytes[4..]].concat(); + + let deserialization_err = header_bytes + .zcash_deserialize_into::
() + .expect_err("blockheader test vector should fail to deserialize"); + + let SerializationError::Parse(err_msg) = deserialization_err else { + panic!("SerializationError variant should be Parse") + }; + + assert_eq!(err_msg, "version must be at least 4"); + + // serialiation errors + + header.version = u32::MAX; + + let serialization_err = header + .zcash_serialize_to_vec() + .expect_err("blockheader test vector with modified version should fail to serialize"); + + assert_eq!( + serialization_err.kind(), + std::io::ErrorKind::Other, + "error kind should be Other" + ); + + let err_msg = serialization_err + .into_inner() + .expect("there should be an inner error"); + + assert_eq!(err_msg.to_string(), "high bit was set in version field"); } } diff --git a/zebra-consensus/src/checkpoint/tests.rs b/zebra-consensus/src/checkpoint/tests.rs index 85fe894951e..66331310735 100644 --- a/zebra-consensus/src/checkpoint/tests.rs +++ b/zebra-consensus/src/checkpoint/tests.rs @@ -504,7 +504,7 @@ async fn wrong_checkpoint_hash_fail() -> Result<(), Report> { // Change the header hash let mut bad_block0 = good_block0.clone(); let bad_block0_mut = Arc::make_mut(&mut bad_block0); - Arc::make_mut(&mut bad_block0_mut.header).version = 0; + Arc::make_mut(&mut bad_block0_mut.header).version = 5; // Make a checkpoint list containing the genesis block checkpoint let genesis_checkpoint_list: BTreeMap =