From 1f5de6beb9696577d642b4416f283ff0a6de384a Mon Sep 17 00:00:00 2001 From: hashmap Date: Sun, 27 Oct 2019 08:40:52 +0100 Subject: [PATCH] Verify headers and blocks only when needed (#3023) * Verify headers and blocks only when needed Curretnly we have some lightweigt validation implemented as part of entity deserialization, which is safer and allows us to not parse the entire object if some part is invalid. At the same time this logic always applies when we read an entity, eg when reading from DB. This PR introduces UntrustedHeader/Block which is used when we read from the network. It does partial validation during read, then it is supposed to be converted into regular header/block which doesn't validate itself. Also this PR adds "lightweight" validation to block header read like we have for block body, so we don't parse block body if the header is invalid. Fixes #1642 * Move version validation to untrusted header * update fuzz tests --- chain/src/pipe.rs | 31 --- core/fuzz/Cargo.lock | 16 +- core/fuzz/fuzz_targets/block_read_v1.rs | 4 +- core/fuzz/fuzz_targets/block_read_v2.rs | 4 +- .../fuzz_targets/compact_block_read_v1.rs | 5 +- .../fuzz_targets/compact_block_read_v2.rs | 5 +- core/src/core/block.rs | 178 +++++++++++++----- core/src/core/compact_block.rs | 32 +++- core/src/pow/cuckatoo.rs | 4 +- core/src/pow/types.rs | 5 +- p2p/src/protocol.rs | 17 +- 11 files changed, 190 insertions(+), 111 deletions(-) diff --git a/chain/src/pipe.rs b/chain/src/pipe.rs index 987ea3533..4219eb83e 100644 --- a/chain/src/pipe.rs +++ b/chain/src/pipe.rs @@ -25,8 +25,6 @@ use crate::store; use crate::txhashset; use crate::types::{Options, Tip}; use crate::util::RwLock; -use chrono::prelude::Utc; -use chrono::Duration; use grin_store; use std::sync::Arc; @@ -314,35 +312,6 @@ fn prev_header_store( /// to make it as cheap as possible. The different validations are also /// arranged by order of cost to have as little DoS surface as possible. fn validate_header(header: &BlockHeader, ctx: &mut BlockContext<'_>) -> Result<(), Error> { - // check version, enforces scheduled hard fork - if !consensus::valid_header_version(header.height, header.version) { - error!( - "Invalid block header version received ({:?}), maybe update Grin?", - header.version - ); - return Err(ErrorKind::InvalidBlockVersion(header.version).into()); - } - - if header.timestamp > Utc::now() + Duration::seconds(12 * (consensus::BLOCK_TIME_SEC as i64)) { - // refuse blocks more than 12 blocks intervals in future (as in bitcoin) - // TODO add warning in p2p code if local time is too different from peers - return Err(ErrorKind::InvalidBlockTime.into()); - } - - if !ctx.opts.contains(Options::SKIP_POW) { - if !header.pow.is_primary() && !header.pow.is_secondary() { - return Err(ErrorKind::LowEdgebits.into()); - } - let edge_bits = header.pow.edge_bits(); - if !(ctx.pow_verifier)(header).is_ok() { - error!( - "pipe: error validating header with cuckoo edge_bits {}", - edge_bits - ); - return Err(ErrorKind::InvalidPow.into()); - } - } - // First I/O cost, delayed as late as possible. let prev = prev_header_store(header, &mut ctx.batch)?; diff --git a/core/fuzz/Cargo.lock b/core/fuzz/Cargo.lock index 824d84cce..b7609d92f 100644 --- a/core/fuzz/Cargo.lock +++ b/core/fuzz/Cargo.lock @@ -365,7 +365,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "grin_core" -version = "2.1.0-beta.1" +version = "3.0.0-alpha.1" dependencies = [ "blake2-rfc 0.2.18 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -374,8 +374,8 @@ dependencies = [ "enum_primitive 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", "failure 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", "failure_derive 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", - "grin_keychain 2.1.0-beta.1", - "grin_util 2.1.0-beta.1", + "grin_keychain 3.0.0-alpha.1", + "grin_util 3.0.0-alpha.1", "lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "lru-cache 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -393,19 +393,19 @@ dependencies = [ name = "grin_core-fuzz" version = "0.0.3" dependencies = [ - "grin_core 2.1.0-beta.1", - "grin_keychain 2.1.0-beta.1", + "grin_core 3.0.0-alpha.1", + "grin_keychain 3.0.0-alpha.1", "libfuzzer-sys 0.1.0 (git+https://github.com/rust-fuzz/libfuzzer-sys.git)", ] [[package]] name = "grin_keychain" -version = "2.1.0-beta.1" +version = "3.0.0-alpha.1" dependencies = [ "blake2-rfc 0.2.18 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "digest 0.7.6 (registry+https://github.com/rust-lang/crates.io-index)", - "grin_util 2.1.0-beta.1", + "grin_util 3.0.0-alpha.1", "hmac 0.6.3 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", @@ -437,7 +437,7 @@ dependencies = [ [[package]] name = "grin_util" -version = "2.1.0-beta.1" +version = "3.0.0-alpha.1" dependencies = [ "backtrace 0.3.14 (registry+https://github.com/rust-lang/crates.io-index)", "base64 0.9.3 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/core/fuzz/fuzz_targets/block_read_v1.rs b/core/fuzz/fuzz_targets/block_read_v1.rs index 941180274..27cc418b9 100644 --- a/core/fuzz/fuzz_targets/block_read_v1.rs +++ b/core/fuzz/fuzz_targets/block_read_v1.rs @@ -3,10 +3,10 @@ extern crate grin_core; #[macro_use] extern crate libfuzzer_sys; -use grin_core::core::Block; +use grin_core::core::UntrustedBlock; use grin_core::ser; fuzz_target!(|data: &[u8]| { let mut d = data.clone(); - let _t: Result = ser::deserialize(&mut d, ser::ProtocolVersion(1)); + let _t: Result = ser::deserialize(&mut d, ser::ProtocolVersion(1)); }); diff --git a/core/fuzz/fuzz_targets/block_read_v2.rs b/core/fuzz/fuzz_targets/block_read_v2.rs index 3414f5f54..40076b32c 100644 --- a/core/fuzz/fuzz_targets/block_read_v2.rs +++ b/core/fuzz/fuzz_targets/block_read_v2.rs @@ -3,10 +3,10 @@ extern crate grin_core; #[macro_use] extern crate libfuzzer_sys; -use grin_core::core::Block; +use grin_core::core::UntrustedBlock; use grin_core::ser; fuzz_target!(|data: &[u8]| { let mut d = data.clone(); - let _t: Result = ser::deserialize(&mut d, ser::ProtocolVersion(2)); + let _t: Result = ser::deserialize(&mut d, ser::ProtocolVersion(2)); }); diff --git a/core/fuzz/fuzz_targets/compact_block_read_v1.rs b/core/fuzz/fuzz_targets/compact_block_read_v1.rs index 5a77aff2c..2fca2dd0a 100644 --- a/core/fuzz/fuzz_targets/compact_block_read_v1.rs +++ b/core/fuzz/fuzz_targets/compact_block_read_v1.rs @@ -3,10 +3,11 @@ extern crate grin_core; #[macro_use] extern crate libfuzzer_sys; -use grin_core::core::CompactBlock; +use grin_core::core::UntrustedCompactBlock; use grin_core::ser; fuzz_target!(|data: &[u8]| { let mut d = data.clone(); - let _t: Result = ser::deserialize(&mut d, ser::ProtocolVersion(1)); + let _t: Result = + ser::deserialize(&mut d, ser::ProtocolVersion(1)); }); diff --git a/core/fuzz/fuzz_targets/compact_block_read_v2.rs b/core/fuzz/fuzz_targets/compact_block_read_v2.rs index c88ff9fd4..9cac9a577 100644 --- a/core/fuzz/fuzz_targets/compact_block_read_v2.rs +++ b/core/fuzz/fuzz_targets/compact_block_read_v2.rs @@ -3,10 +3,11 @@ extern crate grin_core; #[macro_use] extern crate libfuzzer_sys; -use grin_core::core::CompactBlock; +use grin_core::core::UntrustedCompactBlock; use grin_core::ser; fuzz_target!(|data: &[u8]| { let mut d = data.clone(); - let _t: Result = ser::deserialize(&mut d, ser::ProtocolVersion(2)); + let _t: Result = + ser::deserialize(&mut d, ser::ProtocolVersion(2)); }); diff --git a/core/src/core/block.rs b/core/src/core/block.rs index 71d24bf05..59ece33c5 100644 --- a/core/src/core/block.rs +++ b/core/src/core/block.rs @@ -17,6 +17,7 @@ use crate::util::RwLock; use chrono::naive::{MAX_DATE, MIN_DATE}; use chrono::prelude::{DateTime, NaiveDateTime, Utc}; +use chrono::Duration; use std::collections::HashSet; use std::fmt; use std::iter::FromIterator; @@ -34,7 +35,7 @@ use crate::core::{ use crate::global; use crate::keychain::{self, BlindingFactor}; -use crate::pow::{Difficulty, Proof, ProofOfWork}; +use crate::pow::{verify_size, Difficulty, Proof, ProofOfWork}; use crate::ser::{self, FixedLength, PMMRable, Readable, Reader, Writeable, Writer}; use crate::util::{secp, static_secp_instance}; @@ -52,6 +53,12 @@ pub enum Error { TooHeavy, /// Block weight (based on inputs|outputs|kernels) exceeded. WeightExceeded, + /// Block version is invalid for a given block height + InvalidBlockVersion(HeaderVersion), + /// Block time is invalid + InvalidBlockTime, + /// Invalid POW + InvalidPow, /// Kernel not valid due to lock_height exceeding block header height KernelLockHeight(u64), /// Underlying tx related error @@ -289,48 +296,44 @@ impl Writeable for BlockHeader { } } +fn read_block_header(reader: &mut dyn Reader) -> Result { + let version = HeaderVersion::read(reader)?; + let (height, timestamp) = ser_multiread!(reader, read_u64, read_i64); + let prev_hash = Hash::read(reader)?; + let prev_root = Hash::read(reader)?; + let output_root = Hash::read(reader)?; + let range_proof_root = Hash::read(reader)?; + let kernel_root = Hash::read(reader)?; + let total_kernel_offset = BlindingFactor::read(reader)?; + let (output_mmr_size, kernel_mmr_size) = ser_multiread!(reader, read_u64, read_u64); + let pow = ProofOfWork::read(reader)?; + + if timestamp > MAX_DATE.and_hms(0, 0, 0).timestamp() + || timestamp < MIN_DATE.and_hms(0, 0, 0).timestamp() + { + return Err(ser::Error::CorruptedData); + } + + Ok(BlockHeader { + version, + height, + timestamp: DateTime::::from_utc(NaiveDateTime::from_timestamp(timestamp, 0), Utc), + prev_hash, + prev_root, + output_root, + range_proof_root, + kernel_root, + total_kernel_offset, + output_mmr_size, + kernel_mmr_size, + pow, + }) +} + /// Deserialization of a block header impl Readable for BlockHeader { fn read(reader: &mut dyn Reader) -> Result { - let version = HeaderVersion::read(reader)?; - let (height, timestamp) = ser_multiread!(reader, read_u64, read_i64); - let prev_hash = Hash::read(reader)?; - let prev_root = Hash::read(reader)?; - let output_root = Hash::read(reader)?; - let range_proof_root = Hash::read(reader)?; - let kernel_root = Hash::read(reader)?; - let total_kernel_offset = BlindingFactor::read(reader)?; - let (output_mmr_size, kernel_mmr_size) = ser_multiread!(reader, read_u64, read_u64); - let pow = ProofOfWork::read(reader)?; - - if timestamp > MAX_DATE.and_hms(0, 0, 0).timestamp() - || timestamp < MIN_DATE.and_hms(0, 0, 0).timestamp() - { - return Err(ser::Error::CorruptedData); - } - - // Check the block version before proceeding any further. - // We want to do this here because blocks can be pretty large - // and we want to halt processing as early as possible. - // If we receive an invalid block version then the peer is not on our hard-fork. - if !consensus::valid_header_version(height, version) { - return Err(ser::Error::InvalidBlockVersion); - } - - Ok(BlockHeader { - version, - height, - timestamp: DateTime::::from_utc(NaiveDateTime::from_timestamp(timestamp, 0), Utc), - prev_hash, - prev_root, - output_root, - range_proof_root, - kernel_root, - total_kernel_offset, - output_mmr_size, - kernel_mmr_size, - pow, - }) + read_block_header(reader) } } @@ -397,6 +400,59 @@ impl BlockHeader { } } +impl From for BlockHeader { + fn from(header: UntrustedBlockHeader) -> Self { + header.0 + } +} + +/// Block header which does lightweight validation as part of deserialization, +/// it supposed to be used when we can't trust the channel (eg network) +pub struct UntrustedBlockHeader(BlockHeader); + +/// Deserialization of an untrusted block header +impl Readable for UntrustedBlockHeader { + fn read(reader: &mut dyn Reader) -> Result { + let header = read_block_header(reader)?; + if header.timestamp + > Utc::now() + Duration::seconds(12 * (consensus::BLOCK_TIME_SEC as i64)) + { + // refuse blocks more than 12 blocks intervals in future (as in bitcoin) + // TODO add warning in p2p code if local time is too different from peers + error!( + "block header {} validation error: block time is more than 12 blocks in future", + header.hash() + ); + return Err(ser::Error::CorruptedData); + } + + // Check the block version before proceeding any further. + // We want to do this here because blocks can be pretty large + // and we want to halt processing as early as possible. + // If we receive an invalid block version then the peer is not on our hard-fork. + if !consensus::valid_header_version(header.height, header.version) { + return Err(ser::Error::InvalidBlockVersion); + } + + if !header.pow.is_primary() && !header.pow.is_secondary() { + error!( + "block header {} validation error: invalid edge bits", + header.hash() + ); + return Err(ser::Error::CorruptedData); + } + if let Err(e) = verify_size(&header) { + error!( + "block header {} validation error: invalid POW: {}", + header.hash(), + e + ); + return Err(ser::Error::CorruptedData); + } + Ok(UntrustedBlockHeader(header)) + } +} + /// A block as expressed in the MimbleWimble protocol. The reward is /// non-explicit, assumed to be deducible from block height (similar to /// bitcoin's schedule) and expressed as a global transaction fee (added v.H), @@ -435,16 +491,7 @@ impl Writeable for Block { impl Readable for Block { fn read(reader: &mut dyn Reader) -> Result { let header = BlockHeader::read(reader)?; - let body = TransactionBody::read(reader)?; - - // Now "lightweight" validation of the block. - // Treat any validation issues as data corruption. - // An example of this would be reading a block - // that exceeded the allowed number of inputs. - body.validate_read(Weighting::AsBlock) - .map_err(|_| ser::Error::CorruptedData)?; - Ok(Block { header, body }) } } @@ -770,3 +817,36 @@ impl Block { Ok(()) } } + +impl From for Block { + fn from(block: UntrustedBlock) -> Self { + block.0 + } +} + +/// Block which does lightweight validation as part of deserialization, +/// it supposed to be used when we can't trust the channel (eg network) +pub struct UntrustedBlock(Block); + +/// Deserialization of an untrusted block header +impl Readable for UntrustedBlock { + fn read(reader: &mut dyn Reader) -> Result { + // we validate header here before parsing the body + let header = UntrustedBlockHeader::read(reader)?; + let body = TransactionBody::read(reader)?; + + // Now "lightweight" validation of the block. + // Treat any validation issues as data corruption. + // An example of this would be reading a block + // that exceeded the allowed number of inputs. + body.validate_read(Weighting::AsBlock).map_err(|e| { + error!("read validation error: {}", e); + ser::Error::CorruptedData + })?; + let block = Block { + header: header.into(), + body, + }; + Ok(UntrustedBlock(block)) + } +} diff --git a/core/src/core/compact_block.rs b/core/src/core/compact_block.rs index b56fd72a3..c9aec95c8 100644 --- a/core/src/core/compact_block.rs +++ b/core/src/core/compact_block.rs @@ -16,7 +16,7 @@ use rand::{thread_rng, Rng}; -use crate::core::block::{Block, BlockHeader, Error}; +use crate::core::block::{Block, BlockHeader, Error, UntrustedBlockHeader}; use crate::core::hash::{DefaultHashable, Hashed}; use crate::core::id::ShortIdentifiable; use crate::core::{Output, ShortId, TxKernel}; @@ -221,15 +221,41 @@ impl Readable for CompactBlock { let nonce = reader.read_u64()?; let body = CompactBlockBody::read(reader)?; - let cb = CompactBlock { + Ok(CompactBlock { header, nonce, body, + }) + } +} + +impl From for CompactBlock { + fn from(ucb: UntrustedCompactBlock) -> Self { + ucb.0 + } +} + +/// Compackt block which does lightweight validation as part of deserialization, +/// it supposed to be used when we can't trust the channel (eg network) +pub struct UntrustedCompactBlock(CompactBlock); + +/// Implementation of Readable for an untrusted compact block, defines how to read a +/// compact block from a binary stream. +impl Readable for UntrustedCompactBlock { + fn read(reader: &mut dyn Reader) -> Result { + let header = UntrustedBlockHeader::read(reader)?; + let nonce = reader.read_u64()?; + let body = CompactBlockBody::read(reader)?; + + let cb = CompactBlock { + header: header.into(), + nonce, + body, }; // Now validate the compact block and treat any validation error as corrupted data. cb.validate_read().map_err(|_| ser::Error::CorruptedData)?; - Ok(cb) + Ok(UntrustedCompactBlock(cb)) } } diff --git a/core/src/pow/cuckatoo.rs b/core/src/pow/cuckatoo.rs index 912ebb752..2ce678442 100644 --- a/core/src/pow/cuckatoo.rs +++ b/core/src/pow/cuckatoo.rs @@ -53,6 +53,9 @@ where { /// Create a new graph with given parameters pub fn new(max_edges: T, max_sols: u32, proof_size: usize) -> Result, Error> { + if to_u64!(max_edges) >= u64::max_value() / 2 { + return Err(ErrorKind::Verification(format!("graph is to big to build")))?; + } let max_nodes = 2 * to_u64!(max_edges); Ok(Graph { max_edges, @@ -483,5 +486,4 @@ mod test { } Ok(()) } - } diff --git a/core/src/pow/types.rs b/core/src/pow/types.rs index cee2a4af2..0a477dc9f 100644 --- a/core/src/pow/types.rs +++ b/core/src/pow/types.rs @@ -429,7 +429,7 @@ fn read_number(bits: &Vec, bit_start: usize, bit_count: usize) -> u64 { impl Readable for Proof { fn read(reader: &mut dyn Reader) -> Result { let edge_bits = reader.read_u8()?; - if edge_bits == 0 || edge_bits > 64 { + if edge_bits == 0 || edge_bits > 63 { return Err(ser::Error::CorruptedData); } @@ -509,7 +509,7 @@ mod tests { #[test] fn test_proof_rw() { - for edge_bits in 10..64 { + for edge_bits in 10..63 { let mut proof = Proof::new(gen_proof(edge_bits as u32)); proof.edge_bits = edge_bits; let mut buf = Cursor::new(Vec::new()); @@ -541,5 +541,4 @@ mod tests { } v } - } diff --git a/p2p/src/protocol.rs b/p2p/src/protocol.rs index 0f2646ad4..f03bd3408 100644 --- a/p2p/src/protocol.rs +++ b/p2p/src/protocol.rs @@ -161,13 +161,13 @@ impl MessageHandler for Protocol { "handle_payload: received block: msg_len: {}", msg.header.msg_len ); - let b: core::Block = msg.body()?; + let b: core::UntrustedBlock = msg.body()?; // We default to NONE opts here as we do not know know yet why this block was // received. // If we requested this block from a peer due to our node syncing then // the peer adapter will override opts to reflect this. - adapter.block_received(b, &self.peer_info, chain::Options::NONE)?; + adapter.block_received(b.into(), &self.peer_info, chain::Options::NONE)?; Ok(None) } @@ -190,9 +190,9 @@ impl MessageHandler for Protocol { "handle_payload: received compact block: msg_len: {}", msg.header.msg_len ); - let b: core::CompactBlock = msg.body()?; + let b: core::UntrustedCompactBlock = msg.body()?; - adapter.compact_block_received(b, &self.peer_info)?; + adapter.compact_block_received(b.into(), &self.peer_info)?; Ok(None) } @@ -212,8 +212,8 @@ impl MessageHandler for Protocol { // "header first" block propagation - if we have not yet seen this block // we can go request it from some of our peers Type::Header => { - let header: core::BlockHeader = msg.body()?; - adapter.header_received(header, &self.peer_info)?; + let header: core::UntrustedBlockHeader = msg.body()?; + adapter.header_received(header.into(), &self.peer_info)?; Ok(None) } @@ -229,8 +229,9 @@ impl MessageHandler for Protocol { for chunk in (0..count).collect::>().chunks(chunk_size) { let mut headers = vec![]; for _ in chunk { - let (header, bytes_read) = msg.streaming_read()?; - headers.push(header); + let (header, bytes_read) = + msg.streaming_read::()?; + headers.push(header.into()); total_bytes_read += bytes_read; } adapter.headers_received(&headers, &self.peer_info)?;