From 438a75c577588eb36fa9cbfbdbe24fb6281681b5 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 19 May 2023 16:45:05 +0200 Subject: [PATCH 1/3] Adds replay protection checks in prepare_proposal --- apps/src/lib/node/ledger/shell/mod.rs | 58 ++++++++++++++++- .../lib/node/ledger/shell/prepare_proposal.rs | 17 +++-- .../lib/node/ledger/shell/process_proposal.rs | 64 ++++--------------- 3 files changed, 80 insertions(+), 59 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs index 8c617e396b..a689e218e3 100644 --- a/apps/src/lib/node/ledger/shell/mod.rs +++ b/apps/src/lib/node/ledger/shell/mod.rs @@ -22,6 +22,7 @@ use std::path::{Path, PathBuf}; use std::rc::Rc; use borsh::{BorshDeserialize, BorshSerialize}; +use namada::core::types::hash::Hash; use namada::ledger::events::log::EventLog; use namada::ledger::events::Event; use namada::ledger::gas::BlockGasMeter; @@ -30,9 +31,9 @@ use namada::ledger::pos::namada_proof_of_stake::types::{ }; use namada::ledger::storage::write_log::WriteLog; use namada::ledger::storage::{ - DBIter, Sha256Hasher, Storage, StorageHasher, WlStorage, DB, + DBIter, Sha256Hasher, Storage, StorageHasher, TempWlStorage, WlStorage, DB, }; -use namada::ledger::storage_api::{self, StorageRead}; +use namada::ledger::storage_api::{self, StorageRead, StorageWrite}; use namada::ledger::{ibc, pos, protocol, replay_protection}; use namada::proof_of_stake::{self, read_pos_params, slash}; use namada::proto::{self, Tx}; @@ -47,7 +48,7 @@ use namada::types::token::{self}; use namada::types::transaction::MIN_FEE; use namada::types::transaction::{ hash_tx, process_tx, verify_decrypted_correctly, AffineCurve, DecryptedTx, - EllipticCurve, PairingEngine, TxType, + EllipticCurve, PairingEngine, TxType, WrapperTx, }; use namada::types::{address, hash}; use namada::vm::wasm::{TxCache, VpCache}; @@ -112,6 +113,8 @@ pub enum Error { LoadingWasm(String), #[error("Error reading from or writing to storage: {0}")] StorageApi(#[from] storage_api::Error), + #[error("Transaction replay attempt: {0}")] + ReplayAttempt(String), } impl From for TxResult { @@ -646,6 +649,55 @@ where response } + /// Checks that neither the wrapper nor the inner transaction have already + /// been applied. Requires a [`TempWlStorage`] to perform the check during + /// block construction and validation + pub fn replay_protection_checks( + &self, + wrapper: &WrapperTx, + tx_bytes: &[u8], + temp_wl_storage: &mut TempWlStorage, + ) -> Result<()> { + let inner_hash_key = + replay_protection::get_tx_hash_key(&wrapper.tx_hash); + if temp_wl_storage + .has_key(&inner_hash_key) + .expect("Error while checking inner tx hash key in storage") + { + return Err(Error::ReplayAttempt(format!( + "Inner transaction hash {} already in storage", + &wrapper.tx_hash + ))); + } + + // Write inner hash to WAL + temp_wl_storage + .write(&inner_hash_key, ()) + .expect("Couldn't write inner transaction hash to write log"); + + let tx = + Tx::try_from(tx_bytes).expect("Deserialization shouldn't fail"); + let wrapper_hash = Hash(tx.unsigned_hash()); + let wrapper_hash_key = + replay_protection::get_tx_hash_key(&wrapper_hash); + if temp_wl_storage + .has_key(&wrapper_hash_key) + .expect("Error while checking wrapper tx hash key in storage") + { + return Err(Error::ReplayAttempt(format!( + "Wrapper transaction hash {} already in storage", + wrapper_hash + ))); + } + + // Write wrapper hash to WAL + temp_wl_storage + .write(&wrapper_hash_key, ()) + .expect("Couldn't write wrapper tx hash to write log"); + + Ok(()) + } + /// Validate a transaction request. On success, the transaction will /// included in the mempool and propagated to peers, otherwise it will be /// rejected. diff --git a/apps/src/lib/node/ledger/shell/prepare_proposal.rs b/apps/src/lib/node/ledger/shell/prepare_proposal.rs index d48d5cfcec..9b19c9717a 100644 --- a/apps/src/lib/node/ledger/shell/prepare_proposal.rs +++ b/apps/src/lib/node/ledger/shell/prepare_proposal.rs @@ -1,7 +1,7 @@ //! Implementation of the [`RequestPrepareProposal`] ABCI++ method for the Shell use namada::core::hints; -use namada::ledger::storage::{DBIter, StorageHasher, DB}; +use namada::ledger::storage::{DBIter, StorageHasher, TempWlStorage, DB}; use namada::proof_of_stake::pos_queries::PosQueries; use namada::proto::Tx; use namada::types::internal::WrapperTxInQueue; @@ -47,8 +47,12 @@ where let alloc = self.get_encrypted_txs_allocator(); // add encrypted txs - let (encrypted_txs, alloc) = - self.build_encrypted_txs(alloc, &req.txs, &req.time); + let (encrypted_txs, alloc) = self.build_encrypted_txs( + alloc, + TempWlStorage::new(&self.wl_storage.storage), + &req.txs, + &req.time, + ); let mut txs = encrypted_txs; // decrypt the wrapper txs included in the previous block @@ -119,6 +123,7 @@ where fn build_encrypted_txs( &self, mut alloc: EncryptedTxBatchAllocator, + mut temp_wl_storage: TempWlStorage, txs: &[TxBytes], block_time: &Option, ) -> (Vec, BlockSpaceAllocator) { @@ -138,8 +143,10 @@ where if let (Some(block_time), Some(exp)) = (block_time.as_ref(), &tx.expiration) { if block_time > exp { return None } } - if let Ok(TxType::Wrapper(_)) = process_tx(tx) { - return Some(tx_bytes.clone()); + if let Ok(TxType::Wrapper(ref wrapper)) = process_tx(tx) { + if self.replay_protection_checks(wrapper, tx_bytes.as_slice(), &mut temp_wl_storage).is_ok() { + return Some(tx_bytes.clone()) + } } } None diff --git a/apps/src/lib/node/ledger/shell/process_proposal.rs b/apps/src/lib/node/ledger/shell/process_proposal.rs index acc57e5979..dccba7fe1e 100644 --- a/apps/src/lib/node/ledger/shell/process_proposal.rs +++ b/apps/src/lib/node/ledger/shell/process_proposal.rs @@ -4,7 +4,6 @@ use data_encoding::HEXUPPER; use namada::core::hints; use namada::core::ledger::storage::WlStorage; -use namada::core::types::hash::Hash; use namada::ledger::storage::TempWlStorage; use namada::proof_of_stake::pos_queries::PosQueries; use namada::types::internal::WrapperTxInQueue; @@ -429,54 +428,17 @@ where } } else { // Replay protection checks - let inner_hash_key = - replay_protection::get_tx_hash_key(&wrapper.tx_hash); - if temp_wl_storage.has_key(&inner_hash_key).expect( - "Error while checking inner tx hash key in storage", + if let Err(e) = self.replay_protection_checks( + &wrapper, + tx_bytes, + temp_wl_storage, ) { return TxResult { code: ErrorCodes::ReplayTx.into(), - info: format!( - "Inner transaction hash {} already in \ - storage, replay attempt", - &wrapper.tx_hash - ), + info: e.to_string(), }; } - // Write inner hash to WAL - temp_wl_storage - .write_log - .write(&inner_hash_key, vec![]) - .expect( - "Couldn't write inner transaction hash to write \ - log", - ); - - let tx = Tx::try_from(tx_bytes) - .expect("Deserialization shouldn't fail"); - let wrapper_hash = Hash(tx.unsigned_hash()); - let wrapper_hash_key = - replay_protection::get_tx_hash_key(&wrapper_hash); - if temp_wl_storage.has_key(&wrapper_hash_key).expect( - "Error while checking wrapper tx hash key in storage", - ) { - return TxResult { - code: ErrorCodes::ReplayTx.into(), - info: format!( - "Wrapper transaction hash {} already in \ - storage, replay attempt", - wrapper_hash - ), - }; - } - - // Write wrapper hash to WAL - temp_wl_storage - .write_log - .write(&wrapper_hash_key, vec![]) - .expect("Couldn't write wrapper tx hash to write log"); - // If the public key corresponds to the MASP sentinel // transaction key, then the fee payer is effectively // the MASP, otherwise derive @@ -1168,8 +1130,8 @@ mod test_process_proposal { assert_eq!( response[0].result.info, format!( - "Wrapper transaction hash {} already in storage, \ - replay attempt", + "Transaction replay attempt: Wrapper transaction hash \ + {} already in storage", wrapper_unsigned_hash ) ); @@ -1236,8 +1198,8 @@ mod test_process_proposal { assert_eq!( response[1].result.info, format!( - "Inner transaction hash {} already in storage, replay \ - attempt", + "Transaction replay attempt: Inner transaction hash \ + {} already in storage", wrapper.tx_hash ) ); @@ -1299,8 +1261,8 @@ mod test_process_proposal { assert_eq!( response[0].result.info, format!( - "Inner transaction hash {} already in storage, replay \ - attempt", + "Transaction replay attempt: Inner transaction hash \ + {} already in storage", inner_unsigned_hash ) ); @@ -1395,8 +1357,8 @@ mod test_process_proposal { assert_eq!( response[1].result.info, format!( - "Inner transaction hash {} already in storage, replay \ - attempt", + "Transaction replay attempt: Inner transaction hash \ + {} already in storage", inner_unsigned_hash ) ); From 0bbbc16f4d2d1f8e522db64482634513691b8861 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 19 May 2023 16:45:05 +0200 Subject: [PATCH 2/3] prepare_proposal: add replay protection tests --- .../lib/node/ledger/shell/prepare_proposal.rs | 222 +++++++++++++++++- 1 file changed, 221 insertions(+), 1 deletion(-) diff --git a/apps/src/lib/node/ledger/shell/prepare_proposal.rs b/apps/src/lib/node/ledger/shell/prepare_proposal.rs index 9b19c9717a..41d05fea5d 100644 --- a/apps/src/lib/node/ledger/shell/prepare_proposal.rs +++ b/apps/src/lib/node/ledger/shell/prepare_proposal.rs @@ -278,7 +278,9 @@ where mod test_prepare_proposal { use borsh::BorshSerialize; + use namada::ledger::replay_protection; use namada::proof_of_stake::Epoch; + use namada::types::hash::Hash; use namada::types::transaction::{Fee, WrapperTx}; use super::*; @@ -419,6 +421,224 @@ mod test_prepare_proposal { assert_eq!(received, expected_txs); } + /// Test that if the unsigned wrapper tx hash is known (replay attack), the + /// transaction is not included in the block + #[test] + fn test_wrapper_tx_hash() { + let (mut shell, _) = test_utils::setup(1); + + let keypair = crate::wallet::defaults::daewon_keypair(); + + let tx = Tx::new( + "wasm_code".as_bytes().to_owned(), + Some("transaction data".as_bytes().to_owned()), + shell.chain_id.clone(), + None, + ); + let wrapper = WrapperTx::new( + Fee { + amount: 0.into(), + token: shell.wl_storage.storage.native_token.clone(), + }, + &keypair, + Epoch(0), + 0.into(), + tx, + Default::default(), + #[cfg(not(feature = "mainnet"))] + None, + ); + let signed = wrapper + .sign(&keypair, shell.chain_id.clone(), None) + .expect("Test failed"); + + // Write wrapper hash to storage + let wrapper_unsigned_hash = Hash(signed.unsigned_hash()); + let hash_key = + replay_protection::get_tx_hash_key(&wrapper_unsigned_hash); + shell + .wl_storage + .storage + .write(&hash_key, vec![]) + .expect("Test failed"); + + let req = RequestPrepareProposal { + txs: vec![signed.to_bytes()], + ..Default::default() + }; + + let received = + shell.prepare_proposal(req).txs.into_iter().map(|tx_bytes| { + Tx::try_from(tx_bytes.as_slice()) + .expect("Test failed") + .data + .expect("Test failed") + }); + assert_eq!(received.len(), 0); + } + + /// Test that if two identical wrapper txs are proposed for this block, only + /// one gets accepted + #[test] + fn test_wrapper_tx_hash_same_block() { + let (shell, _) = test_utils::setup(1); + + let keypair = crate::wallet::defaults::daewon_keypair(); + + let tx = Tx::new( + "wasm_code".as_bytes().to_owned(), + Some("transaction data".as_bytes().to_owned()), + shell.chain_id.clone(), + None, + ); + let wrapper = WrapperTx::new( + Fee { + amount: 0.into(), + token: shell.wl_storage.storage.native_token.clone(), + }, + &keypair, + Epoch(0), + 0.into(), + tx, + Default::default(), + #[cfg(not(feature = "mainnet"))] + None, + ); + let signed = wrapper + .sign(&keypair, shell.chain_id.clone(), None) + .expect("Test failed"); + let req = RequestPrepareProposal { + txs: vec![signed.to_bytes(); 2], + ..Default::default() + }; + let received = + shell.prepare_proposal(req).txs.into_iter().map(|tx_bytes| { + Tx::try_from(tx_bytes.as_slice()) + .expect("Test failed") + .data + .expect("Test failed") + }); + assert_eq!(received.len(), 1); + } + + /// Test that if the unsigned inner tx hash is known (replay attack), the + /// transaction is not included in the block + #[test] + fn test_inner_tx_hash() { + let (mut shell, _) = test_utils::setup(1); + + let keypair = crate::wallet::defaults::daewon_keypair(); + + let tx = Tx::new( + "wasm_code".as_bytes().to_owned(), + Some("transaction data".as_bytes().to_owned()), + shell.chain_id.clone(), + None, + ); + let wrapper = WrapperTx::new( + Fee { + amount: 0.into(), + token: shell.wl_storage.storage.native_token.clone(), + }, + &keypair, + Epoch(0), + 0.into(), + tx, + Default::default(), + #[cfg(not(feature = "mainnet"))] + None, + ); + let inner_unsigned_hash = wrapper.tx_hash.clone(); + let signed = wrapper + .sign(&keypair, shell.chain_id.clone(), None) + .expect("Test failed"); + + // Write inner hash to storage + let hash_key = replay_protection::get_tx_hash_key(&inner_unsigned_hash); + shell + .wl_storage + .storage + .write(&hash_key, vec![]) + .expect("Test failed"); + + let req = RequestPrepareProposal { + txs: vec![signed.to_bytes()], + ..Default::default() + }; + + let received = + shell.prepare_proposal(req).txs.into_iter().map(|tx_bytes| { + Tx::try_from(tx_bytes.as_slice()) + .expect("Test failed") + .data + .expect("Test failed") + }); + assert_eq!(received.len(), 0); + } + + /// Test that if two identical decrypted txs are proposed for this block, + /// only one gets accepted + #[test] + fn test_inner_tx_hash_same_block() { + let (shell, _) = test_utils::setup(1); + + let keypair = crate::wallet::defaults::daewon_keypair(); + let keypair_2 = crate::wallet::defaults::daewon_keypair(); + + let tx = Tx::new( + "wasm_code".as_bytes().to_owned(), + Some("transaction data".as_bytes().to_owned()), + shell.chain_id.clone(), + None, + ); + let wrapper = WrapperTx::new( + Fee { + amount: 0.into(), + token: shell.wl_storage.storage.native_token.clone(), + }, + &keypair, + Epoch(0), + 0.into(), + tx.clone(), + Default::default(), + #[cfg(not(feature = "mainnet"))] + None, + ); + let signed = wrapper + .sign(&keypair, shell.chain_id.clone(), None) + .expect("Test failed"); + + let new_wrapper = WrapperTx::new( + Fee { + amount: 0.into(), + token: shell.wl_storage.storage.native_token.clone(), + }, + &keypair_2, + Epoch(0), + 0.into(), + tx, + Default::default(), + #[cfg(not(feature = "mainnet"))] + None, + ); + let new_signed = new_wrapper + .sign(&keypair, shell.chain_id.clone(), None) + .expect("Test failed"); + + let req = RequestPrepareProposal { + txs: vec![signed.to_bytes(), new_signed.to_bytes()], + ..Default::default() + }; + let received = + shell.prepare_proposal(req).txs.into_iter().map(|tx_bytes| { + Tx::try_from(tx_bytes.as_slice()) + .expect("Test failed") + .data + .expect("Test failed") + }); + assert_eq!(received.len(), 1); + } + /// Test that expired wrapper transactions are not included in the block #[test] fn test_expired_wrapper_tx() { @@ -462,6 +682,6 @@ mod test_prepare_proposal { }; let result = shell.prepare_proposal(req); eprintln!("Proposal: {:?}", result.txs); - assert!(result.txs.is_empty()); + assert_eq!(result.txs.len(), 0); } } From 52f803c44240f84aa3e2a160d2111a071c1a7474 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Sat, 20 May 2023 19:17:49 +0200 Subject: [PATCH 3/3] changelog: add #1405 --- .changelog/unreleased/bug-fixes/1405-bugfix-replay-prepare.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/1405-bugfix-replay-prepare.md diff --git a/.changelog/unreleased/bug-fixes/1405-bugfix-replay-prepare.md b/.changelog/unreleased/bug-fixes/1405-bugfix-replay-prepare.md new file mode 100644 index 0000000000..a32b40e2b3 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/1405-bugfix-replay-prepare.md @@ -0,0 +1,3 @@ +- Fixed a bug in `prepare_proposal` causing the creation + of blocks containing already applied transactions. + ([#1405](https://github.com/anoma/namada/pull/1405)) \ No newline at end of file