From 146bb48849e5393004be5c88beefe76fdf009aba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89lo=C3=AFs?= Date: Wed, 13 Oct 2021 14:48:35 +0200 Subject: [PATCH] Fix security issue: transaction validity controls must be executed in STF (#495) * add security tests * add pre_dispatch phase to fiy security issues * Revert frame-evm changes * keeping *_self_contained aligned * remove unused default impl --- frame/ethereum/src/lib.rs | 211 ++++++++++++------ frame/ethereum/src/mock.rs | 61 ++++- frame/ethereum/src/tests.rs | 66 +++++- .../self-contained/src/checked_extrinsic.rs | 6 + primitives/self-contained/src/lib.rs | 14 ++ template/runtime/src/lib.rs | 10 + 6 files changed, 287 insertions(+), 81 deletions(-) diff --git a/frame/ethereum/src/lib.rs b/frame/ethereum/src/lib.rs index 5c96f9a2af..82f4727867 100644 --- a/frame/ethereum/src/lib.rs +++ b/frame/ethereum/src/lib.rs @@ -118,78 +118,26 @@ where } } - pub fn validate_self_contained(&self, origin: &H160) -> Option { + pub fn pre_dispatch_self_contained( + &self, + origin: &H160, + ) -> Option> { if let Call::transact(transaction) = self { - let validate = || { - // We must ensure a transaction can pay the cost of its data bytes. - // If it can't it should not be included in a block. - let mut gasometer = evm::gasometer::Gasometer::new( - transaction.gas_limit.low_u64(), - ::config(), - ); - let transaction_cost = match transaction.action { - TransactionAction::Call(_) => { - evm::gasometer::call_transaction_cost(&transaction.input) - } - TransactionAction::Create => { - evm::gasometer::create_transaction_cost(&transaction.input) - } - }; - if gasometer.record_transaction(transaction_cost).is_err() { - return InvalidTransaction::Custom( - TransactionValidationError::InvalidGasLimit as u8, - ) - .into(); - } - - if let Some(chain_id) = transaction.signature.chain_id() { - if chain_id != T::ChainId::get() { - return InvalidTransaction::Custom( - TransactionValidationError::InvalidChainId as u8, - ) - .into(); - } - } - - if transaction.gas_limit >= T::BlockGasLimit::get() { - return InvalidTransaction::Custom( - TransactionValidationError::InvalidGasLimit as u8, - ) - .into(); - } - - let account_data = pallet_evm::Pallet::::account_basic(&origin); - - if transaction.nonce < account_data.nonce { - return InvalidTransaction::Stale.into(); - } - - let fee = transaction.gas_price.saturating_mul(transaction.gas_limit); - let total_payment = transaction.value.saturating_add(fee); - if account_data.balance < total_payment { - return InvalidTransaction::Payment.into(); - } - - let min_gas_price = T::FeeCalculator::min_gas_price(); - - if transaction.gas_price < min_gas_price { - return InvalidTransaction::Payment.into(); - } - - let mut builder = ValidTransactionBuilder::default() - .and_provides((origin, transaction.nonce)) - .priority(transaction.gas_price.unique_saturated_into()); - - if transaction.nonce > account_data.nonce { - if let Some(prev_nonce) = transaction.nonce.checked_sub(1.into()) { - builder = builder.and_requires((origin, prev_nonce)) - } - } - - builder.build() - }; + Some(Pallet::::validate_transaction_in_block( + *origin, + &transaction, + )) + } else { + None + } + } - Some(validate()) + pub fn validate_self_contained(&self, origin: &H160) -> Option { + if let Call::transact(transaction) = self { + Some(Pallet::::validate_transaction_in_pool( + *origin, + transaction, + )) } else { None } @@ -259,9 +207,12 @@ pub mod pallet { "pre-block transaction signature invalid; the block cannot be built", ); - Self::do_transact(source, transaction).expect( + Self::validate_transaction_in_block(source, &transaction).expect( "pre-block transaction verification failed; the block cannot be built", ); + Self::apply_validated_transaction(source, transaction).expect( + "pre-block transaction execution failed; the block cannot be built", + ); } } @@ -287,7 +238,7 @@ pub mod pallet { Error::::PreLogExists, ); - Self::do_transact(source, transaction) + Self::apply_validated_transaction(source, transaction) } } @@ -418,7 +369,98 @@ impl Pallet { } } - fn do_transact(source: H160, transaction: Transaction) -> DispatchResultWithPostInfo { + // Common controls to be performed in the same way by the pool and the + // State Transition Function (STF). + // This is the case for all controls except those concerning the nonce. + fn validate_transaction_common( + origin: H160, + transaction: &Transaction, + ) -> Result { + // We must ensure a transaction can pay the cost of its data bytes. + // If it can't it should not be included in a block. + let mut gasometer = evm::gasometer::Gasometer::new( + transaction.gas_limit.low_u64(), + ::config(), + ); + let transaction_cost = match transaction.action { + TransactionAction::Call(_) => evm::gasometer::call_transaction_cost(&transaction.input), + TransactionAction::Create => { + evm::gasometer::create_transaction_cost(&transaction.input) + } + }; + if gasometer.record_transaction(transaction_cost).is_err() { + return Err(InvalidTransaction::Custom( + TransactionValidationError::InvalidGasLimit as u8, + ) + .into()); + } + + if let Some(chain_id) = transaction.signature.chain_id() { + if chain_id != T::ChainId::get() { + return Err(InvalidTransaction::Custom( + TransactionValidationError::InvalidChainId as u8, + ) + .into()); + } + } + + if transaction.gas_limit >= T::BlockGasLimit::get() { + return Err(InvalidTransaction::Custom( + TransactionValidationError::InvalidGasLimit as u8, + ) + .into()); + } + + let account_data = pallet_evm::Pallet::::account_basic(&origin); + + let fee = transaction.gas_price.saturating_mul(transaction.gas_limit); + let total_payment = transaction.value.saturating_add(fee); + if account_data.balance < total_payment { + return Err(InvalidTransaction::Payment.into()); + } + + let min_gas_price = T::FeeCalculator::min_gas_price(); + + if transaction.gas_price < min_gas_price { + return Err(InvalidTransaction::Payment.into()); + } + + Ok(account_data.nonce) + } + + // Controls that must be performed by the pool. + // The controls common with the State Transition Function (STF) are in + // the function `validate_transaction_common`. + fn validate_transaction_in_pool( + origin: H160, + transaction: &Transaction, + ) -> TransactionValidity { + let account_nonce = Self::validate_transaction_common(origin, transaction)?; + + if transaction.nonce < account_nonce { + return Err(InvalidTransaction::Stale.into()); + } + + // The tag provides and requires must be filled correctly according to the nonce. + let mut builder = ValidTransactionBuilder::default() + .and_provides((origin, transaction.nonce)) + .priority(transaction.gas_price.unique_saturated_into()); + + // In the context of the pool, a transaction with + // too high a nonce is still considered valid + if transaction.nonce > account_nonce { + if let Some(prev_nonce) = transaction.nonce.checked_sub(1.into()) { + builder = builder.and_requires((origin, prev_nonce)) + } + } + + builder.build() + } + + fn apply_validated_transaction( + source: H160, + transaction: Transaction, + ) -> DispatchResultWithPostInfo { let transaction_hash = H256::from_slice(Keccak256::digest(&rlp::encode(&transaction)).as_slice()); let transaction_index = Pending::::get().len() as u32; @@ -565,6 +607,29 @@ impl Pallet { } } } + + /// Validate an Ethereum transaction already in block + /// + /// This function must be called during the pre-dispatch phase + /// (just before applying the extrinsic). + pub fn validate_transaction_in_block( + origin: H160, + transaction: ðereum::TransactionV0, + ) -> Result<(), TransactionValidityError> { + let account_nonce = Self::validate_transaction_common(origin, transaction)?; + + // In the context of the block, a transaction with a nonce that is + // too high should be considered invalid and make the whole block invalid. + if transaction.nonce > account_nonce { + Err(TransactionValidityError::Invalid( + InvalidTransaction::Future, + )) + } else if transaction.nonce < account_nonce { + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)) + } else { + Ok(()) + } + } } #[derive(Eq, PartialEq, Clone, sp_runtime::RuntimeDebug)] diff --git a/frame/ethereum/src/mock.rs b/frame/ethereum/src/mock.rs index 5956762dff..5a8a385722 100644 --- a/frame/ethereum/src/mock.rs +++ b/frame/ethereum/src/mock.rs @@ -19,6 +19,7 @@ use super::*; use crate::IntermediateStateRoot; +use codec::{WrapperTypeDecode, WrapperTypeEncode}; use ethereum::{TransactionAction, TransactionSignature}; use frame_support::{parameter_types, traits::FindAuthor, ConsensusEngineId, PalletId}; use pallet_evm::{AddressMapping, EnsureAddressTruncated, FeeCalculator}; @@ -27,11 +28,13 @@ use sha3::Digest; use sp_core::{H160, H256, U256}; use sp_runtime::{ testing::Header, - traits::{BlakeTwo256, IdentityLookup}, + traits::{BlakeTwo256, IdentityLookup, SignedExtension}, AccountId32, }; -type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; +pub type SignedExtra = (frame_system::CheckSpecVersion,); + +type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; frame_support::construct_runtime! { @@ -166,6 +169,54 @@ impl crate::Config for Test { type StateRoot = IntermediateStateRoot; } +impl fp_self_contained::SelfContainedCall for Call { + type SignedInfo = H160; + + fn is_self_contained(&self) -> bool { + match self { + Call::Ethereum(call) => call.is_self_contained(), + _ => false, + } + } + + fn check_self_contained(&self) -> Option> { + match self { + Call::Ethereum(call) => call.check_self_contained(), + _ => None, + } + } + + fn validate_self_contained(&self, info: &Self::SignedInfo) -> Option { + match self { + Call::Ethereum(call) => call.validate_self_contained(info), + _ => None, + } + } + + fn pre_dispatch_self_contained( + &self, + info: &Self::SignedInfo, + ) -> Option> { + match self { + Call::Ethereum(call) => call.pre_dispatch_self_contained(info), + _ => None, + } + } + + fn apply_self_contained( + self, + info: Self::SignedInfo, + ) -> Option>> { + use sp_runtime::traits::Dispatchable as _; + match self { + call @ Call::Ethereum(crate::Call::transact(_)) => { + Some(call.dispatch(Origin::from(crate::RawOrigin::EthereumTransaction(info)))) + } + _ => None, + } + } +} + pub struct AccountInfo { pub address: H160, pub account_id: AccountId32, @@ -255,6 +306,10 @@ impl UnsignedTransaction { } pub fn sign(&self, key: &H256) -> Transaction { + self.sign_with_chain_id(key, ChainId::get()) + } + + pub fn sign_with_chain_id(&self, key: &H256, chain_id: u64) -> Transaction { let hash = self.signing_hash(); let msg = libsecp256k1::Message::parse(hash.as_fixed_bytes()); let s = libsecp256k1::sign( @@ -264,7 +319,7 @@ impl UnsignedTransaction { let sig = s.0.serialize(); let sig = TransactionSignature::new( - s.1.serialize() as u64 % 2 + ChainId::get() * 2 + 35, + s.1.serialize() as u64 % 2 + chain_id * 2 + 35, H256::from_slice(&sig[0..32]), H256::from_slice(&sig[32..64]), ) diff --git a/frame/ethereum/src/tests.rs b/frame/ethereum/src/tests.rs index b1aafcdcda..b793c7edb3 100644 --- a/frame/ethereum/src/tests.rs +++ b/frame/ethereum/src/tests.rs @@ -18,13 +18,18 @@ //! Consensus extension module tests for BABE consensus. use crate::{ - mock::*, CallOrCreateInfo, Error, RawOrigin, Transaction, TransactionAction, - ValidTransactionBuilder, H160, H256, U256, + mock::*, CallOrCreateInfo, Error, RawOrigin, Transaction, TransactionAction, H160, H256, U256, }; use ethereum::TransactionSignature; -use frame_support::{assert_err, assert_noop, assert_ok, unsigned::ValidateUnsigned}; +use frame_support::{ + assert_err, assert_noop, assert_ok, + unsigned::{TransactionValidityError, ValidateUnsigned}, +}; use rustc_hex::{FromHex, ToHex}; -use sp_runtime::transaction_validity::{InvalidTransaction, TransactionSource}; +use sp_runtime::traits::Applyable; +use sp_runtime::transaction_validity::{ + InvalidTransaction, TransactionSource, ValidTransactionBuilder, +}; use std::str::FromStr; // This ERC-20 contract mints the maximum amount of tokens to the contract creator. @@ -91,7 +96,7 @@ fn transaction_without_enough_gas_should_not_work() { } #[test] -fn transaction_with_invalid_nonce_should_not_work() { +fn transaction_with_to_low_nonce_should_not_work() { let (pairs, mut ext) = new_test_ext(1); let alice = &pairs[0]; @@ -140,6 +145,57 @@ fn transaction_with_invalid_nonce_should_not_work() { }); } +#[test] +fn transaction_with_to_hight_nonce_should_fail_in_block() { + let (pairs, mut ext) = new_test_ext(1); + let alice = &pairs[0]; + + ext.execute_with(|| { + let mut transaction = default_erc20_creation_unsigned_transaction(); + transaction.nonce = U256::one(); + + let signed = transaction.sign(&alice.private_key); + let call = crate::Call::::transact(signed); + let source = call.check_self_contained().unwrap().unwrap(); + let extrinsic = fp_self_contained::CheckedExtrinsic::<_, _, SignedExtra, _> { + signed: fp_self_contained::CheckedSignature::SelfContained(source), + function: Call::Ethereum(call), + }; + use frame_support::weights::GetDispatchInfo as _; + let dispatch_info = extrinsic.get_dispatch_info(); + assert_err!( + extrinsic.apply::(&dispatch_info, 0), + TransactionValidityError::Invalid(InvalidTransaction::Future) + ); + }); +} + +#[test] +fn transaction_with_invalid_chain_id_should_fail_in_block() { + let (pairs, mut ext) = new_test_ext(1); + let alice = &pairs[0]; + + ext.execute_with(|| { + let transaction = + default_erc20_creation_unsigned_transaction().sign_with_chain_id(&alice.private_key, 1); + + let call = crate::Call::::transact(transaction); + let source = call.check_self_contained().unwrap().unwrap(); + let extrinsic = fp_self_contained::CheckedExtrinsic::<_, _, SignedExtra, _> { + signed: fp_self_contained::CheckedSignature::SelfContained(source), + function: Call::Ethereum(call), + }; + use frame_support::weights::GetDispatchInfo as _; + let dispatch_info = extrinsic.get_dispatch_info(); + assert_err!( + extrinsic.apply::(&dispatch_info, 0), + TransactionValidityError::Invalid(InvalidTransaction::Custom( + crate::TransactionValidationError::InvalidChainId as u8, + )) + ); + }); +} + #[test] fn contract_constructor_should_get_executed() { let (pairs, mut ext) = new_test_ext(1); diff --git a/primitives/self-contained/src/checked_extrinsic.rs b/primitives/self-contained/src/checked_extrinsic.rs index e0ecebff67..41a66c60aa 100644 --- a/primitives/self-contained/src/checked_extrinsic.rs +++ b/primitives/self-contained/src/checked_extrinsic.rs @@ -135,6 +135,12 @@ where Ok(res) } CheckedSignature::SelfContained(signed_info) => { + // If pre-dispatch fail, the block must be considered invalid + self.function + .pre_dispatch_self_contained(&signed_info) + .ok_or(TransactionValidityError::Invalid( + InvalidTransaction::BadProof, + ))??; Ok(self.function.apply_self_contained(signed_info).ok_or( TransactionValidityError::Invalid(InvalidTransaction::BadProof), )?) diff --git a/primitives/self-contained/src/lib.rs b/primitives/self-contained/src/lib.rs index 82aebb7a9c..62cca45818 100644 --- a/primitives/self-contained/src/lib.rs +++ b/primitives/self-contained/src/lib.rs @@ -44,6 +44,20 @@ pub trait SelfContainedCall: Dispatchable { /// Validate a self-contained function. Returns `None` if the /// function is not a self-contained. fn validate_self_contained(&self, info: &Self::SignedInfo) -> Option; + /// Do any pre-flight stuff for a self-contained call. + /// + /// Note this function by default delegates to `validate_self_contained`, so that + /// all checks performed for the transaction queue are also performed during + /// the dispatch phase (applying the extrinsic). + /// + /// If you ever override this function, you need to make sure to always + /// perform the same validation as in `validate_self_contained`. + /// + /// Returns `None` if the function is not a self-contained. + fn pre_dispatch_self_contained( + &self, + info: &Self::SignedInfo, + ) -> Option>; /// Apply a self-contained function. Returns `None` if the /// function is not a self-contained. fn apply_self_contained( diff --git a/template/runtime/src/lib.rs b/template/runtime/src/lib.rs index 2d8d9b03ef..209a35c20a 100644 --- a/template/runtime/src/lib.rs +++ b/template/runtime/src/lib.rs @@ -440,6 +440,16 @@ impl fp_self_contained::SelfContainedCall for Call { } } + fn pre_dispatch_self_contained( + &self, + info: &Self::SignedInfo, + ) -> Option> { + match self { + Call::Ethereum(call) => call.pre_dispatch_self_contained(info), + _ => None, + } + } + fn apply_self_contained( self, info: Self::SignedInfo,