[NCC-E005955-M2F] zebra-chain: Check that header block version field is valid when serializing blocks

[teor2345]

Impact

Zebra assumes all serialization/deserialization functions cannot produce invalid data. Unenforced constraints violate this assumption and may affect consensus or interoperability.

Description

Zebra implements the ZCashSerialize trait for consensus-critical data.

zebra/zebra-chain/src/serialization/zcash_serialize.rs

Lines 19 to 30 in 5a88fe7

	pub trait ZcashSerialize: Sized {
	/// Write `self` to the given `writer` using the canonical format.
	///
	/// This function has a `zcash_` prefix to alert the reader that the
	/// serialization in use is consensus-critical serialization, rather than
	/// some other kind of serialization.
	///
	/// Notice that the error type is [`std::io::Error`]; this indicates that
	/// serialization MUST be infallible up to errors in the underlying writer.
	/// In other words, any type implementing `ZcashSerialize` must make illegal
	/// states unrepresentable.
	fn zcash_serialize<W: io::Write>(&self, writer: W) -> Result<(), io::Error>;

Note the highlighted requirement that illegal states must be unrepresentable.

The function zcash_deserialize() for Header contains historical information regarding the version field of the header. In particular, it specifies that the only valid version number is 4, but that incorrect implementations in the past resulted in several blocks with an incorrect value. The function has some special case handling for this, as well as an explicit check that the parsed version is greater than or equal to 4:

zebra/zebra-chain/src/block/serialize.rs

Lines 75 to 82 in 5a88fe7

	// # Consensus
	//
	// > The block version number MUST be greater than or equal to 4.
	//
	// https://zips.z.cash/protocol/protocol.pdf#blockheader
	if version < 4 {
	return Err(SerializationError::Parse("version must be at least 4"));
	}

This is similarly captured in other annotations for the version field, where it is specified that:

The only constraint is that it must be at least 4 when interpreted as an i32 .

In comparison, the corresponding zcash_deserialize() function does not perform validation on the version field:

zebra/zebra-chain/src/block/serialize.rs

Lines 28 to 43 in 5a88fe7

	fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
	writer.write_u32::<LittleEndian>(self.version)?;
	self.previous_block_hash.zcash_serialize(&mut writer)?;
	writer.write_all(&self.merkle_root.0[..])?;
	writer.write_all(&self.commitment_bytes[..])?;
	writer.write_u32::<LittleEndian>(
	self.time
	.timestamp()
	.try_into()
	.expect("deserialized and generated timestamps are u32 values"),
	)?;
	writer.write_u32::<LittleEndian>(self.difficulty_threshold.0)?;
	writer.write_all(&self.nonce[..])?;
	self.solution.zcash_serialize(&mut writer)?;
	Ok(())
	}

No instances within the codebase will currently set this value incorrectly, but it appears to be a violation of the specified constraint. It is possible to construct a Header such that the output of zcash_serialize will not correctly deserialize within Zebra.

Recommendation

Align the constraint checks for the version field such that any serialized Header will correctly deserialize without an error.

Location

zebra-chain/src/block/serialize.rs

Original issue description

Once Zebra starts generating its own blocks, we'll need to do the version check in a few more places:

• make sure serialized blocks contain a version that's 4 or greater, based on the consensus rules
• impose a stricter version check on generated blocks, to ensure the version is always 4

This task is open to contributors, but some parts of it depend on future Zebra features.

[yaahc]

Removing open-to-contributors on this one based on henry's comment pending further discussion in today's gardening session

[oxarbitrage]

Comment from Henry says "not sure about that one" but nothing more. I think we should detail further.

[yaahc]

I think the issue is that this depends on generating blocks which is something we don't have implemented yet, but I'm not sure, @hdevalence?

[oxarbitrage]

I am asking because if that is the case i think we could still check the version in the receiving side just as we did with header_solution and coinbase.

[teor2345]

The BlockVerifier checks the version on blocks as of PR #748.

[teor2345]

Oops, that's not correct, I misread the code.

Note: generated blocks aren't part of the "sync and validate" milestone. They only happen if we implement mining.

[teor2345]

We don't plan on generating blocks any time soon.

[teor2345]

This came up in the audit: there is a missing check in the block header version serialize function.