From 082502d4f35632ee6776626cc1859502dba09bf1 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Sat, 7 May 2022 03:19:50 +0800 Subject: [PATCH] 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 | 2 + entry/benches/entry_sigverify.rs | 2 + entry/src/entry.rs | 1 + ledger-tool/src/main.rs | 2 + ledger/src/blockstore.rs | 12 +- rpc/src/rpc.rs | 10 +- rpc/src/transaction_status_service.rs | 1 + runtime/src/bank.rs | 20 ++- runtime/src/cost_tracker.rs | 1 + sdk/program/src/message/versions/mod.rs | 16 +- sdk/program/src/message/versions/v0/mod.rs | 182 ++++++++++++++++----- sdk/src/feature_set.rs | 7 +- sdk/src/transaction/sanitized.rs | 3 +- sdk/src/transaction/versioned.rs | 45 ++--- transaction-status/src/lib.rs | 9 +- 15 files changed, 227 insertions(+), 86 deletions(-) diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index da85044b86..0373079f1b 100644 --- a/core/src/banking_stage.rs +++ b/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(); diff --git a/entry/benches/entry_sigverify.rs b/entry/benches/entry_sigverify.rs index b3a1b7b5cd..1f4abf9182 100644 --- a/entry/benches/entry_sigverify.rs +++ b/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 ) }?; diff --git a/entry/src/entry.rs b/entry/src/entry.rs index ad7abbd4d8..f74b1e35a1 100644 --- a/entry/src/entry.rs +++ b/entry/src/entry.rs @@ -1014,6 +1014,7 @@ mod tests { message_hash, None, SimpleAddressLoader::Disabled, + true, // require_static_program_ids ) }?; diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index be0f195fab..ff14ce7b46 100644 --- a/ledger-tool/src/main.rs +++ b/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); diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs index 79d433d541..a60367666e 100644 --- a/ledger/src/blockstore.rs +++ b/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: {:?}, \ diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index 011d2c8d38..e72c3551b6 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -4291,8 +4291,14 @@ fn sanitize_transaction( transaction: VersionedTransaction, address_loader: impl AddressLoader, ) -> Result { - 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) -> Arc> { diff --git a/rpc/src/transaction_status_service.rs b/rpc/src/transaction_status_service.rs index 35b0fed007..3607217e8c 100644 --- a/rpc/src/transaction_status_service.rs +++ b/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(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 637f087fba..a3ca553ec2 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3800,7 +3800,16 @@ impl Bank { pub fn prepare_entry_batch(&self, txs: Vec) -> Result { 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::>>()?; 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 diff --git a/runtime/src/cost_tracker.rs b/runtime/src/cost_tracker.rs index 3af6f31ae8..05f35ce9d1 100644 --- a/runtime/src/cost_tracker.rs +++ b/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); diff --git a/sdk/program/src/message/versions/mod.rs b/sdk/program/src/message/versions/mod.rs index 28cd284b71..05d93c96c6 100644 --- a/sdk/program/src/message/versions/mod.rs +++ b/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(&self, serializer: S) -> Result where diff --git a/sdk/program/src/message/versions/v0/mod.rs b/sdk/program/src/message/versions/v0/mod.rs index 81c0461180..e7cfa5a5c6 100644 --- a/sdk/program/src/message/versions/v0/mod.rs +++ b/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, } -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()); } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 04187b29c5..b1ab0041c2 100644 --- a/sdk/src/feature_set.rs +++ b/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 = [ @@ -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() diff --git a/sdk/src/transaction/sanitized.rs b/sdk/src/transaction/sanitized.rs index ce90fbd0b2..fb250f11a6 100644 --- a/sdk/src/transaction/sanitized.rs +++ b/sdk/src/transaction/sanitized.rs @@ -80,8 +80,9 @@ impl SanitizedTransaction { message_hash: impl Into, is_simple_vote_tx: Option, address_loader: impl AddressLoader, + require_static_program_ids: bool, ) -> Result { - tx.sanitize()?; + tx.sanitize(require_static_program_ids)?; let message_hash = match message_hash.into() { MessageHash::Compute => tx.message.hash(), diff --git a/sdk/src/transaction/versioned.rs b/sdk/src/transaction/versioned.rs index 650f0810eb..a735cc5a35 100644 --- a/sdk/src/transaction/versioned.rs +++ b/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 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 { diff --git a/transaction-status/src/lib.rs b/transaction-status/src/lib.rs index 545acdc4fb..3b690d4f6d 100644 --- a/transaction-status/src/lib.rs +++ b/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() + }) } }