From d5961e9d9f005966f409fbddd40c3651591b27fb Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Thu, 1 Jul 2021 13:06:59 -0500 Subject: [PATCH] Reject transactions with extra signatures (#18306) * Reject transactions with extra signatures * fix tests * fix check * fix check * tx method * fix checks --- client/src/rpc_custom_error.rs | 10 +++++ ledger/src/blockstore_processor.rs | 7 +++- ledger/src/entry.rs | 67 ++++++++++++++++++++++++++++-- rpc/src/rpc.rs | 4 ++ runtime/src/bank.rs | 5 +++ sdk/src/feature_set.rs | 4 ++ sdk/src/transaction.rs | 5 +++ 7 files changed, 97 insertions(+), 5 deletions(-) diff --git a/client/src/rpc_custom_error.rs b/client/src/rpc_custom_error.rs index 3e80c1a52..82487e3a9 100644 --- a/client/src/rpc_custom_error.rs +++ b/client/src/rpc_custom_error.rs @@ -18,6 +18,7 @@ pub const JSON_RPC_SERVER_ERROR_LONG_TERM_STORAGE_SLOT_SKIPPED: i64 = -32009; pub const JSON_RPC_SERVER_ERROR_KEY_EXCLUDED_FROM_SECONDARY_INDEX: i64 = -32010; pub const JSON_RPC_SERVER_ERROR_TRANSACTION_HISTORY_NOT_AVAILABLE: i64 = -32011; pub const JSON_RPC_SCAN_ERROR: i64 = -32012; +pub const JSON_RPC_SERVER_ERROR_TRANSACTION_SIGNATURE_LEN_MISMATCH: i64 = -32013; #[derive(Error, Debug)] pub enum RpcCustomError { @@ -51,6 +52,8 @@ pub enum RpcCustomError { TransactionHistoryNotAvailable, #[error("ScanError")] ScanError { message: String }, + #[error("TransactionSignatureLenMismatch")] + TransactionSignatureLenMismatch, } #[derive(Debug, Serialize, Deserialize)] @@ -151,6 +154,13 @@ impl From for Error { message, data: None, }, + RpcCustomError::TransactionSignatureLenMismatch => Self { + code: ErrorCode::ServerError( + JSON_RPC_SERVER_ERROR_TRANSACTION_SIGNATURE_LEN_MISMATCH, + ), + message: "Transaction signature length mismatch".to_string(), + data: None, + }, } } } diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 769bdcc58..67352b5f9 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -791,8 +791,11 @@ pub fn confirm_slot( }; let check_start = Instant::now(); - let check_result = - entries.verify_and_hash_transactions(skip_verification, bank.secp256k1_program_enabled()); + let check_result = entries.verify_and_hash_transactions( + skip_verification, + bank.secp256k1_program_enabled(), + bank.verify_tx_signatures_len_enabled(), + ); if check_result.is_none() { warn!("Ledger proof of history failed at slot: {}", slot); return Err(BlockError::InvalidEntryHash.into()); diff --git a/ledger/src/entry.rs b/ledger/src/entry.rs index 2c45bf9c9..98774fcd6 100644 --- a/ledger/src/entry.rs +++ b/ledger/src/entry.rs @@ -360,6 +360,7 @@ pub trait EntrySlice { &self, skip_verification: bool, secp256k1_program_enabled: bool, + verify_tx_signatures_len: bool, ) -> Option>>; } @@ -515,6 +516,7 @@ impl EntrySlice for [Entry] { &'a self, skip_verification: bool, secp256k1_program_enabled: bool, + verify_tx_signatures_len: bool, ) -> Option>> { let verify_and_hash = |tx: &'a Transaction| -> Option> { let message_hash = if !skip_verification { @@ -526,6 +528,9 @@ impl EntrySlice for [Entry] { // Verify tx precompiles if secp256k1 program is enabled. tx.verify_precompiles().ok()?; } + if verify_tx_signatures_len && !tx.verify_signatures_len() { + return None; + } tx.verify_and_hash_message().ok()? } else { tx.message().hash() @@ -885,6 +890,62 @@ mod tests { assert!(!bad_ticks.verify(&one)); // inductive step, bad } + #[test] + fn test_verify_and_hash_transactions_sig_len() { + let mut rng = rand::thread_rng(); + let recent_blockhash = hash_new_rand(&mut rng); + let from_keypair = Keypair::new(); + let to_keypair = Keypair::new(); + let from_pubkey = from_keypair.pubkey(); + let to_pubkey = to_keypair.pubkey(); + + enum TestCase { + AddSignature, + RemoveSignature, + } + + let make_transaction = |case: TestCase| { + let message = Message::new( + &[system_instruction::transfer(&from_pubkey, &to_pubkey, 1)], + Some(&from_pubkey), + ); + let mut tx = Transaction::new(&[&from_keypair], message, recent_blockhash); + assert_eq!(tx.message.header.num_required_signatures, 1); + match case { + TestCase::AddSignature => { + let signature = to_keypair.sign_message(&tx.message.serialize()); + tx.signatures.push(signature); + } + TestCase::RemoveSignature => { + tx.signatures.remove(0); + } + } + tx + }; + // No signatures. + { + let tx = make_transaction(TestCase::RemoveSignature); + let entries = vec![next_entry(&recent_blockhash, 1, vec![tx])]; + assert!(entries[..] + .verify_and_hash_transactions(false, false, false) + .is_some()); + assert!(entries[..] + .verify_and_hash_transactions(false, false, true) + .is_none()); + } + // Too many signatures. + { + let tx = make_transaction(TestCase::AddSignature); + let entries = vec![next_entry(&recent_blockhash, 1, vec![tx])]; + assert!(entries[..] + .verify_and_hash_transactions(false, false, false) + .is_some()); + assert!(entries[..] + .verify_and_hash_transactions(false, false, true) + .is_none()); + } + } + #[test] fn test_verify_and_hash_transactions_packet_data_size() { let mut rng = rand::thread_rng(); @@ -906,7 +967,7 @@ mod tests { let entries = vec![next_entry(&recent_blockhash, 1, vec![tx.clone()])]; assert!(bincode::serialized_size(&tx).unwrap() <= PACKET_DATA_SIZE as u64); assert!(entries[..] - .verify_and_hash_transactions(false, false) + .verify_and_hash_transactions(false, false, false) .is_some()); } // Big transaction. @@ -915,7 +976,7 @@ mod tests { let entries = vec![next_entry(&recent_blockhash, 1, vec![tx.clone()])]; assert!(bincode::serialized_size(&tx).unwrap() > PACKET_DATA_SIZE as u64); assert!(entries[..] - .verify_and_hash_transactions(false, false) + .verify_and_hash_transactions(false, false, false) .is_none()); } // Assert that verify fails as soon as serialized @@ -926,7 +987,7 @@ mod tests { assert_eq!( bincode::serialized_size(&tx).unwrap() <= PACKET_DATA_SIZE as u64, entries[..] - .verify_and_hash_transactions(false, false) + .verify_and_hash_transactions(false, false, false) .is_some(), ); } diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index 131f9b9b6..2eebfcae9 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -1899,6 +1899,10 @@ fn verify_transaction(transaction: &Transaction) -> Result<()> { return Err(RpcCustomError::TransactionPrecompileVerificationFailure(e).into()); } + if !transaction.verify_signatures_len() { + return Err(RpcCustomError::TransactionSignatureVerificationFailure.into()); + } + Ok(()) } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 08f255aa7..a56289083 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -5051,6 +5051,11 @@ impl Bank { .is_active(&feature_set::check_init_vote_data::id()) } + pub fn verify_tx_signatures_len_enabled(&self) -> bool { + self.feature_set + .is_active(&feature_set::verify_tx_signatures_len::id()) + } + // Check if the wallclock time from bank creation to now has exceeded the allotted // time for transaction processing pub fn should_bank_still_be_processing_txs( diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 1496600bc..c210e804a 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -147,6 +147,10 @@ pub mod dedupe_config_program_signers { solana_sdk::declare_id!("8kEuAshXLsgkUEdcFVLqrjCGGHVWFW99ZZpxvAzzMtBp"); } +pub mod verify_tx_signatures_len { + solana_sdk::declare_id!("EVW9B5xD9FFK7vw1SBARwMA4s5eRo5eKJdKpsBikzKBz"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ diff --git a/sdk/src/transaction.rs b/sdk/src/transaction.rs index 2125043ee..9d2a3c0bf 100644 --- a/sdk/src/transaction.rs +++ b/sdk/src/transaction.rs @@ -361,6 +361,11 @@ impl Transaction { } } + /// Verify the length of signatures matches the value in the message header + pub fn verify_signatures_len(&self) -> bool { + self.signatures.len() == self.message.header.num_required_signatures as usize + } + /// Verify the transaction and hash its message pub fn verify_and_hash_message(&self) -> Result { let message_bytes = self.message_data();