From 08db6fe29f8a80663e31db3cba48a84027c49b1d Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 5 Aug 2018 21:11:16 +0000 Subject: [PATCH 1/2] script: let Instructions iterator enforce minimal pushes; remove `IntoIter` impl to force users to choose --- src/blockdata/opcodes.rs | 2 +- src/blockdata/script.rs | 148 ++++++++++++++++++++++++++++++++++----- src/util/contracthash.rs | 2 +- 3 files changed, 134 insertions(+), 18 deletions(-) diff --git a/src/blockdata/opcodes.rs b/src/blockdata/opcodes.rs index 3e3f46c1..004daf94 100644 --- a/src/blockdata/opcodes.rs +++ b/src/blockdata/opcodes.rs @@ -194,7 +194,7 @@ pub enum All { OP_PUSHDATA2 = 0x4d, /// Read the next 4 bytes as N; push the next N bytes as an array onto the stack OP_PUSHDATA4 = 0x4e, - /// Push the array [0x80] onto the stack + /// Push the array [0x81] onto the stack OP_PUSHNUM_NEG1 = 0x4f, /// Synonym for OP_RETURN OP_RESERVED = 0x50, diff --git a/src/blockdata/script.rs b/src/blockdata/script.rs index 17bda089..e5836d0a 100644 --- a/src/blockdata/script.rs +++ b/src/blockdata/script.rs @@ -151,6 +151,9 @@ display_from_debug!(Builder); /// would help you. #[derive(PartialEq, Eq, Debug, Clone)] pub enum Error { + /// Something did a non-minimal push; for more information see + /// `https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki#Push_operators` + NonMinimalPush, /// Some opcode expected a parameter, but it was missing or truncated EarlyEndOfScript, /// Tried to read an array off the stack as a number when it was more than 4 bytes @@ -180,6 +183,7 @@ impl error::Error for Error { fn description(&self) -> &'static str { match *self { + Error::NonMinimalPush => "non-minimal datapush", Error::EarlyEndOfScript => "unexpected end of script", Error::NumericOverflow => "numeric overflow (number on stack larger than 4 bytes)", #[cfg(feature="bitcoinconsensus")] @@ -373,6 +377,17 @@ impl Script { opcodes::All::from(self.0[0]).classify() == opcodes::Class::IllegalOp) } + /// Iterate over the script in the form of `Instruction`s, which are an enum covering + /// opcodes, datapushes and errors. At most one error will be returned and then the + /// iterator will end. To instead iterate over the script as sequence of bytes, treat + /// it as a slice using `script[..]` or convert it to a vector using `into_bytes()`. + pub fn iter(&self, enforce_minimal: bool) -> Instructions { + Instructions { + data: &self.0[..], + enforce_minimal: enforce_minimal, + } + } + #[cfg(feature="bitcoinconsensus")] /// verify spend of an input script /// # Parameters @@ -408,13 +423,8 @@ pub enum Instruction<'a> { /// Iterator over a script returning parsed opcodes pub struct Instructions<'a> { - data: &'a [u8] -} - -impl<'a> IntoIterator for &'a Script { - type Item = Instruction<'a>; - type IntoIter = Instructions<'a>; - fn into_iter(self) -> Instructions<'a> { Instructions { data: &self.0[..] } } + data: &'a [u8], + enforce_minimal: bool, } impl<'a> Iterator for Instructions<'a> { @@ -429,41 +439,87 @@ impl<'a> Iterator for Instructions<'a> { opcodes::Class::PushBytes(n) => { let n = n as usize; if self.data.len() < n + 1 { + self.data = &[]; // Kill iterator so that it does not return an infinite stream of errors return Some(Instruction::Error(Error::EarlyEndOfScript)); } + if self.enforce_minimal { + if n == 1 && (self.data[1] == 0x81 || (self.data[1] > 0 && self.data[1] <= 16)) { + self.data = &[]; + return Some(Instruction::Error(Error::NonMinimalPush)); + } + } let ret = Some(Instruction::PushBytes(&self.data[1..n+1])); self.data = &self.data[n + 1..]; ret } opcodes::Class::Ordinary(opcodes::Ordinary::OP_PUSHDATA1) => { - if self.data.len() < 2 { return Some(Instruction::Error(Error::EarlyEndOfScript)); } + if self.data.len() < 2 { + self.data = &[]; + return Some(Instruction::Error(Error::EarlyEndOfScript)); + } let n = match read_uint(&self.data[1..], 1) { Ok(n) => n, - Err(e) => { return Some(Instruction::Error(e)); } + Err(e) => { + self.data = &[]; + return Some(Instruction::Error(e)); + } }; - if self.data.len() < n + 2 { return Some(Instruction::Error(Error::EarlyEndOfScript)); } + if self.data.len() < n + 2 { + self.data = &[]; + return Some(Instruction::Error(Error::EarlyEndOfScript)); + } + if self.enforce_minimal && n < 76 { + self.data = &[]; + return Some(Instruction::Error(Error::NonMinimalPush)); + } let ret = Some(Instruction::PushBytes(&self.data[2..n+2])); self.data = &self.data[n + 2..]; ret } opcodes::Class::Ordinary(opcodes::Ordinary::OP_PUSHDATA2) => { - if self.data.len() < 3 { return Some(Instruction::Error(Error::EarlyEndOfScript)); } + if self.data.len() < 3 { + self.data = &[]; + return Some(Instruction::Error(Error::EarlyEndOfScript)); + } let n = match read_uint(&self.data[1..], 2) { Ok(n) => n, - Err(e) => { return Some(Instruction::Error(e)); } + Err(e) => { + self.data = &[]; + return Some(Instruction::Error(e)); + } }; - if self.data.len() < n + 3 { return Some(Instruction::Error(Error::EarlyEndOfScript)); } + if self.enforce_minimal && n < 0x100 { + self.data = &[]; + return Some(Instruction::Error(Error::NonMinimalPush)); + } + if self.data.len() < n + 3 { + self.data = &[]; + return Some(Instruction::Error(Error::EarlyEndOfScript)); + } let ret = Some(Instruction::PushBytes(&self.data[3..n + 3])); self.data = &self.data[n + 3..]; ret } opcodes::Class::Ordinary(opcodes::Ordinary::OP_PUSHDATA4) => { - if self.data.len() < 5 { return Some(Instruction::Error(Error::EarlyEndOfScript)); } + if self.data.len() < 5 { + self.data = &[]; + return Some(Instruction::Error(Error::EarlyEndOfScript)); + } let n = match read_uint(&self.data[1..], 4) { Ok(n) => n, - Err(e) => { return Some(Instruction::Error(e)); } + Err(e) => { + self.data = &[]; + return Some(Instruction::Error(e)); + } }; - if self.data.len() < n + 5 { return Some(Instruction::Error(Error::EarlyEndOfScript)); } + if self.enforce_minimal && n < 0x10000 { + self.data = &[]; + return Some(Instruction::Error(Error::NonMinimalPush)); + } + if self.data.len() < n + 5 { + self.data = &[]; + return Some(Instruction::Error(Error::EarlyEndOfScript)); + } let ret = Some(Instruction::PushBytes(&self.data[5..n + 5])); self.data = &self.data[n + 5..]; ret @@ -787,6 +843,66 @@ mod test { assert_eq!(redeem_script.to_v0_p2wsh().to_p2sh(), expected_out); } + #[test] + fn test_iterator() { + let zero = hex_script!("00"); + let zeropush = hex_script!("0100"); + + let nonminimal = hex_script!("4c0169b2"); // PUSHDATA1 for no reason + let minimal = hex_script!("0169b2"); // minimal + let nonminimal_alt = hex_script!("026900b2"); // non-minimal number but minimal push (should be OK) + + let v_zero: Vec = zero.iter(true).collect(); + let v_zeropush: Vec = zeropush.iter(true).collect(); + + let v_min: Vec = minimal.iter(true).collect(); + let v_nonmin: Vec = nonminimal.iter(true).collect(); + let v_nonmin_alt: Vec = nonminimal_alt.iter(true).collect(); + let slop_v_min: Vec = minimal.iter(false).collect(); + let slop_v_nonmin: Vec = nonminimal.iter(false).collect(); + let slop_v_nonmin_alt: Vec = nonminimal_alt.iter(false).collect(); + + assert_eq!( + v_zero, + vec![ + Instruction::PushBytes(&[]), + ] + ); + assert_eq!( + v_zeropush, + vec![ + Instruction::PushBytes(&[0]), + ] + ); + + assert_eq!( + v_min, + vec![ + Instruction::PushBytes(&[105]), + Instruction::Op(opcodes::All::OP_NOP3), + ] + ); + + assert_eq!( + v_nonmin, + vec![ + Instruction::Error(Error::NonMinimalPush), + ] + ); + + assert_eq!( + v_nonmin_alt, + vec![ + Instruction::PushBytes(&[105, 0]), + Instruction::Op(opcodes::All::OP_NOP3), + ] + ); + + assert_eq!(v_min, slop_v_min); + assert_eq!(v_min, slop_v_nonmin); + assert_eq!(v_nonmin_alt, slop_v_nonmin_alt); + } + #[test] #[cfg(feature="bitcoinconsensus")] fn test_bitcoinconsensus () { diff --git a/src/util/contracthash.rs b/src/util/contracthash.rs index b405eee5..88b3c40d 100644 --- a/src/util/contracthash.rs +++ b/src/util/contracthash.rs @@ -237,7 +237,7 @@ pub fn untemplate(script: &script::Script) -> Result<(Template, Vec), } let mut mode = Mode::SeekingKeys; - for instruction in script.into_iter() { + for instruction in script.iter(false) { match instruction { script::Instruction::PushBytes(data) => { let n = data.len(); From fc0fec7e1912b43c760850552187806eee9c8bae Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 20 Aug 2018 18:30:05 +0000 Subject: [PATCH 2/2] fuzz: add Script::iter tests to script deserialization test --- fuzz/fuzz_targets/deserialize_script.rs | 32 ++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/fuzz/fuzz_targets/deserialize_script.rs b/fuzz/fuzz_targets/deserialize_script.rs index a31a74c6..fcf23d6d 100644 --- a/fuzz/fuzz_targets/deserialize_script.rs +++ b/fuzz/fuzz_targets/deserialize_script.rs @@ -1,7 +1,37 @@ extern crate bitcoin; +use bitcoin::blockdata::script; +use bitcoin::network::serialize; + fn do_test(data: &[u8]) { - let _: Result = bitcoin::network::serialize::deserialize(data); + let s: Result = serialize::deserialize(data); + if let Ok(script) = s { + let _: Vec = script.iter(false).collect(); + let enforce_min: Vec = script.iter(true).collect(); + + let mut b = script::Builder::new(); + for ins in enforce_min { + match ins { + script::Instruction::Error(_) => return, + script::Instruction::Op(op) => { b = b.push_opcode(op); } + script::Instruction::PushBytes(bytes) => { + // Any one-byte pushes, except -0, which can be interpreted as numbers, should be + // reserialized as numbers. (For -1 through 16, this will use special ops; for + // others it'll just reserialize them as pushes.) + if bytes.len() == 1 && bytes[0] != 0x80 && bytes[0] != 0x00 { + if let Ok(num) = script::read_scriptint(bytes) { + b = b.push_int(num); + } else { + b = b.push_slice(bytes); + } + } else { + b = b.push_slice(bytes); + } + } + } + } + assert_eq!(b.into_script(), script); + } } #[cfg(feature = "afl")]