Fail secp256k1 if the instruction data looks incorrect (#19300)

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));
         }

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<RwLock<CostTracker>>,
         banking_stage_stats: &BankingStageStats,
     ) -> (Vec<SanitizedTransaction>, Vec<usize>, Vec<usize>) {
@@ -1062,7 +1063,10 @@ impl BankingStage {
                     Err(TransactionError::UnsupportedVersion)
                 })
                 .ok()?;
-                tx.verify_precompiles(libsecp256k1_0_5_upgrade_enabled)
+                tx.verify_precompiles(
+                    libsecp256k1_0_5_upgrade_enabled,
+                    libsecp256k1_fail_on_bad_count,
+                )
                 .ok()?;
                 Some((tx, *tx_index))
             })
@@ -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,
             );

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());
     }
 }

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 {

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())

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<Pubkey, &'static str> = [
@@ -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()

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)
+        );
+    }
 }

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)?;
             }

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)?;
             }