From 9a230ca7f63ead39638fe3482e4eed72998bb4dd Mon Sep 17 00:00:00 2001 From: AntiochP <30642645+antiochp@users.noreply.github.com> Date: Fri, 13 Oct 2017 13:23:18 -0400 Subject: [PATCH] fail deserialization if inputs or outputs are not sorted for txs (#173) * fail deserialization if input or outputs are not sorted for txs, fail deserialization if inputs or outputs or kernels are not sorted for blocks, make sure we sort them before serialization * extract consensus rule into verify_sort_order on VerifySortOrder trait * rework the sort and write into WriteableSorted trait * fix wallet sending bug (fees related) so we can test * refactored read_and_verify_ordered out into ser, used in transaction and block --- chain/tests/mine_simple_chain.rs | 1 - core/src/consensus.rs | 10 +++++++-- core/src/core/block.rs | 35 ++++++++++++----------------- core/src/core/hash.rs | 16 ++++++++++++++ core/src/core/transaction.rs | 23 ++++++++++--------- core/src/ser.rs | 38 ++++++++++++++++++++++++++++++++ 6 files changed, 89 insertions(+), 34 deletions(-) diff --git a/chain/tests/mine_simple_chain.rs b/chain/tests/mine_simple_chain.rs index db97b2d9..f5e5544a 100644 --- a/chain/tests/mine_simple_chain.rs +++ b/chain/tests/mine_simple_chain.rs @@ -59,7 +59,6 @@ fn setup(dir_name: &str) -> Chain { #[test] fn mine_empty_chain() { let chain = setup(".grin"); - let keychain = Keychain::from_random_seed().unwrap(); // mine and add a few blocks diff --git a/core/src/consensus.rs b/core/src/consensus.rs index 9ac9af1c..f0fb190d 100644 --- a/core/src/consensus.rs +++ b/core/src/consensus.rs @@ -21,6 +21,7 @@ use std::fmt; +use ser; use core::target::Difficulty; /// The block subsidy amount @@ -76,8 +77,7 @@ pub const MAX_BLOCK_WEIGHT: usize = 80_000; /// Whether a block exceeds the maximum acceptable weight pub fn exceeds_weight(input_len: usize, output_len: usize, kernel_len: usize) -> bool { - input_len * BLOCK_INPUT_WEIGHT + - output_len * BLOCK_OUTPUT_WEIGHT + + input_len * BLOCK_INPUT_WEIGHT + output_len * BLOCK_OUTPUT_WEIGHT + kernel_len * BLOCK_KERNEL_WEIGHT > MAX_BLOCK_WEIGHT } @@ -206,6 +206,12 @@ where ) } +/// Consensus rule that collections of items are sorted lexicographically over the wire. +pub trait VerifySortOrder { + /// Verify a collection of items is sorted as required. + fn verify_sort_order(&self) -> Result<(), ser::Error>; +} + #[cfg(test)] use std; diff --git a/core/src/core/block.rs b/core/src/core/block.rs index 454d70dc..e7e80bc6 100644 --- a/core/src/core/block.rs +++ b/core/src/core/block.rs @@ -23,7 +23,7 @@ use core::{Input, Output, Proof, TxKernel, Transaction, COINBASE_KERNEL, COINBAS use consensus::{MINIMUM_DIFFICULTY, REWARD, reward, exceeds_weight}; use core::hash::{Hash, Hashed, ZERO_HASH}; use core::target::Difficulty; -use ser::{self, Readable, Reader, Writeable, Writer}; +use ser::{self, Readable, Reader, Writeable, Writer, WriteableSorted, read_and_verify_sorted}; use global; use keychain; @@ -189,15 +189,14 @@ impl Writeable for Block { [write_u64, self.kernels.len() as u64] ); - for inp in &self.inputs { - try!(inp.write(writer)); - } - for out in &self.outputs { - try!(out.write(writer)); - } - for proof in &self.kernels { - try!(proof.write(writer)); - } + let mut inputs = self.inputs.clone(); + let mut outputs = self.outputs.clone(); + let mut kernels = self.kernels.clone(); + + // Consensus rule that everything is sorted in lexicographical order on the wire. + try!(inputs.write_sorted(writer)); + try!(outputs.write_sorted(writer)); + try!(kernels.write_sorted(writer)); } Ok(()) } @@ -209,12 +208,12 @@ impl Readable for Block { fn read(reader: &mut Reader) -> Result { let header = try!(BlockHeader::read(reader)); - let (input_len, output_len, proof_len) = + let (input_len, output_len, kernel_len) = ser_multiread!(reader, read_u64, read_u64, read_u64); - let inputs = try!((0..input_len).map(|_| Input::read(reader)).collect()); - let outputs = try!((0..output_len).map(|_| Output::read(reader)).collect()); - let kernels = try!((0..proof_len).map(|_| TxKernel::read(reader)).collect()); + let inputs = read_and_verify_sorted(reader, input_len)?; + let outputs = read_and_verify_sorted(reader, output_len)?; + let kernels = read_and_verify_sorted(reader, kernel_len)?; Ok(Block { header: header, @@ -292,7 +291,7 @@ impl Block { // build vectors with all inputs and all outputs, ordering them by hash // needs to be a fold so we don't end up with a vector of vectors and we // want to fully own the refs (not just a pointer like flat_map). - let mut inputs = txs.iter().fold(vec![], |mut acc, ref tx| { + let inputs = txs.iter().fold(vec![], |mut acc, ref tx| { let mut inputs = tx.inputs.clone(); acc.append(&mut inputs); acc @@ -304,9 +303,6 @@ impl Block { }); outputs.push(reward_out); - inputs.sort_by_key(|inp| inp.hash()); - outputs.sort_by_key(|out| out.hash()); - // calculate the overall Merkle tree and fees Ok( @@ -401,9 +397,6 @@ impl Block { let mut all_kernels = self.kernels.clone(); all_kernels.append(&mut other.kernels.clone()); - all_inputs.sort_by_key(|inp| inp.hash()); - all_outputs.sort_by_key(|out| out.hash()); - Block { // compact will fix the merkle tree header: BlockHeader { diff --git a/core/src/core/hash.rs b/core/src/core/hash.rs index b0248709..4cf3adfd 100644 --- a/core/src/core/hash.rs +++ b/core/src/core/hash.rs @@ -23,6 +23,7 @@ use std::convert::AsRef; use blake2::blake2b::Blake2b; +use consensus::VerifySortOrder; use ser::{self, Reader, Readable, Writer, Writeable, Error, AsFixedBytes}; /// A hash consisting of all zeroes, used as a sentinel. No known preimage. @@ -191,3 +192,18 @@ impl Hashed for [u8; 0] { Hash(ret) } } + +impl VerifySortOrder for Vec { + fn verify_sort_order(&self) -> Result<(), ser::Error> { + match self + .iter() + .map(|item| item.hash()) + .collect::>() + .windows(2) + .any(|pair| pair[0] > pair[1]) + { + true => Err(ser::Error::BadlySorted), + false => Ok(()), + } + } +} diff --git a/core/src/core/transaction.rs b/core/src/core/transaction.rs index 233ea1e1..eef87e68 100644 --- a/core/src/core/transaction.rs +++ b/core/src/core/transaction.rs @@ -22,7 +22,8 @@ use std::ops; use core::Committed; use core::pmmr::Summable; use keychain::{Identifier, Keychain}; -use ser::{self, Reader, Writer, Readable, Writeable}; +use ser::{self, Reader, Writer, Readable, Writeable, WriteableSorted, read_and_verify_sorted}; + bitflags! { /// Options for a kernel's structure or use @@ -153,16 +154,19 @@ impl Writeable for Transaction { [write_u64, self.inputs.len() as u64], [write_u64, self.outputs.len() as u64] ); - for inp in &self.inputs { - try!(inp.write(writer)); - } - for out in &self.outputs { - try!(out.write(writer)); - } + + // Consensus rule that everything is sorted in lexicographical order on the wire. + let mut inputs = self.inputs.clone(); + let mut outputs = self.outputs.clone(); + + try!(inputs.write_sorted(writer)); + try!(outputs.write_sorted(writer)); + Ok(()) } } + /// Implementation of Readable for a transaction, defines how to read a full /// transaction from a binary stream. impl Readable for Transaction { @@ -170,8 +174,8 @@ impl Readable for Transaction { let (fee, lock_height, excess_sig, input_len, output_len) = ser_multiread!(reader, read_u64, read_u64, read_vec, read_u64, read_u64); - let inputs = try!((0..input_len).map(|_| Input::read(reader)).collect()); - let outputs = try!((0..output_len).map(|_| Output::read(reader)).collect()); + let inputs = read_and_verify_sorted(reader, input_len)?; + let outputs = read_and_verify_sorted(reader, output_len)?; Ok(Transaction { fee: fee, @@ -184,7 +188,6 @@ impl Readable for Transaction { } } - impl Committed for Transaction { fn inputs_committed(&self) -> &Vec { &self.inputs diff --git a/core/src/ser.rs b/core/src/ser.rs index d8e531e6..3f033c13 100644 --- a/core/src/ser.rs +++ b/core/src/ser.rs @@ -23,6 +23,8 @@ use std::{error, fmt, cmp}; use std::io::{self, Write, Read}; use byteorder::{ByteOrder, ReadBytesExt, BigEndian}; use keychain::{Identifier, IDENTIFIER_SIZE}; +use core::hash::Hashed; +use consensus::VerifySortOrder; use secp::pedersen::Commitment; use secp::pedersen::RangeProof; use secp::constants::{MAX_PROOF_SIZE, PEDERSEN_COMMITMENT_SIZE}; @@ -43,6 +45,8 @@ pub enum Error { CorruptedData, /// When asked to read too much data TooLargeReadErr, + /// Something was not sorted when consensus rules requires it to be sorted + BadlySorted, } impl From for Error { @@ -61,6 +65,7 @@ impl fmt::Display for Error { } => write!(f, "expected {:?}, got {:?}", e, r), Error::CorruptedData => f.write_str("corrupted data"), Error::TooLargeReadErr => f.write_str("too large read"), + Error::BadlySorted => f.write_str("badly sorted data"), } } } @@ -82,6 +87,7 @@ impl error::Error for Error { } => "unexpected data", Error::CorruptedData => "corrupted data", Error::TooLargeReadErr => "too large read", + Error::BadlySorted => "badly sorted data", } } } @@ -180,6 +186,25 @@ pub trait Writeable { fn write(&self, writer: &mut W) -> Result<(), Error>; } +/// Trait to allow a collection of Writeables to be written in lexicographical sort order. +pub trait WriteableSorted { + /// Write the data but sort it first. + fn write_sorted(&mut self, writer: &mut W) -> Result<(), Error>; +} + +/// Reads a collection of serialized items into a Vec +/// and verifies they are lexicographically ordered. +/// +/// A consensus rule requires everything is sorted lexicographically to avoid +/// leaking any information through specific ordering of items. +pub fn read_and_verify_sorted(reader: &mut Reader, count: u64) -> Result, Error> + where T: Readable + Hashed +{ + let result: Vec = try!((0..count).map(|_| T::read(reader)).collect()); + result.verify_sort_order()?; + Ok(result) +} + /// Trait that every type that can be deserialized from binary must implement. /// Reads directly to a Reader, a utility type thinly wrapping an /// underlying Read implementation. @@ -390,6 +415,19 @@ where } } +impl WriteableSorted for Vec +where + T: Writeable + Hashed, +{ + fn write_sorted(&mut self, writer: &mut W) -> Result<(), Error> { + self.sort_by_key(|elmt| elmt.hash()); + for elmt in self { + elmt.write(writer)?; + } + Ok(()) + } +} + impl<'a, A: Writeable> Writeable for &'a A { fn write(&self, writer: &mut W) -> Result<(), Error> { Writeable::write(*self, writer)