Don't use caller passed executable account (#14276) (#14501)

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),

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 => {

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<Rc<RefCell<Account>>>, Vec<AccountReferences<'a>>);
+type TranslatedAccounts<'a> = (
+    Vec<Rc<RefCell<Account>>>,
+    Vec<Option<AccountReferences<'a>>>,
+);
 
 /// Implemented by language specific data structure translators
 trait SyscallInvokeSigned<'a> {
@@ -737,7 +739,8 @@ trait SyscallInvokeSigned<'a> {
     ) -> Result<Instruction, EbpfError<BPFError>>;
     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 accounts = Vec::with_capacity(account_keys.len());
-        let mut refs = Vec::with_capacity(message.account_keys.len());
+        let mut refs = Vec::with_capacity(account_keys.len());
-        'root: for account_key in message.account_keys.iter() {
+        '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 accounts = Vec::with_capacity(account_keys.len());
-        let mut refs = Vec::with_capacity(message.account_keys.len());
+        let mut refs = Vec::with_capacity(account_keys.len());
-        'root: for account_key in message.account_keys.iter() {
+        '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::<Vec<&KeyedAccount>>();
-    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(
+    let (mut accounts, mut account_refs) = syscall.translate_accounts(
-        &message,
+        &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 {
+        if let Some(account_ref) = account_ref {
-            *account_ref.lamports = account.lamports;
+            if message.is_writable(i) && !account.executable {
-            *account_ref.owner = account.owner;
+                *account_ref.lamports = account.lamports;
-            if account_ref.data.len() != account.data.len() {
+                *account_ref.owner = account.owner;
-                if !account_ref.data.is_empty() {
+                if account_ref.data.len() != account.data.len() {
-                    // Only support for `CreateAccount` at this time.
+                    if !account_ref.data.is_empty() {
-                    // Need a way to limit total realloc size across multiple CPI calls
+                        // Only support for `CreateAccount` at this time.
-                    return Err(
+                        // Need a way to limit total realloc size across multiple CPI calls
-                        SyscallError::InstructionError(InstructionError::InvalidRealloc).into(),
+                        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 {
+                account_ref
-                    return Err(
+                    .data
-                        SyscallError::InstructionError(InstructionError::InvalidRealloc).into(),
+                    .clone_from_slice(&account.data[0..account_ref.data.len()]);
-                    );
-                }
-                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()]);
         }
     }
 

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
+        let program_id_index = message.instructions[0].program_id_index as usize;
-            .program_id(0)
+
-            .ok_or(InstructionError::MissingAccount)?;
+        Ok((message, program_id, program_id_index))
-        Ok((message, id))
     }
 
     /// 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::<Result<Vec<_>, 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![];