From d815fe37c8c0bbc9378fa93fed2c157d73971d0b Mon Sep 17 00:00:00 2001 From: Jack May Date: Fri, 8 Jan 2021 17:21:09 -0800 Subject: [PATCH] Don't use caller passed executable account (#14276) (#14501) --- programs/bpf/c/src/invoke/invoke.c | 3 +- programs/bpf/rust/invoke/src/lib.rs | 6 +- programs/bpf_loader/src/syscalls.rs | 109 ++++++++++++++++------------ runtime/src/message_processor.rs | 59 ++------------- 4 files changed, 77 insertions(+), 100 deletions(-) diff --git a/programs/bpf/c/src/invoke/invoke.c b/programs/bpf/c/src/invoke/invoke.c index df6062c4ae..80181c8bd1 100644 --- a/programs/bpf/c/src/invoke/invoke.c +++ b/programs/bpf/c/src/invoke/invoke.c @@ -293,7 +293,8 @@ extern uint64_t entrypoint(const uint8_t *input) { case TEST_EMPTY_ACCOUNTS_SLICE: { sol_log("Empty accounts slice "); - SolAccountMeta arguments[] = {}; + SolAccountMeta arguments[] = { + {accounts[INVOKED_ARGUMENT_INDEX].key, false, false}}; uint8_t data[] = {}; const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, arguments, SOL_ARRAY_SIZE(arguments), diff --git a/programs/bpf/rust/invoke/src/lib.rs b/programs/bpf/rust/invoke/src/lib.rs index 953182f103..b755d40263 100644 --- a/programs/bpf/rust/invoke/src/lib.rs +++ b/programs/bpf/rust/invoke/src/lib.rs @@ -377,7 +377,11 @@ fn process_instruction( } TEST_EMPTY_ACCOUNTS_SLICE => { msg!("Empty accounts slice"); - let instruction = create_instruction(*accounts[INVOKED_PROGRAM_INDEX].key, &[], vec![]); + let instruction = create_instruction( + *accounts[INVOKED_PROGRAM_INDEX].key, + &[(accounts[INVOKED_ARGUMENT_INDEX].key, false, false)], + vec![], + ); invoke(&instruction, &[])?; } TEST_CAP_SEEDS => { diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index 6fea325831..716123514c 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -23,7 +23,6 @@ use solana_sdk::{ hash::{Hasher, HASH_BYTES}, instruction::{AccountMeta, Instruction, InstructionError}, keyed_account::KeyedAccount, - message::Message, native_loader, process_instruction::{stable_log, ComputeMeter, InvokeContext, Logger}, program_error::ProgramError, @@ -723,7 +722,10 @@ struct AccountReferences<'a> { ref_to_len_in_vm: &'a mut u64, serialized_len_ptr: &'a mut u64, } -type TranslatedAccounts<'a> = (Vec>>, Vec>); +type TranslatedAccounts<'a> = ( + Vec>>, + Vec>>, +); /// Implemented by language specific data structure translators trait SyscallInvokeSigned<'a> { @@ -737,7 +739,8 @@ trait SyscallInvokeSigned<'a> { ) -> Result>; fn translate_accounts( &self, - message: &Message, + account_keys: &[Pubkey], + program_account_index: usize, account_infos_addr: u64, account_infos_len: usize, ro_regions: &[MemoryRegion], @@ -801,7 +804,8 @@ impl<'a> SyscallInvokeSigned<'a> for SyscallInvokeSignedRust<'a> { fn translate_accounts( &self, - message: &Message, + account_keys: &[Pubkey], + program_account_index: usize, account_infos_addr: u64, account_infos_len: usize, ro_regions: &[MemoryRegion], @@ -819,9 +823,13 @@ impl<'a> SyscallInvokeSigned<'a> for SyscallInvokeSignedRust<'a> { &[] }; - let mut accounts = Vec::with_capacity(message.account_keys.len()); - let mut refs = Vec::with_capacity(message.account_keys.len()); - 'root: for account_key in message.account_keys.iter() { + let mut accounts = Vec::with_capacity(account_keys.len()); + let mut refs = Vec::with_capacity(account_keys.len()); + 'root: for (i, account_key) in account_keys.iter().enumerate() { + if i == program_account_index { + // Don't look for caller passed executable, runtime already has it + continue 'root; + } for account_info in account_infos.iter() { let key = translate_type!( Pubkey, @@ -890,14 +898,14 @@ impl<'a> SyscallInvokeSigned<'a> for SyscallInvokeSignedRust<'a> { owner: *owner, rent_epoch: account_info.rent_epoch, }))); - refs.push(AccountReferences { + refs.push(Some(AccountReferences { lamports, owner, data, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr, - }); + })); continue 'root; } } @@ -1095,7 +1103,8 @@ impl<'a> SyscallInvokeSigned<'a> for SyscallInvokeSignedC<'a> { fn translate_accounts( &self, - message: &Message, + account_keys: &[Pubkey], + program_account_index: usize, account_infos_addr: u64, account_infos_len: usize, ro_regions: &[MemoryRegion], @@ -1108,9 +1117,13 @@ impl<'a> SyscallInvokeSigned<'a> for SyscallInvokeSignedC<'a> { ro_regions, self.loader_id )?; - let mut accounts = Vec::with_capacity(message.account_keys.len()); - let mut refs = Vec::with_capacity(message.account_keys.len()); - 'root: for account_key in message.account_keys.iter() { + let mut accounts = Vec::with_capacity(account_keys.len()); + let mut refs = Vec::with_capacity(account_keys.len()); + 'root: for (i, account_key) in account_keys.iter().enumerate() { + if i == program_account_index { + // Don't look for caller passed executable, runtime already has it + continue 'root; + } for account_info in account_infos.iter() { let key = translate_type!(Pubkey, account_info.key_addr, ro_regions, self.loader_id)?; @@ -1159,14 +1172,14 @@ impl<'a> SyscallInvokeSigned<'a> for SyscallInvokeSignedC<'a> { owner: *owner, rent_epoch: account_info.rent_epoch, }))); - refs.push(AccountReferences { + refs.push(Some(AccountReferences { lamports, owner, data, vm_data_addr, ref_to_len_in_vm, serialized_len_ptr, - }); + })); continue 'root; } } @@ -1311,14 +1324,15 @@ fn call<'a>( .get_callers_keyed_accounts() .iter() .collect::>(); - let (message, callee_program_id) = + let (message, callee_program_id, program_id_index) = MessageProcessor::create_message(&instruction, &keyed_account_refs, &signers) .map_err(SyscallError::InstructionError)?; if invoke_context.is_feature_active(&limit_cpi_loader_invoke::id()) { check_authorized_program(&callee_program_id)?; } - let (accounts, account_refs) = syscall.translate_accounts( - &message, + let (mut accounts, mut account_refs) = syscall.translate_accounts( + &message.account_keys, + program_id_index, account_infos_addr, account_infos_len as usize, ro_regions, @@ -1359,6 +1373,8 @@ fn call<'a>( } else { None }; + accounts.insert(program_id_index, Rc::new(program_account.clone())); + account_refs.insert(program_id_index, None); let mut executable_accounts = vec![(callee_program_id, program_account)]; if let Some(programdata) = programdata_executable { executable_accounts.push(programdata); @@ -1379,37 +1395,40 @@ fn call<'a>( } // Copy results back to caller - for (i, (account, account_ref)) in accounts.iter().zip(account_refs).enumerate() { let account = account.borrow(); - if message.is_writable(i) && !account.executable { - *account_ref.lamports = account.lamports; - *account_ref.owner = account.owner; - if account_ref.data.len() != account.data.len() { - if !account_ref.data.is_empty() { - // Only support for `CreateAccount` at this time. - // Need a way to limit total realloc size across multiple CPI calls - return Err( - SyscallError::InstructionError(InstructionError::InvalidRealloc).into(), - ); + if let Some(account_ref) = account_ref { + if message.is_writable(i) && !account.executable { + *account_ref.lamports = account.lamports; + *account_ref.owner = account.owner; + if account_ref.data.len() != account.data.len() { + if !account_ref.data.is_empty() { + // Only support for `CreateAccount` at this time. + // Need a way to limit total realloc size across multiple CPI calls + return Err(SyscallError::InstructionError( + InstructionError::InvalidRealloc, + ) + .into()); + } + if account.data.len() > account_ref.data.len() + MAX_PERMITTED_DATA_INCREASE { + return Err(SyscallError::InstructionError( + InstructionError::InvalidRealloc, + ) + .into()); + } + let _ = translate!( + account_ref.vm_data_addr, + account.data.len() as u64, + rw_regions, + self.loader_id + )?; + *account_ref.ref_to_len_in_vm = account.data.len() as u64; + *account_ref.serialized_len_ptr = account.data.len() as u64; } - if account.data.len() > account_ref.data.len() + MAX_PERMITTED_DATA_INCREASE { - return Err( - SyscallError::InstructionError(InstructionError::InvalidRealloc).into(), - ); - } - let _ = translate!( - account_ref.vm_data_addr, - account.data.len() as u64, - rw_regions, - self.loader_id - )?; - *account_ref.ref_to_len_in_vm = account.data.len() as u64; - *account_ref.serialized_len_ptr = account.data.len() as u64; + account_ref + .data + .clone_from_slice(&account.data[0..account_ref.data.len()]); } - account_ref - .data - .clone_from_slice(&account.data[0..account_ref.data.len()]); } } diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 85b33a6270..7099911a31 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -510,58 +510,11 @@ impl MessageProcessor { Err(InstructionError::UnsupportedProgramId) } - /// Verify instruction against the caller - pub fn verify_instruction( - keyed_accounts: &[&KeyedAccount], - instruction: &Instruction, - signers: &[Pubkey], - ) -> Result<(), InstructionError> { - // Check for privilege escalation - for account in instruction.accounts.iter() { - let keyed_account = keyed_accounts - .iter() - .find_map(|keyed_account| { - if &account.pubkey == keyed_account.unsigned_key() { - Some(keyed_account) - } else { - None - } - }) - .ok_or(InstructionError::MissingAccount)?; - // Readonly account cannot become writable - if account.is_writable && !keyed_account.is_writable() { - return Err(InstructionError::PrivilegeEscalation); - } - - if account.is_signer && // If message indicates account is signed - !( // one of the following needs to be true: - keyed_account.signer_key().is_some() // Signed in the parent instruction - || signers.contains(&account.pubkey) // Signed by the program - ) { - return Err(InstructionError::PrivilegeEscalation); - } - } - - // validate the caller has access to the program account - let _ = keyed_accounts - .iter() - .find_map(|keyed_account| { - if &instruction.program_id == keyed_account.unsigned_key() { - Some(keyed_account) - } else { - None - } - }) - .ok_or(InstructionError::MissingAccount)?; - - Ok(()) - } - pub fn create_message( instruction: &Instruction, keyed_accounts: &[&KeyedAccount], signers: &[Pubkey], - ) -> Result<(Message, Pubkey), InstructionError> { + ) -> Result<(Message, Pubkey, usize), InstructionError> { // Check for privilege escalation for account in instruction.accounts.iter() { let keyed_account = keyed_accounts @@ -589,6 +542,7 @@ impl MessageProcessor { } // validate the caller has access to the program account + let program_id = instruction.program_id; let _ = keyed_accounts .iter() .find_map(|keyed_account| { @@ -601,10 +555,9 @@ impl MessageProcessor { .ok_or(InstructionError::MissingAccount)?; let message = Message::new(&[instruction.clone()], None); - let id = *message - .program_id(0) - .ok_or(InstructionError::MissingAccount)?; - Ok((message, id)) + let program_id_index = message.instructions[0].program_id_index as usize; + + Ok((message, program_id, program_id_index)) } /// Entrypoint for a cross-program invocation from a native program @@ -622,7 +575,7 @@ impl MessageProcessor { .iter() .map(|seeds| Pubkey::create_program_address(&seeds, caller_program_id)) .collect::, solana_sdk::pubkey::PubkeyError>>()?; - let (message, callee_program_id) = + let (message, callee_program_id, _) = Self::create_message(&instruction, &keyed_accounts, &signers)?; let mut accounts = vec![]; let mut account_refs = vec![];