From 646f1b0c4a6f5100fca02a43ecadc00441cc3fb2 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 3 May 2018 08:01:13 +0100 Subject: [PATCH] Don't panic in import_block if invalid rlp (#8522) * Don't panic in import_block if invalid rlp * Remove redundant type annotation * Replace RLP header view usage with safe decoding Using the view will panic with invalid RLP. Here we use Rlp decoding directly which will return a `Result<_, DecoderError>`. While this path currently should not have any invalid RLP - it makes it safer if ever called with invalid RLP from other code paths. --- ethcore/src/client/client.rs | 16 +++++++--------- ethcore/src/error.rs | 2 ++ ethcore/src/tests/client.rs | 20 ++++++++++++++++++++ ethcore/src/verification/queue/kind.rs | 9 ++++----- ethcore/src/verification/queue/mod.rs | 23 ++++++++++++++--------- 5 files changed, 47 insertions(+), 23 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index bd6201a15f3..a580a58f13a 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -452,9 +452,7 @@ impl Importer { /// /// The block is guaranteed to be the next best blocks in the /// first block sequence. Does no sealing or transaction validation. - fn import_old_block(&self, block_bytes: Bytes, receipts_bytes: Bytes, db: &KeyValueDB, chain: &BlockChain) -> Result { - let block = view!(BlockView, &block_bytes); - let header = block.header(); + fn import_old_block(&self, header: &Header, block_bytes: Bytes, receipts_bytes: Bytes, db: &KeyValueDB, chain: &BlockChain) -> Result { let receipts = ::rlp::decode_list(&receipts_bytes); let hash = header.hash(); let _import_lock = self.import_lock.lock(); @@ -1413,7 +1411,7 @@ impl ImportBlock for Client { use verification::queue::kind::blocks::Unverified; // create unverified block here so the `keccak` calculation can be cached. - let unverified = Unverified::new(bytes); + let unverified = Unverified::from_rlp(bytes)?; { if self.chain.read().is_known(&unverified.hash()) { @@ -1428,19 +1426,19 @@ impl ImportBlock for Client { } fn import_block_with_receipts(&self, block_bytes: Bytes, receipts_bytes: Bytes) -> Result { + let header: Header = ::rlp::Rlp::new(&block_bytes).val_at(0)?; { // check block order - let header = view!(BlockView, &block_bytes).header_view(); if self.chain.read().is_known(&header.hash()) { bail!(BlockImportErrorKind::Import(ImportErrorKind::AlreadyInChain)); } - let status = self.block_status(BlockId::Hash(header.parent_hash())); - if status == BlockStatus::Unknown || status == BlockStatus::Pending { - bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(header.parent_hash()))); + let status = self.block_status(BlockId::Hash(*header.parent_hash())); + if status == BlockStatus::Unknown || status == BlockStatus::Pending { + bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(*header.parent_hash()))); } } - self.importer.import_old_block(block_bytes, receipts_bytes, &**self.db.read(), &*self.chain.read()).map_err(Into::into) + self.importer.import_old_block(&header, block_bytes, receipts_bytes, &**self.db.read(), &*self.chain.read()).map_err(Into::into) } } diff --git a/ethcore/src/error.rs b/ethcore/src/error.rs index b68bf355347..561701e7620 100644 --- a/ethcore/src/error.rs +++ b/ethcore/src/error.rs @@ -190,6 +190,7 @@ error_chain! { foreign_links { Block(BlockError) #[doc = "Block error"]; + Decoder(::rlp::DecoderError) #[doc = "Rlp decoding error"]; } errors { @@ -206,6 +207,7 @@ impl From for BlockImportError { match e { Error(ErrorKind::Block(block_error), _) => BlockImportErrorKind::Block(block_error).into(), Error(ErrorKind::Import(import_error), _) => BlockImportErrorKind::Import(import_error.into()).into(), + Error(ErrorKind::Util(util_error::ErrorKind::Decoder(decoder_err)), _) => BlockImportErrorKind::Decoder(decoder_err).into(), _ => BlockImportErrorKind::Other(format!("other block import error: {:?}", e)).into(), } } diff --git a/ethcore/src/tests/client.rs b/ethcore/src/tests/client.rs index 0b8200e938a..6dcad9ba62f 100644 --- a/ethcore/src/tests/client.rs +++ b/ethcore/src/tests/client.rs @@ -36,6 +36,7 @@ use views::BlockView; use ethkey::KeyPair; use transaction::{PendingTransaction, Transaction, Action, Condition}; use miner::MinerService; +use rlp::{RlpStream, EMPTY_LIST_RLP}; use tempdir::TempDir; #[test] @@ -111,6 +112,25 @@ fn imports_good_block() { assert!(!block.into_inner().is_empty()); } +#[test] +fn fails_to_import_block_with_invalid_rlp() { + use error::{BlockImportError, BlockImportErrorKind}; + + let client = generate_dummy_client(6); + let mut rlp = RlpStream::new_list(3); + rlp.append_raw(&EMPTY_LIST_RLP, 1); // empty header + rlp.append_raw(&EMPTY_LIST_RLP, 1); + rlp.append_raw(&EMPTY_LIST_RLP, 1); + let invalid_header_block = rlp.out(); + + match client.import_block(invalid_header_block) { + Err(BlockImportError(BlockImportErrorKind::Decoder(_), _)) => (), // all good + Err(_) => panic!("Should fail with a decoder error"), + Ok(_) => panic!("Should not import block with invalid header"), + } +} + + #[test] fn query_none_block() { let tempdir = TempDir::new("").unwrap(); diff --git a/ethcore/src/verification/queue/kind.rs b/ethcore/src/verification/queue/kind.rs index 7007da5be1f..ce9bddf4efe 100644 --- a/ethcore/src/verification/queue/kind.rs +++ b/ethcore/src/verification/queue/kind.rs @@ -119,14 +119,13 @@ pub mod blocks { impl Unverified { /// Create an `Unverified` from raw bytes. - pub fn new(bytes: Bytes) -> Self { - use views::BlockView; + pub fn from_rlp(bytes: Bytes) -> Result { - let header = view!(BlockView, &bytes).header(); - Unverified { + let header = ::rlp::Rlp::new(&bytes).val_at(0)?; + Ok(Unverified { header: header, bytes: bytes, - } + }) } } diff --git a/ethcore/src/verification/queue/mod.rs b/ethcore/src/verification/queue/mod.rs index ca633b0f3de..f7a558f33d1 100644 --- a/ethcore/src/verification/queue/mod.rs +++ b/ethcore/src/verification/queue/mod.rs @@ -734,6 +734,7 @@ mod tests { use test_helpers::{get_good_dummy_block_seq, get_good_dummy_block}; use error::*; use views::BlockView; + use bytes::Bytes; // create a test block queue. // auto_scaling enables verifier adjustment. @@ -746,6 +747,10 @@ mod tests { BlockQueue::new(config, engine, IoChannel::disconnected(), true) } + fn new_unverified(bytes: Bytes) -> Unverified { + Unverified::from_rlp(bytes).expect("Should be valid rlp") + } + #[test] fn can_be_created() { // TODO better test @@ -757,7 +762,7 @@ mod tests { #[test] fn can_import_blocks() { let queue = get_test_queue(false); - if let Err(e) = queue.import(Unverified::new(get_good_dummy_block())) { + if let Err(e) = queue.import(new_unverified(get_good_dummy_block())) { panic!("error importing block that is valid by definition({:?})", e); } } @@ -765,11 +770,11 @@ mod tests { #[test] fn returns_error_for_duplicates() { let queue = get_test_queue(false); - if let Err(e) = queue.import(Unverified::new(get_good_dummy_block())) { + if let Err(e) = queue.import(new_unverified(get_good_dummy_block())) { panic!("error importing block that is valid by definition({:?})", e); } - let duplicate_import = queue.import(Unverified::new(get_good_dummy_block())); + let duplicate_import = queue.import(new_unverified(get_good_dummy_block())); match duplicate_import { Err(e) => { match e { @@ -786,7 +791,7 @@ mod tests { let queue = get_test_queue(false); let block = get_good_dummy_block(); let hash = view!(BlockView, &block).header().hash().clone(); - if let Err(e) = queue.import(Unverified::new(block)) { + if let Err(e) = queue.import(new_unverified(block)) { panic!("error importing block that is valid by definition({:?})", e); } queue.flush(); @@ -802,14 +807,14 @@ mod tests { let queue = get_test_queue(false); let block = get_good_dummy_block(); let hash = view!(BlockView, &block).header().hash().clone(); - if let Err(e) = queue.import(Unverified::new(block)) { + if let Err(e) = queue.import(new_unverified(block)) { panic!("error importing block that is valid by definition({:?})", e); } queue.flush(); queue.drain(10); queue.mark_as_good(&[ hash ]); - if let Err(e) = queue.import(Unverified::new(get_good_dummy_block())) { + if let Err(e) = queue.import(new_unverified(get_good_dummy_block())) { panic!("error importing block that has already been drained ({:?})", e); } } @@ -817,7 +822,7 @@ mod tests { #[test] fn returns_empty_once_finished() { let queue = get_test_queue(false); - queue.import(Unverified::new(get_good_dummy_block())) + queue.import(new_unverified(get_good_dummy_block())) .expect("error importing block that is valid by definition"); queue.flush(); queue.drain(1); @@ -835,7 +840,7 @@ mod tests { assert!(!queue.queue_info().is_full()); let mut blocks = get_good_dummy_block_seq(50); for b in blocks.drain(..) { - queue.import(Unverified::new(b)).unwrap(); + queue.import(new_unverified(b)).unwrap(); } assert!(queue.queue_info().is_full()); } @@ -863,7 +868,7 @@ mod tests { *queue.state.0.lock() = State::Work(0); for block in get_good_dummy_block_seq(5000) { - queue.import(Unverified::new(block)).expect("Block good by definition; qed"); + queue.import(new_unverified(block)).expect("Block good by definition; qed"); } // almost all unverified == bump verifier count.