Fail tx sanitization when ix program id uses lookup table (#25035)

* Fail tx sanitization when ix program id uses lookup table

* feedback

core/src/banking_stage.rs

@@ -1742,6 +1742,7 @@ impl BankingStage {
             *deserialized_packet.message_hash(),
             Some(deserialized_packet.is_simple_vote()),
             address_loader,
+            feature_set.is_active(&feature_set::require_static_program_ids_in_transaction::ID),
         )
         .ok()?;
         tx.verify_precompiles(feature_set).ok()?;
@@ -3667,6 +3668,7 @@ mod tests {
             MessageHash::Compute,
             Some(false),
             bank.as_ref(),
+            true, // require_static_program_ids
         )
         .unwrap();
 

entry/benches/entry_sigverify.rs

@@ -40,6 +40,7 @@ fn bench_gpusigverify(bencher: &mut Bencher) {
                     message_hash,
                     None,
                     SimpleAddressLoader::Disabled,
+                    true, // require_static_program_ids
                 )
             }?;
 
@@ -81,6 +82,7 @@ fn bench_cpusigverify(bencher: &mut Bencher) {
                     message_hash,
                     None,
                     SimpleAddressLoader::Disabled,
+                    true, // require_static_program_ids
                 )
             }?;
 

entry/src/entry.rs

@@ -1014,6 +1014,7 @@ mod tests {
                         message_hash,
                         None,
                         SimpleAddressLoader::Disabled,
+                        true, // require_static_program_ids
                     )
                 }?;
 

ledger-tool/src/main.rs

@@ -243,6 +243,7 @@ fn output_slot(
                     MessageHash::Compute,
                     None,
                     SimpleAddressLoader::Disabled,
+                    true, // require_static_program_ids
                 );
 
                 match sanitize_result {
@@ -813,6 +814,7 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String>
                     MessageHash::Compute,
                     None,
                     SimpleAddressLoader::Disabled,
+                    true, // require_static_program_ids
                 )
                 .map_err(|err| {
                     warn!("Failed to compute cost of transaction: {:?}", err);

ledger/src/blockstore.rs

@@ -35,7 +35,6 @@ use {
         genesis_config::{GenesisConfig, DEFAULT_GENESIS_ARCHIVE, DEFAULT_GENESIS_FILE},
         hash::Hash,
         pubkey::Pubkey,
-        sanitize::Sanitize,
         signature::{Keypair, Signature, Signer},
         timing::timestamp,
         transaction::VersionedTransaction,
@@ -1979,7 +1978,12 @@ impl Blockstore {
                     .into_iter()
                     .flat_map(|entry| entry.transactions)
                     .map(|transaction| {
-                        if let Err(err) = transaction.sanitize() {
+                        if let Err(err) = transaction.sanitize(
+                            // Don't enable additional sanitization checks until
+                            // all clusters have activated the static program id
+                            // feature gate so that bigtable upload isn't affected
+                            false, // require_static_program_ids
+                        ) {
                             warn!(
                                 "Blockstore::get_block sanitize failed: {:?}, \
                                 slot: {:?}, \
@@ -2358,7 +2362,9 @@ impl Blockstore {
             .cloned()
             .flat_map(|entry| entry.transactions)
             .map(|transaction| {
-                if let Err(err) = transaction.sanitize() {
+                if let Err(err) = transaction.sanitize(
+                    true, // require_static_program_ids
+                ) {
                     warn!(
                         "Blockstore::find_transaction_in_slot sanitize failed: {:?}, \
                         slot: {:?}, \

rpc/src/rpc.rs

@@ -4291,8 +4291,14 @@ fn sanitize_transaction(
     transaction: VersionedTransaction,
     address_loader: impl AddressLoader,
 ) -> Result<SanitizedTransaction> {
-    SanitizedTransaction::try_create(transaction, MessageHash::Compute, None, address_loader)
-        .map_err(|err| Error::invalid_params(format!("invalid transaction: {}", err)))
+    SanitizedTransaction::try_create(
+        transaction,
+        MessageHash::Compute,
+        None,
+        address_loader,
+        true, // require_static_program_ids
+    )
+    .map_err(|err| Error::invalid_params(format!("invalid transaction: {}", err)))
 }
 
 pub(crate) fn create_validator_exit(exit: &Arc<AtomicBool>) -> Arc<RwLock<Exit>> {

rpc/src/transaction_status_service.rs

@@ -315,6 +315,7 @@ pub(crate) mod tests {
             MessageHash::Compute,
             None,
             SimpleAddressLoader::Disabled,
+            true, // require_static_program_ids
         )
         .unwrap();
 

runtime/src/bank.rs

@@ -3800,7 +3800,16 @@ impl Bank {
     pub fn prepare_entry_batch(&self, txs: Vec<VersionedTransaction>) -> Result<TransactionBatch> {
         let sanitized_txs = txs
             .into_iter()
-            .map(|tx| SanitizedTransaction::try_create(tx, MessageHash::Compute, None, self))
+            .map(|tx| {
+                SanitizedTransaction::try_create(
+                    tx,
+                    MessageHash::Compute,
+                    None,
+                    self,
+                    self.feature_set
+                        .is_active(&feature_set::require_static_program_ids_in_transaction::ID),
+                )
+            })
             .collect::<Result<Vec<_>>>()?;
         let lock_results = self
             .rc
@@ -6327,7 +6336,14 @@ impl Bank {
                 tx.message.hash()
             };
 
-            SanitizedTransaction::try_create(tx, message_hash, None, self)
+            SanitizedTransaction::try_create(
+                tx,
+                message_hash,
+                None,
+                self,
+                self.feature_set
+                    .is_active(&feature_set::require_static_program_ids_in_transaction::ID),
+            )
         }?;
 
         if verification_mode == TransactionVerificationMode::HashAndVerifyPrecompiles

runtime/src/cost_tracker.rs

@@ -356,6 +356,7 @@ mod tests {
             MessageHash::Compute,
             Some(true),
             SimpleAddressLoader::Disabled,
+            true, // require_static_program_ids
         )
         .unwrap();
         let mut tx_cost = TransactionCost::new_with_capacity(1);

sdk/program/src/message/versions/mod.rs

@@ -36,6 +36,13 @@ pub enum VersionedMessage {
 }
 
 impl VersionedMessage {
+    pub fn sanitize(&self, require_static_program_ids: bool) -> Result<(), SanitizeError> {
+        match self {
+            Self::Legacy(message) => message.sanitize(),
+            Self::V0(message) => message.sanitize(require_static_program_ids),
+        }
+    }
+
     pub fn header(&self) -> &MessageHeader {
         match self {
             Self::Legacy(message) => &message.header,
@@ -148,15 +155,6 @@ impl Default for VersionedMessage {
     }
 }
 
-impl Sanitize for VersionedMessage {
-    fn sanitize(&self) -> Result<(), SanitizeError> {
-        match self {
-            Self::Legacy(message) => message.sanitize(),
-            Self::V0(message) => message.sanitize(),
-        }
-    }
-}
-
 impl Serialize for VersionedMessage {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
     where

sdk/program/src/message/versions/v0/mod.rs

@@ -19,7 +19,7 @@ use crate::{
         MessageHeader, MESSAGE_VERSION_PREFIX,
     },
     pubkey::Pubkey,
-    sanitize::{Sanitize, SanitizeError},
+    sanitize::SanitizeError,
     short_vec, sysvar,
 };
 pub use loaded::*;
@@ -52,7 +52,9 @@ pub struct MessageAddressTableLookup {
 #[derive(Serialize, Deserialize, Default, Debug, PartialEq, Eq, Clone, AbiExample)]
 #[serde(rename_all = "camelCase")]
 pub struct Message {
-    /// The message header, identifying signed and read-only `account_keys`
+    /// The message header, identifying signed and read-only `account_keys`.
+    /// Header values only describe static `account_keys`, they do not describe
+    /// any additional account keys loaded via address table lookups.
     pub header: MessageHeader,
 
     /// List of accounts loaded by this transaction.
@@ -67,7 +69,10 @@ pub struct Message {
     ///
     /// # Notes
     ///
-    /// Account and program indexes will index into the list of addresses
+    /// Program indexes must index into the list of message `account_keys` because
+    /// program id's cannot be dynamically loaded from a lookup table.
+    ///
+    /// Account indexes must index into the list of addresses
     /// constructed from the concatenation of three key lists:
     ///   1) message `account_keys`
     ///   2) ordered list of keys loaded from `writable` lookup table indexes
@@ -81,13 +86,13 @@ pub struct Message {
     pub address_table_lookups: Vec<MessageAddressTableLookup>,
 }
 
-impl Sanitize for Message {
-    fn sanitize(&self) -> Result<(), SanitizeError> {
-        // signing area and read-only non-signing area should not
-        // overlap
+impl Message {
+    /// Sanitize message fields and compiled instruction indexes
+    pub fn sanitize(&self, reject_dynamic_program_ids: bool) -> Result<(), SanitizeError> {
+        let num_static_account_keys = self.account_keys.len();
         if usize::from(self.header.num_required_signatures)
             .saturating_add(usize::from(self.header.num_readonly_unsigned_accounts))
-            > self.account_keys.len()
+            > num_static_account_keys
         {
             return Err(SanitizeError::IndexOutOfBounds);
         }
@@ -97,29 +102,59 @@ impl Sanitize for Message {
             return Err(SanitizeError::InvalidValue);
         }
 
-        let mut num_loaded_accounts = self.account_keys.len();
-        for lookup in &self.address_table_lookups {
-            let num_table_loaded_accounts = lookup
-                .writable_indexes
-                .len()
-                .saturating_add(lookup.readonly_indexes.len());
+        let num_dynamic_account_keys = {
+            let mut total_lookup_keys: usize = 0;
+            for lookup in &self.address_table_lookups {
+                let num_lookup_indexes = lookup
+                    .writable_indexes
+                    .len()
+                    .saturating_add(lookup.readonly_indexes.len());
 
-            // each lookup table must be used to load at least one account
-            if num_table_loaded_accounts == 0 {
-                return Err(SanitizeError::InvalidValue);
+                // each lookup table must be used to load at least one account
+                if num_lookup_indexes == 0 {
+                    return Err(SanitizeError::InvalidValue);
+                }
+
+                total_lookup_keys = total_lookup_keys.saturating_add(num_lookup_indexes);
             }
+            total_lookup_keys
+        };
 
-            num_loaded_accounts = num_loaded_accounts.saturating_add(num_table_loaded_accounts);
+        // this is redundant with the above sanitization checks which require that:
+        // 1) the header describes at least 1 RW account
+        // 2) the header doesn't describe more account keys than the number of account keys
+        if num_static_account_keys == 0 {
+            return Err(SanitizeError::InvalidValue);
         }
 
-        // the number of loaded accounts must be <= 256 since account indices are
-        // encoded as `u8`
-        if num_loaded_accounts > 256 {
+        // the combined number of static and dynamic account keys must be <= 256
+        // since account indices are encoded as `u8`
+        let total_account_keys = num_static_account_keys.saturating_add(num_dynamic_account_keys);
+        if total_account_keys > 256 {
             return Err(SanitizeError::IndexOutOfBounds);
         }
 
+        // `expect` is safe because of earlier check that
+        // `num_static_account_keys` is non-zero
+        let max_account_ix = total_account_keys
+            .checked_sub(1)
+            .expect("message doesn't contain any account keys");
+
+        // switch to rejecting program ids loaded from lookup tables so that
+        // static analysis on program instructions can be performed without
+        // loading on-chain data from a bank
+        let max_program_id_ix = if reject_dynamic_program_ids {
+            // `expect` is safe because of earlier check that
+            // `num_static_account_keys` is non-zero
+            num_static_account_keys
+                .checked_sub(1)
+                .expect("message doesn't contain any static account keys")
+        } else {
+            max_account_ix
+        };
+
         for ci in &self.instructions {
-            if usize::from(ci.program_id_index) >= num_loaded_accounts {
+            if usize::from(ci.program_id_index) > max_program_id_ix {
                 return Err(SanitizeError::IndexOutOfBounds);
             }
             // A program cannot be a payer.
@@ -127,7 +162,7 @@ impl Sanitize for Message {
                 return Err(SanitizeError::IndexOutOfBounds);
             }
             for ai in &ci.accounts {
-                if usize::from(*ai) >= num_loaded_accounts {
+                if usize::from(*ai) > max_account_ix {
                     return Err(SanitizeError::IndexOutOfBounds);
                 }
             }
@@ -337,7 +372,9 @@ mod tests {
             account_keys: vec![Pubkey::new_unique()],
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_ok());
     }
 
@@ -356,7 +393,9 @@ mod tests {
             }],
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_ok());
     }
 
@@ -375,13 +414,15 @@ mod tests {
             }],
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_ok());
     }
 
     #[test]
-    fn test_sanitize_with_table_lookup_and_ix() {
-        assert!(Message {
+    fn test_sanitize_with_table_lookup_and_ix_with_dynamic_program_id() {
+        let message = Message {
             header: MessageHeader {
                 num_required_signatures: 1,
                 ..MessageHeader::default()
@@ -395,11 +436,43 @@ mod tests {
             instructions: vec![CompiledInstruction {
                 program_id_index: 4,
                 accounts: vec![0, 1, 2, 3],
+                data: vec![],
+            }],
+            ..Message::default()
+        };
+
+        assert!(message.sanitize(
+            false, // require_static_program_ids
+        ).is_ok());
+
+        assert!(message.sanitize(
+            true, // require_static_program_ids
+        ).is_err());
+    }
+
+    #[test]
+    fn test_sanitize_with_table_lookup_and_ix_with_static_program_id() {
+        assert!(Message {
+            header: MessageHeader {
+                num_required_signatures: 1,
+                ..MessageHeader::default()
+            },
+            account_keys: vec![Pubkey::new_unique(), Pubkey::new_unique()],
+            address_table_lookups: vec![MessageAddressTableLookup {
+                account_key: Pubkey::new_unique(),
+                writable_indexes: vec![1, 2, 3],
+                readonly_indexes: vec![0],
+            }],
+            instructions: vec![CompiledInstruction {
+                program_id_index: 1,
+                accounts: vec![2, 3, 4, 5],
                 data: vec![]
             }],
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_ok());
     }
 
@@ -410,7 +483,9 @@ mod tests {
             account_keys: vec![Pubkey::new_unique()],
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_err());
     }
 
@@ -425,7 +500,9 @@ mod tests {
             account_keys: vec![Pubkey::new_unique()],
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_err());
     }
 
@@ -444,7 +521,9 @@ mod tests {
             }],
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_err());
     }
 
@@ -458,7 +537,9 @@ mod tests {
             account_keys: (0..=u8::MAX).map(|_| Pubkey::new_unique()).collect(),
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_ok());
     }
 
@@ -472,7 +553,9 @@ mod tests {
             account_keys: (0..=256).map(|_| Pubkey::new_unique()).collect(),
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_err());
     }
 
@@ -491,7 +574,9 @@ mod tests {
             }],
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_ok());
     }
 
@@ -510,13 +595,15 @@ mod tests {
             }],
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_err());
     }
 
     #[test]
     fn test_sanitize_with_invalid_ix_program_id() {
-        assert!(Message {
+        let message = Message {
             header: MessageHeader {
                 num_required_signatures: 1,
                 ..MessageHeader::default()
@@ -530,12 +617,17 @@ mod tests {
             instructions: vec![CompiledInstruction {
                 program_id_index: 2,
                 accounts: vec![],
-                data: vec![]
+                data: vec![],
             }],
             ..Message::default()
-        }
-        .sanitize()
-        .is_err());
+        };
+
+        assert!(message
+            .sanitize(true /* require_static_program_ids */)
+            .is_err());
+        assert!(message
+            .sanitize(false /* require_static_program_ids */)
+            .is_err());
     }
 
     #[test]
@@ -545,7 +637,7 @@ mod tests {
                 num_required_signatures: 1,
                 ..MessageHeader::default()
             },
-            account_keys: vec![Pubkey::new_unique()],
+            account_keys: vec![Pubkey::new_unique(), Pubkey::new_unique()],
             address_table_lookups: vec![MessageAddressTableLookup {
                 account_key: Pubkey::new_unique(),
                 writable_indexes: vec![],
@@ -553,12 +645,14 @@ mod tests {
             }],
             instructions: vec![CompiledInstruction {
                 program_id_index: 1,
-                accounts: vec![2],
+                accounts: vec![3],
                 data: vec![]
             }],
             ..Message::default()
         }
-        .sanitize()
+        .sanitize(
+            true, // require_static_program_ids
+        )
         .is_err());
     }
 

sdk/src/feature_set.rs

@@ -383,6 +383,10 @@ pub mod stake_allow_zero_undelegated_amount {
     solana_sdk::declare_id!("sTKz343FM8mqtyGvYWvbLpTThw3ixRM4Xk8QvZ985mw");
 }
 
+pub mod require_static_program_ids_in_transaction {
+    solana_sdk::declare_id!("8FdwgyHFEjhAdjWfV2vfqk7wA1g9X3fQpKH7SBpEv3kC");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -472,7 +476,8 @@ lazy_static! {
         (spl_token_v3_4_0::id(), "SPL Token Program version 3.4.0 release #24740"),
         (spl_associated_token_account_v1_1_0::id(), "SPL Associated Token Account Program version 1.1.0 release #24741"),
         (default_units_per_instruction::id(), "Default max tx-wide compute units calculated per instruction"),
-        (stake_allow_zero_undelegated_amount::id(), "Allow zero-lamport undelegated amount for initialized stakes #24670")
+        (stake_allow_zero_undelegated_amount::id(), "Allow zero-lamport undelegated amount for initialized stakes #24670"),
+        (require_static_program_ids_in_transaction::id(), "require static program ids in versioned transactions"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()

sdk/src/transaction/sanitized.rs

@@ -80,8 +80,9 @@ impl SanitizedTransaction {
         message_hash: impl Into<MessageHash>,
         is_simple_vote_tx: Option<bool>,
         address_loader: impl AddressLoader,
+        require_static_program_ids: bool,
     ) -> Result<Self> {
-        tx.sanitize()?;
+        tx.sanitize(require_static_program_ids)?;
 
         let message_hash = match message_hash.into() {
             MessageHash::Compute => tx.message.hash(),

sdk/src/transaction/versioned.rs

@@ -6,7 +6,7 @@ use {
     crate::{
         hash::Hash,
         message::VersionedMessage,
-        sanitize::{Sanitize, SanitizeError},
+        sanitize::SanitizeError,
         short_vec,
         signature::Signature,
         signer::SignerError,
@@ -46,27 +46,6 @@ pub struct VersionedTransaction {
     pub message: VersionedMessage,
 }
 
-impl Sanitize for VersionedTransaction {
-    fn sanitize(&self) -> std::result::Result<(), SanitizeError> {
-        self.message.sanitize()?;
-
-        let num_required_signatures = usize::from(self.message.header().num_required_signatures);
-        match num_required_signatures.cmp(&self.signatures.len()) {
-            Ordering::Greater => Err(SanitizeError::IndexOutOfBounds),
-            Ordering::Less => Err(SanitizeError::InvalidValue),
-            Ordering::Equal => Ok(()),
-        }?;
-
-        // Signatures are verified before message keys are loaded so all signers
-        // must correspond to static account keys.
-        if self.signatures.len() > self.message.static_account_keys().len() {
-            return Err(SanitizeError::IndexOutOfBounds);
-        }
-
-        Ok(())
-    }
-}
-
 impl From<Transaction> for VersionedTransaction {
     fn from(transaction: Transaction) -> Self {
         Self {
@@ -111,6 +90,28 @@ impl VersionedTransaction {
         })
     }
 
+    pub fn sanitize(
+        &self,
+        require_static_program_ids: bool,
+    ) -> std::result::Result<(), SanitizeError> {
+        self.message.sanitize(require_static_program_ids)?;
+
+        let num_required_signatures = usize::from(self.message.header().num_required_signatures);
+        match num_required_signatures.cmp(&self.signatures.len()) {
+            Ordering::Greater => Err(SanitizeError::IndexOutOfBounds),
+            Ordering::Less => Err(SanitizeError::InvalidValue),
+            Ordering::Equal => Ok(()),
+        }?;
+
+        // Signatures are verified before message keys are loaded so all signers
+        // must correspond to static account keys.
+        if self.signatures.len() > self.message.static_account_keys().len() {
+            return Err(SanitizeError::IndexOutOfBounds);
+        }
+
+        Ok(())
+    }
+
     /// Returns the version of the transaction
     pub fn version(&self) -> TransactionVersion {
         match self.message {

transaction-status/src/lib.rs

@@ -16,7 +16,6 @@ use {
             AccountKeys, Message, MessageHeader, VersionedMessage,
         },
         pubkey::Pubkey,
-        sanitize::Sanitize,
         signature::Signature,
         transaction::{
             Result as TransactionResult, Transaction, TransactionError, TransactionVersion,
@@ -867,7 +866,13 @@ impl EncodedTransaction {
                 .and_then(|bytes| bincode::deserialize(&bytes).ok()),
         };
 
-        transaction.filter(|transaction| transaction.sanitize().is_ok())
+        transaction.filter(|transaction| {
+            transaction
+                .sanitize(
+                    true, // require_static_program_ids
+                )
+                .is_ok()
+        })
     }
 }