From 3eee22266706c464ffebbf8f075b30de72da4b99 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 9 Sep 2021 03:06:02 +0000 Subject: [PATCH] Return error if Transaction contains writable executable or ProgramData accounts (backport #19629) (#19729) * Return error if Transaction contains writable executable or ProgramData accounts (#19629) * Return error if Transaction locks an executable as writable * Return error if a ProgramData account is writable but the upgradable loader isn't present * Remove unreachable clause * Fixup bpf tests * Review comments * Add new TransactionError * Disallow writes to any upgradeable-loader account when loader not present; remove is_upgradeable_loader_present exception for all other executables (cherry picked from commit 38bbb779890c363cb4b7bcf7b19c1348fe41320c) # Conflicts: # programs/bpf/tests/programs.rs # runtime/src/accounts.rs # runtime/src/bank.rs # sdk/src/transaction.rs # storage-proto/proto/transaction_by_addr.proto # storage-proto/src/convert.rs * Fix conflicts Co-authored-by: Tyera Eulberg Co-authored-by: Tyera Eulberg --- programs/bpf/tests/programs.rs | 16 +- runtime/src/accounts.rs | 339 ++++++++++++++++-- runtime/src/accounts_db.rs | 1 + runtime/src/bank.rs | 8 +- sdk/src/transaction.rs | 13 + storage-proto/proto/transaction_by_addr.proto | 3 + storage-proto/src/convert.rs | 12 + 7 files changed, 362 insertions(+), 30 deletions(-) diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 1dcb229e9a..1c80aaeed6 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1842,10 +1842,10 @@ fn test_program_bpf_invoke_upgradeable_via_cpi() { invoke_and_return, &[0], vec![ - AccountMeta::new(program_id, false), - AccountMeta::new(program_id, false), - AccountMeta::new(clock::id(), false), - AccountMeta::new(fees::id(), false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(clock::id(), false), + AccountMeta::new_readonly(fees::id(), false), ], ); @@ -2022,10 +2022,10 @@ fn test_program_bpf_upgrade_via_cpi() { invoke_and_return, &[0], vec![ - AccountMeta::new(program_id, false), - AccountMeta::new(program_id, false), - AccountMeta::new(clock::id(), false), - AccountMeta::new(fees::id(), false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(clock::id(), false), + AccountMeta::new_readonly(fees::id(), false), ], ); diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index b81efa7219..d5c8dbc123 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -206,6 +206,7 @@ impl Accounts { let mut rent_debits = RentDebits::default(); let demote_program_write_locks = feature_set.is_active(&feature_set::demote_program_write_locks::id()); + let is_upgradeable_loader_present = is_upgradeable_loader_present(message); for (i, key) in message.account_keys.iter().enumerate() { let account = if message.is_non_loader_key(key, i) { @@ -216,9 +217,6 @@ impl Accounts { if solana_sdk::sysvar::instructions::check_id(key) && feature_set.is_active(&feature_set::instructions_sysvar_enabled::id()) { - if message.is_writable(i, demote_program_write_locks) { - return Err(TransactionError::InvalidAccountIndex); - } Self::construct_instructions_account(message, demote_program_write_locks) } else { let (account, rent) = self @@ -235,26 +233,42 @@ impl Accounts { }) .unwrap_or_default(); - if account.executable && bpf_loader_upgradeable::check_id(&account.owner) { - // The upgradeable loader requires the derived ProgramData account - if let Ok(UpgradeableLoaderState::Program { - programdata_address, - }) = account.state() + if bpf_loader_upgradeable::check_id(&account.owner) { + if demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) + && !is_upgradeable_loader_present { - if let Some(account) = self - .accounts_db - .load(ancestors, &programdata_address) - .map(|(account, _)| account) - { - account_deps.push((programdata_address, account)); - } else { - error_counters.account_not_found += 1; - return Err(TransactionError::ProgramAccountNotFound); - } - } else { - error_counters.invalid_program_for_execution += 1; - return Err(TransactionError::InvalidProgramForExecution); + error_counters.invalid_writable_account += 1; + return Err(TransactionError::InvalidWritableAccount); } + + if account.executable { + // The upgradeable loader requires the derived ProgramData account + if let Ok(UpgradeableLoaderState::Program { + programdata_address, + }) = account.state() + { + if let Some(account) = self + .accounts_db + .load(ancestors, &programdata_address) + .map(|(account, _)| account) + { + account_deps.push((programdata_address, account)); + } else { + error_counters.account_not_found += 1; + return Err(TransactionError::ProgramAccountNotFound); + } + } else { + error_counters.invalid_program_for_execution += 1; + return Err(TransactionError::InvalidProgramForExecution); + } + } + } else if account.executable + && demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) + { + error_counters.invalid_writable_account += 1; + return Err(TransactionError::InvalidWritableAccount); } tx_rent += rent; @@ -1049,6 +1063,13 @@ pub fn prepare_if_nonce_account( false } +fn is_upgradeable_loader_present(message: &Message) -> bool { + message + .account_keys + .iter() + .any(|&key| key == bpf_loader_upgradeable::id()) +} + pub fn create_test_accounts( accounts: &Accounts, pubkeys: &mut Vec, @@ -1668,6 +1689,282 @@ mod tests { assert_eq!(loaded, vec![]); } + #[test] + fn test_load_accounts_executable_with_write_lock() { + let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new(); + let mut error_counters = ErrorCounters::default(); + + let keypair = Keypair::new(); + let key0 = keypair.pubkey(); + let key1 = Pubkey::new(&[5u8; 32]); + let key2 = Pubkey::new(&[6u8; 32]); + + let mut account = AccountSharedData::new(1, 0, &Pubkey::default()); + account.set_rent_epoch(1); + accounts.push((key0, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key1, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key2, account)); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let mut message = Message::new_with_compiled_instructions( + 1, + 0, + 1, // only one executable marked as readonly + vec![key0, key1, key2], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + assert_eq!( + loaded_accounts[0], + (Err(TransactionError::InvalidWritableAccount), None) + ); + + // Mark executables as readonly + message.account_keys = vec![key0, key1, key2]; // revert key change + message.header.num_readonly_unsigned_accounts = 2; // mark both executables as readonly + let tx = Transaction::new(&[&keypair], message, Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!( + result.accounts[..2], + accounts + .iter() + .map(|(_, account)| account) + .cloned() + .collect::>()[..2] + ); + assert_eq!(result.loaders[0], vec![accounts[2].clone()]); + } + + #[test] + fn test_load_accounts_upgradeable_with_write_lock() { + let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new(); + let mut error_counters = ErrorCounters::default(); + + let keypair = Keypair::new(); + let key0 = keypair.pubkey(); + let key1 = Pubkey::new(&[5u8; 32]); + let key2 = Pubkey::new(&[6u8; 32]); + let programdata_key1 = Pubkey::new(&[7u8; 32]); + let programdata_key2 = Pubkey::new(&[8u8; 32]); + + let mut account = AccountSharedData::new(1, 0, &Pubkey::default()); + account.set_rent_epoch(1); + accounts.push((key0, account)); + + let program_data = UpgradeableLoaderState::ProgramData { + slot: 42, + upgrade_authority_address: None, + }; + + let program = UpgradeableLoaderState::Program { + programdata_address: programdata_key1, + }; + let mut account = + AccountSharedData::new_data(40, &program, &bpf_loader_upgradeable::id()).unwrap(); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key1, account)); + let mut account = + AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap(); + account.set_rent_epoch(1); + accounts.push((programdata_key1, account)); + + let program = UpgradeableLoaderState::Program { + programdata_address: programdata_key2, + }; + let mut account = + AccountSharedData::new_data(40, &program, &bpf_loader_upgradeable::id()).unwrap(); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key2, account)); + let mut account = + AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap(); + account.set_rent_epoch(1); + accounts.push((programdata_key2, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((bpf_loader_upgradeable::id(), account)); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let mut message = Message::new_with_compiled_instructions( + 1, + 0, + 1, // only one executable marked as readonly + vec![key0, key1, key2], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + assert_eq!( + loaded_accounts[0], + (Err(TransactionError::InvalidWritableAccount), None) + ); + + // Solution 1: include bpf_loader_upgradeable account + message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()]; + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!( + result.accounts[..2], + accounts + .iter() + .map(|(_, account)| account) + .cloned() + .collect::>()[..2] + ); + assert_eq!(result.loaders[0], vec![accounts[5].clone()]); + + // Solution 2: mark programdata as readonly + message.account_keys = vec![key0, key1, key2]; // revert key change + message.header.num_readonly_unsigned_accounts = 2; // mark both executables as readonly + let tx = Transaction::new(&[&keypair], message, Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!( + result.accounts[..2], + accounts + .iter() + .map(|(_, account)| account) + .cloned() + .collect::>()[..2] + ); + assert_eq!( + result.loaders[0], + vec![ + accounts[5].clone(), + accounts[3].clone(), + accounts[4].clone() + ] + ); + } + + #[test] + fn test_load_accounts_programdata_with_write_lock() { + let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new(); + let mut error_counters = ErrorCounters::default(); + + let keypair = Keypair::new(); + let key0 = keypair.pubkey(); + let key1 = Pubkey::new(&[5u8; 32]); + let key2 = Pubkey::new(&[6u8; 32]); + + let mut account = AccountSharedData::new(1, 0, &Pubkey::default()); + account.set_rent_epoch(1); + accounts.push((key0, account)); + + let program_data = UpgradeableLoaderState::ProgramData { + slot: 42, + upgrade_authority_address: None, + }; + let mut account = + AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap(); + account.set_rent_epoch(1); + accounts.push((key1, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key2, account)); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let mut message = Message::new_with_compiled_instructions( + 1, + 0, + 1, // only the program marked as readonly + vec![key0, key1, key2], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + assert_eq!( + loaded_accounts[0], + (Err(TransactionError::InvalidWritableAccount), None) + ); + + // Solution 1: include bpf_loader_upgradeable account + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable + account.set_executable(true); + account.set_rent_epoch(1); + let accounts_with_upgradeable_loader = vec![ + accounts[0].clone(), + accounts[1].clone(), + (bpf_loader_upgradeable::id(), account), + ]; + message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()]; + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = + load_accounts(tx, &accounts_with_upgradeable_loader, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!( + result.accounts[..2], + accounts_with_upgradeable_loader + .iter() + .map(|(_, account)| account) + .cloned() + .collect::>()[..2] + ); + assert_eq!( + result.loaders[0], + vec![accounts_with_upgradeable_loader[2].clone()] + ); + + // Solution 2: mark programdata as readonly + message.account_keys = vec![key0, key1, key2]; // revert key change + message.header.num_readonly_unsigned_accounts = 2; // extend readonly set to include programdata + let tx = Transaction::new(&[&keypair], message, Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!( + result.accounts[..2], + accounts + .iter() + .map(|(_, account)| account) + .cloned() + .collect::>()[..2] + ); + assert_eq!(result.loaders[0], vec![accounts[2].clone()]); + } + #[test] fn test_accounts_account_not_found() { let accounts = Accounts::new_with_config( diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index eecbfd686f..29210a50ed 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -122,6 +122,7 @@ pub struct ErrorCounters { pub invalid_account_index: usize, pub invalid_program_for_execution: usize, pub not_allowed_during_cluster_maintenance: usize, + pub invalid_writable_account: usize, } #[derive(Default, Debug, PartialEq, Clone)] diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 867737bf2f..95197c9e88 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -144,7 +144,7 @@ impl ExecuteTimings { } type BankStatusCache = StatusCache>; -#[frozen_abi(digest = "HhY4tMP5KZU9fw9VLpMMUikfvNVCLksocZBUKjt8ZjYH")] +#[frozen_abi(digest = "5Br3PNyyX1L7XoS4jYLt5JTeMXowLSsu7v9LhokC8vnq")] pub type BankSlotDelta = SlotDelta>; type TransactionAccountRefCells = Vec>>; type TransactionAccountDepRefCells = Vec<(Pubkey, Rc>)>; @@ -2813,6 +2813,12 @@ impl Bank { error_counters.not_allowed_during_cluster_maintenance ); } + if 0 != error_counters.invalid_writable_account { + inc_new_counter_info!( + "bank-process_transactions-error-invalid_writable_account", + error_counters.invalid_writable_account + ); + } } /// Converts Accounts into RefCell, this involves moving diff --git a/sdk/src/transaction.rs b/sdk/src/transaction.rs index e53e6dc4bf..8ec6d08072 100644 --- a/sdk/src/transaction.rs +++ b/sdk/src/transaction.rs @@ -99,6 +99,19 @@ pub enum TransactionError { /// Transaction processing left an account with an outstanding borrowed reference #[error("Transaction processing left an account with an outstanding borrowed reference")] AccountBorrowOutstanding, + + #[error( + "Transaction could not fit into current block without exceeding the Max Block Cost Limit" + )] + WouldExceedMaxBlockCostLimit, + + /// Transaction version is unsupported + #[error("Transaction version is unsupported")] + UnsupportedVersion, + + /// Transaction loads a writable account that cannot be written + #[error("Transaction loads a writable account that cannot be written")] + InvalidWritableAccount, } pub type Result = result::Result; diff --git a/storage-proto/proto/transaction_by_addr.proto b/storage-proto/proto/transaction_by_addr.proto index 44e300ce34..0278d81d7c 100644 --- a/storage-proto/proto/transaction_by_addr.proto +++ b/storage-proto/proto/transaction_by_addr.proto @@ -41,6 +41,9 @@ enum TransactionErrorType { SANITIZE_FAILURE = 14; CLUSTER_MAINTENANCE = 15; ACCOUNT_BORROW_OUTSTANDING_TX = 16; + WOULD_EXCEED_MAX_BLOCK_COST_LIMIT = 17; + UNSUPPORTED_VERSION = 18; + INVALID_WRITABLE_ACCOUNT = 19; } message InstructionError { diff --git a/storage-proto/src/convert.rs b/storage-proto/src/convert.rs index a2304bec7b..b64b236b36 100644 --- a/storage-proto/src/convert.rs +++ b/storage-proto/src/convert.rs @@ -548,6 +548,9 @@ impl TryFrom for TransactionError { 14 => TransactionError::SanitizeFailure, 15 => TransactionError::ClusterMaintenance, 16 => TransactionError::AccountBorrowOutstanding, + 17 => TransactionError::WouldExceedMaxBlockCostLimit, + 18 => TransactionError::UnsupportedVersion, + 19 => TransactionError::InvalidWritableAccount, _ => return Err("Invalid TransactionError"), }) } @@ -606,6 +609,15 @@ impl From for tx_by_addr::TransactionError { TransactionError::AccountBorrowOutstanding => { tx_by_addr::TransactionErrorType::AccountBorrowOutstandingTx } + TransactionError::WouldExceedMaxBlockCostLimit => { + tx_by_addr::TransactionErrorType::WouldExceedMaxBlockCostLimit + } + TransactionError::UnsupportedVersion => { + tx_by_addr::TransactionErrorType::UnsupportedVersion + } + TransactionError::InvalidWritableAccount => { + tx_by_addr::TransactionErrorType::InvalidWritableAccount + } } as i32, instruction_error: match transaction_error { TransactionError::InstructionError(index, ref instruction_error) => {