From 3ec33e7d02475e0d0a65d9f2805389f8bed993cd Mon Sep 17 00:00:00 2001 From: Jack May Date: Thu, 19 Aug 2021 13:13:54 -0700 Subject: [PATCH] Fail secp256k1 if the instruction data looks incorrect (#19300) --- banks-server/src/banks_server.rs | 7 ++++++- core/src/banking_stage.rs | 10 ++++++++-- programs/secp256k1/src/lib.rs | 4 ++-- rpc/src/rpc.rs | 13 +++++++++++-- runtime/src/bank.rs | 7 ++++++- sdk/src/feature_set.rs | 5 +++++ sdk/src/secp256k1_instruction.rs | 26 ++++++++++++++++++++++++-- sdk/src/transaction/mod.rs | 7 ++++++- sdk/src/transaction/sanitized.rs | 7 ++++++- 9 files changed, 74 insertions(+), 12 deletions(-) diff --git a/banks-server/src/banks_server.rs b/banks-server/src/banks_server.rs index 4dace6464d..b528d5963c 100644 --- a/banks-server/src/banks_server.rs +++ b/banks-server/src/banks_server.rs @@ -131,10 +131,14 @@ impl BanksServer { fn verify_transaction( transaction: &Transaction, libsecp256k1_0_5_upgrade_enabled: bool, + libsecp256k1_fail_on_bad_count: bool, ) -> transaction::Result<()> { if let Err(err) = transaction.verify() { Err(err) - } else if let Err(err) = transaction.verify_precompiles(libsecp256k1_0_5_upgrade_enabled) { + } else if let Err(err) = transaction.verify_precompiles( + libsecp256k1_0_5_upgrade_enabled, + libsecp256k1_fail_on_bad_count, + ) { Err(err) } else { Ok(()) @@ -228,6 +232,7 @@ impl Banks for BanksServer { if let Err(err) = verify_transaction( &transaction, self.bank(commitment).libsecp256k1_0_5_upgrade_enabled(), + self.bank(commitment).libsecp256k1_fail_on_bad_count(), ) { return Some(Err(err)); } diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index 1ae719e069..97bffd0931 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -1046,6 +1046,7 @@ impl BankingStage { msgs: &Packets, transaction_indexes: &[usize], libsecp256k1_0_5_upgrade_enabled: bool, + libsecp256k1_fail_on_bad_count: bool, cost_tracker: &Arc>, banking_stage_stats: &BankingStageStats, ) -> (Vec, Vec, Vec) { @@ -1062,8 +1063,11 @@ impl BankingStage { Err(TransactionError::UnsupportedVersion) }) .ok()?; - tx.verify_precompiles(libsecp256k1_0_5_upgrade_enabled) - .ok()?; + tx.verify_precompiles( + libsecp256k1_0_5_upgrade_enabled, + libsecp256k1_fail_on_bad_count, + ) + .ok()?; Some((tx, *tx_index)) }) .collect(); @@ -1158,6 +1162,7 @@ impl BankingStage { msgs, &packet_indexes, bank.libsecp256k1_0_5_upgrade_enabled(), + bank.libsecp256k1_fail_on_bad_count(), cost_tracker, banking_stage_stats, ); @@ -1260,6 +1265,7 @@ impl BankingStage { msgs, transaction_indexes, bank.libsecp256k1_0_5_upgrade_enabled(), + bank.libsecp256k1_fail_on_bad_count(), cost_tracker, banking_stage_stats, ); diff --git a/programs/secp256k1/src/lib.rs b/programs/secp256k1/src/lib.rs index 2f1aee3e1e..a54163f4d9 100644 --- a/programs/secp256k1/src/lib.rs +++ b/programs/secp256k1/src/lib.rs @@ -44,7 +44,7 @@ pub mod test { Hash::default(), ); - assert!(tx.verify_precompiles(false).is_ok()); + assert!(tx.verify_precompiles(false, true).is_ok()); let index = thread_rng().gen_range(0, secp_instruction.data.len()); secp_instruction.data[index] = secp_instruction.data[index].wrapping_add(12); @@ -54,6 +54,6 @@ pub mod test { &[&mint_keypair], Hash::default(), ); - assert!(tx.verify_precompiles(false).is_err()); + assert!(tx.verify_precompiles(false, true).is_err()); } } diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index 1545d6d65d..e61047f752 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -1965,12 +1965,16 @@ impl JsonRpcRequestProcessor { fn verify_transaction( transaction: &SanitizedTransaction, libsecp256k1_0_5_upgrade_enabled: bool, + libsecp256k1_fail_on_bad_count: bool, ) -> Result<()> { if transaction.verify().is_err() { return Err(RpcCustomError::TransactionSignatureVerificationFailure.into()); } - if let Err(e) = transaction.verify_precompiles(libsecp256k1_0_5_upgrade_enabled) { + if let Err(e) = transaction.verify_precompiles( + libsecp256k1_0_5_upgrade_enabled, + libsecp256k1_fail_on_bad_count, + ) { return Err(RpcCustomError::TransactionPrecompileVerificationFailure(e).into()); } @@ -3324,6 +3328,7 @@ pub mod rpc_full { if let Err(e) = verify_transaction( &transaction, preflight_bank.libsecp256k1_0_5_upgrade_enabled(), + preflight_bank.libsecp256k1_fail_on_bad_count(), ) { return Err(e); } @@ -3409,7 +3414,11 @@ pub mod rpc_full { let transaction = sanitize_transaction(unsanitized_tx)?; if config.sig_verify { - verify_transaction(&transaction, bank.libsecp256k1_0_5_upgrade_enabled())?; + verify_transaction( + &transaction, + bank.libsecp256k1_0_5_upgrade_enabled(), + bank.libsecp256k1_fail_on_bad_count(), + )?; } let TransactionSimulationResult { diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 071402317a..2ab5acedaa 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -5011,7 +5011,7 @@ impl Bank { } if !skip_verification { - sanitized_tx.verify_precompiles(self.libsecp256k1_0_5_upgrade_enabled())?; + sanitized_tx.verify_precompiles(self.libsecp256k1_0_5_upgrade_enabled(), true)?; } Ok(sanitized_tx) @@ -5467,6 +5467,11 @@ impl Bank { .is_active(&feature_set::libsecp256k1_0_5_upgrade_enabled::id()) } + pub fn libsecp256k1_fail_on_bad_count(&self) -> bool { + self.feature_set + .is_active(&feature_set::libsecp256k1_fail_on_bad_count::id()) + } + pub fn merge_nonce_error_into_system_error(&self) -> bool { self.feature_set .is_active(&feature_set::merge_nonce_error_into_system_error::id()) diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index ea24c6b841..804ae9f58f 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -199,6 +199,10 @@ pub mod versioned_tx_message_enabled { solana_sdk::declare_id!("3KZZ6Ks1885aGBQ45fwRcPXVBCtzUvxhUTkwKMR41Tca"); } +pub mod libsecp256k1_fail_on_bad_count { + solana_sdk::declare_id!("8aXvSuopd1PUj7UhehfXJRg6619RHp8ZvwTyyJHdUYsj"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -243,6 +247,7 @@ lazy_static! { (gate_large_block::id(), "validator checks block cost against max limit in realtime, reject if exceeds."), (mem_overlap_fix::id(), "memory overlap fix"), (versioned_tx_message_enabled::id(), "enable versioned transaction message processing"), + (libsecp256k1_fail_on_bad_count::id(), "Fail libsec256k1_verify if count appears wrong") /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/sdk/src/secp256k1_instruction.rs b/sdk/src/secp256k1_instruction.rs index c6d9bd2b16..b60b9c6103 100644 --- a/sdk/src/secp256k1_instruction.rs +++ b/sdk/src/secp256k1_instruction.rs @@ -103,11 +103,18 @@ pub fn verify_eth_addresses( data: &[u8], instruction_datas: &[&[u8]], libsecp256k1_0_5_upgrade_enabled: bool, + libsecp256k1_fail_on_bad_count: bool, ) -> Result<(), Secp256k1Error> { if data.is_empty() { return Err(Secp256k1Error::InvalidInstructionDataSize); } let count = data[0] as usize; + if libsecp256k1_fail_on_bad_count && count == 0 && data.len() > 1 { + // count is zero but the instruction data indicates that is probably not + // correct, fail the instruction to catch probable invalid secp256k1 + // instruction construction. + return Err(Secp256k1Error::InvalidInstructionDataSize); + } let expected_data_size = count .saturating_mul(SIGNATURE_OFFSETS_SERIALIZED_SIZE) .saturating_add(1); @@ -215,7 +222,7 @@ pub mod test { let writer = std::io::Cursor::new(&mut instruction_data[1..]); bincode::serialize_into(writer, &offsets).unwrap(); - verify_eth_addresses(&instruction_data, &[&[0u8; 100]], false) + verify_eth_addresses(&instruction_data, &[&[0u8; 100]], false, true) } #[test] @@ -230,7 +237,7 @@ pub mod test { instruction_data.truncate(instruction_data.len() - 1); assert_eq!( - verify_eth_addresses(&instruction_data, &[&[0u8; 100]], false), + verify_eth_addresses(&instruction_data, &[&[0u8; 100]], false, true), Err(Secp256k1Error::InvalidInstructionDataSize) ); @@ -346,4 +353,19 @@ pub mod test { Err(Secp256k1Error::InvalidSignature) ); } + + #[test] + fn test_count_is_zero_but_sig_data_exists() { + solana_logger::setup(); + + let mut instruction_data = vec![0u8; DATA_START]; + let offsets = SecpSignatureOffsets::default(); + instruction_data[0] = 0; + let writer = std::io::Cursor::new(&mut instruction_data[1..]); + bincode::serialize_into(writer, &offsets).unwrap(); + assert_eq!( + verify_eth_addresses(&instruction_data, &[&[0u8; 100]], false, true), + Err(Secp256k1Error::InvalidInstructionDataSize) + ); + } } diff --git a/sdk/src/transaction/mod.rs b/sdk/src/transaction/mod.rs index 92bb71b698..c4cdcdcf6c 100644 --- a/sdk/src/transaction/mod.rs +++ b/sdk/src/transaction/mod.rs @@ -426,7 +426,11 @@ impl Transaction { .collect() } - pub fn verify_precompiles(&self, libsecp256k1_0_5_upgrade_enabled: bool) -> Result<()> { + pub fn verify_precompiles( + &self, + libsecp256k1_0_5_upgrade_enabled: bool, + libsecp256k1_fail_on_bad_count: bool, + ) -> Result<()> { for instruction in &self.message().instructions { // The Transaction may not be sanitized at this point if instruction.program_id_index as usize >= self.message().account_keys.len() { @@ -445,6 +449,7 @@ impl Transaction { data, &instruction_datas, libsecp256k1_0_5_upgrade_enabled, + libsecp256k1_fail_on_bad_count, ); e.map_err(|_| TransactionError::InvalidAccountIndex)?; } diff --git a/sdk/src/transaction/sanitized.rs b/sdk/src/transaction/sanitized.rs index 1bcd262441..592468988d 100644 --- a/sdk/src/transaction/sanitized.rs +++ b/sdk/src/transaction/sanitized.rs @@ -203,7 +203,11 @@ impl SanitizedTransaction { } /// Verify the encoded secp256k1 signatures in this transaction - pub fn verify_precompiles(&self, libsecp256k1_0_5_upgrade_enabled: bool) -> Result<()> { + pub fn verify_precompiles( + &self, + libsecp256k1_0_5_upgrade_enabled: bool, + libsecp256k1_fail_on_bad_count: bool, + ) -> Result<()> { for (program_id, instruction) in self.message.program_instructions_iter() { if secp256k1_program::check_id(program_id) { let instruction_datas: Vec<_> = self @@ -217,6 +221,7 @@ impl SanitizedTransaction { data, &instruction_datas, libsecp256k1_0_5_upgrade_enabled, + libsecp256k1_fail_on_bad_count, ); e.map_err(|_| TransactionError::InvalidAccountIndex)?; }