From 106b5b93f5775fd4927255f4940d5e84772cd1a9 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Fri, 13 Dec 2019 18:12:22 +0100 Subject: [PATCH] remove null signatures (#11335) * tx: clean up legacy eip-86 based null signature * tx: add a test for null signature rejection * tx: revert json txn changes * fix evmbin bench build * tx: put UNSIGNED_SENDER behind 'test-helpers' feature * Revert "tx: put UNSIGNED_SENDER behind 'test-helpers' feature" This reverts commit 1dde47831e4dc5098d064e6022ead43512c31efb. * tx: add comment for null_sign * even more cleanup * Revert "even more cleanup" This reverts commit be29f032415c53d4d40842f93f1fd1fb6f9cc6bd. --- ethcore/src/client/evm_test_client.rs | 2 +- ethcore/src/machine.rs | 2 +- ethcore/types/src/transaction/transaction.rs | 62 ++++++++++++-------- evmbin/Cargo.toml | 2 +- json/src/spec/builtin.rs | 22 +++++++ 5 files changed, 62 insertions(+), 28 deletions(-) diff --git a/ethcore/src/client/evm_test_client.rs b/ethcore/src/client/evm_test_client.rs index ad08ecef550..1034472d0f3 100644 --- a/ethcore/src/client/evm_test_client.rs +++ b/ethcore/src/client/evm_test_client.rs @@ -245,7 +245,7 @@ impl<'a> EvmTestClient<'a> { ) -> std::result::Result, TransactErr> { let initial_gas = transaction.gas; // Verify transaction - let is_ok = transaction.verify_basic(true, None, false); + let is_ok = transaction.verify_basic(true, None); if let Err(error) = is_ok { return Err( TransactErr{ diff --git a/ethcore/src/machine.rs b/ethcore/src/machine.rs index abac7058ad8..66463171382 100644 --- a/ethcore/src/machine.rs +++ b/ethcore/src/machine.rs @@ -367,7 +367,7 @@ impl Machine { } else { None }; - t.verify_basic(check_low_s, chain_id, false)?; + t.verify_basic(check_low_s, chain_id)?; Ok(()) } diff --git a/ethcore/types/src/transaction/transaction.rs b/ethcore/types/src/transaction/transaction.rs index 08fcc3ea914..3dbe47692f7 100644 --- a/ethcore/types/src/transaction/transaction.rs +++ b/ethcore/types/src/transaction/transaction.rs @@ -138,6 +138,7 @@ impl Transaction { } } +#[cfg(any(test, feature = "test-helpers"))] impl From for SignedTransaction { fn from(t: ethjson::transaction::Transaction) -> Self { let to: Option = t.to.into(); @@ -237,7 +238,10 @@ impl Transaction { } } - /// Add EIP-86 compatible empty signature. + /// Legacy EIP-86 compatible empty signature. + /// This method is used in json tests as well as + /// signature verification tests. + #[cfg(any(test, feature = "test-helpers"))] pub fn null_sign(self, chain_id: u64) -> SignedTransaction { SignedTransaction { transaction: UnverifiedTransaction { @@ -295,7 +299,7 @@ impl rlp::Decodable for UnverifiedTransaction { v: d.val_at(6)?, r: d.val_at(7)?, s: d.val_at(8)?, - hash: hash, + hash, }) } } @@ -312,7 +316,7 @@ impl UnverifiedTransaction { self } - /// Checks is signature is empty. + /// Checks if the signature is empty. pub fn is_unsigned(&self) -> bool { self.r.is_zero() && self.s.is_zero() } @@ -386,17 +390,12 @@ impl UnverifiedTransaction { } /// Verify basic signature params. Does not attempt sender recovery. - pub fn verify_basic(&self, check_low_s: bool, chain_id: Option, allow_empty_signature: bool) -> Result<(), error::Error> { - if check_low_s && !(allow_empty_signature && self.is_unsigned()) { - self.check_low_s()?; - } - // Disallow unsigned transactions in case EIP-86 is disabled. - if !allow_empty_signature && self.is_unsigned() { + pub fn verify_basic(&self, check_low_s: bool, chain_id: Option) -> Result<(), error::Error> { + if self.is_unsigned() { return Err(ethkey::Error::InvalidSignature.into()); } - // EIP-86: Transactions of this form MUST have gasprice = 0, nonce = 0, value = 0, and do NOT increment the nonce of account 0. - if allow_empty_signature && self.is_unsigned() && !(self.gas_price.is_zero() && self.value.is_zero() && self.nonce.is_zero()) { - return Err(ethkey::Error::InvalidSignature.into()) + if check_low_s { + self.check_low_s()?; } match (self.chain_id(), chain_id) { (None, _) => {}, @@ -436,20 +435,15 @@ impl SignedTransaction { /// Try to verify transaction and recover sender. pub fn new(transaction: UnverifiedTransaction) -> Result { if transaction.is_unsigned() { - Ok(SignedTransaction { - transaction: transaction, - sender: UNSIGNED_SENDER, - public: None, - }) - } else { - let public = transaction.recover_public()?; - let sender = public_to_address(&public); - Ok(SignedTransaction { - transaction: transaction, - sender: sender, - public: Some(public), - }) + return Err(ethkey::Error::InvalidSignature); } + let public = transaction.recover_public()?; + let sender = public_to_address(&public); + Ok(SignedTransaction { + transaction, + sender, + public: Some(public), + }) } /// Returns transaction sender. @@ -640,6 +634,24 @@ mod tests { assert_eq!(t.chain_id(), None); } + #[test] + fn should_reject_null_signature() { + let t = Transaction { + nonce: U256::zero(), + gas_price: U256::from(10000000000u64), + gas: U256::from(21000), + action: Action::Call(Address::from_str("d46e8dd67c5d32be8058bb8eb970870f07244567").unwrap()), + value: U256::from(1), + data: vec![] + }.null_sign(1); + + let res = SignedTransaction::new(t.transaction); + match res { + Err(parity_crypto::publickey::Error::InvalidSignature) => {} + _ => panic!("null signature should be rejected"), + } + } + #[test] fn should_recover_from_chain_specific_signing() { use ethkey::{Random, Generator}; diff --git a/evmbin/Cargo.toml b/evmbin/Cargo.toml index 6ef9c94597f..c9ca633391f 100644 --- a/evmbin/Cargo.toml +++ b/evmbin/Cargo.toml @@ -9,7 +9,7 @@ name = "parity-evm" path = "./src/main.rs" [dependencies] -common-types = { path = "../ethcore/types" } +common-types = { path = "../ethcore/types", features = ["test-helpers"] } docopt = "1.0" env_logger = "0.5" ethcore = { path = "../ethcore", features = ["test-helpers", "json-tests", "to-pod-full"] } diff --git a/json/src/spec/builtin.rs b/json/src/spec/builtin.rs index 471b738573f..ca734d2ffe5 100644 --- a/json/src/spec/builtin.rs +++ b/json/src/spec/builtin.rs @@ -203,6 +203,28 @@ mod tests { ]); } + #[test] + fn deserialization_alt_bn128_const_operations() { + let s = r#"{ + "name": "alt_bn128_mul", + "pricing": { + "100500": { + "price": { "alt_bn128_const_operations": { "price": 123 }} + } + } + }"#; + let builtin: Builtin = serde_json::from_str::(s).unwrap().into(); + assert_eq!(builtin.name, "alt_bn128_mul"); + assert_eq!(builtin.pricing, map![ + 100500 => PricingAt { + info: None, + price: Pricing::AltBn128ConstOperations(AltBn128ConstOperations { + price: 123, + }), + } + ]); + } + #[test] fn activate_at() { let s = r#"{