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

@@ -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<RpcCustomError> 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,
+            },
         }
     }
 }

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

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<Vec<EntryType<'_>>>;
 }
 
@@ -515,6 +516,7 @@ impl EntrySlice for [Entry] {
         &'a self,
         skip_verification: bool,
         secp256k1_program_enabled: bool,
+        verify_tx_signatures_len: bool,
     ) -> Option<Vec<EntryType<'a>>> {
         let verify_and_hash = |tx: &'a Transaction| -> Option<HashedTransaction<'a>> {
             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(),
             );
         }

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

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(

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

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<Hash> {
         let message_bytes = self.message_data();