From cfb28a1b2e828a7c6d47a5ac3f314f571b9ccabc Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 30 Dec 2020 20:10:13 +0000 Subject: [PATCH] Prevent bpf loader impersonators (bp #14278) (#14353) * Prevent bpf loader impersonators (#14278) (cherry picked from commit ee0a80a092fa2f34b9c8ec097159ec20c6476a97) # Conflicts: # programs/bpf_loader/src/lib.rs # runtime/src/message_processor.rs * resolve conflicts Co-authored-by: Jack May --- programs/bpf/tests/programs.rs | 42 ++++++++++ programs/bpf_loader/src/lib.rs | 128 ++++++++++++++++++------------- runtime/src/message_processor.rs | 24 ++++-- 3 files changed, 132 insertions(+), 62 deletions(-) diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 5d98e232d4..9277fa28f6 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1570,3 +1570,45 @@ fn test_program_bpf_invoke_upgradeable_via_cpi() { TransactionError::InstructionError(0, InstructionError::Custom(42)) ); } + +#[test] +#[cfg(any(feature = "bpf_c", feature = "bpf_rust"))] +fn test_program_bpf_disguised_as_bpf_loader() { + solana_logger::setup(); + + let mut programs = Vec::new(); + #[cfg(feature = "bpf_c")] + { + programs.extend_from_slice(&[("noop")]); + } + #[cfg(feature = "bpf_rust")] + { + programs.extend_from_slice(&[("noop")]); + } + + for program in programs.iter() { + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config(50); + let mut bank = Bank::new(&genesis_config); + let (name, id, entrypoint) = solana_bpf_loader_deprecated_program!(); + bank.add_builtin(&name, id, entrypoint); + let bank_client = BankClient::new(bank); + + let program_id = load_bpf_program( + &bank_client, + &bpf_loader_deprecated::id(), + &mint_keypair, + program, + ); + let account_metas = vec![AccountMeta::new_readonly(program_id, false)]; + let instruction = Instruction::new(bpf_loader_deprecated::id(), &1u8, account_metas); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + assert_eq!( + result.unwrap_err().unwrap(), + TransactionError::InstructionError(0, InstructionError::IncorrectProgramId) + ); + } +} diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 6300e19192..2a98e3dce3 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -126,6 +126,12 @@ fn write_program_data( Ok(()) } +fn check_loader_id(id: &Pubkey) -> bool { + bpf_loader::check_id(id) + || bpf_loader_deprecated::check_id(id) + || bpf_loader_upgradeable::check_id(id) +} + /// Create the BPF virtual machine pub fn create_vm<'a>( loader_id: &'a Pubkey, @@ -154,41 +160,41 @@ pub fn process_instruction( ) -> Result<(), InstructionError> { let logger = invoke_context.get_logger(); - if !(bpf_loader::check_id(program_id) - || bpf_loader_deprecated::check_id(program_id) - || bpf_loader_upgradeable::check_id(program_id)) - { - log!(logger, "Invalid BPF loader id"); - return Err(InstructionError::IncorrectProgramId); - } - let account_iter = &mut keyed_accounts.iter(); let first_account = next_keyed_account(account_iter)?; if first_account.executable()? { - let (program, keyed_accounts, offset) = if bpf_loader_upgradeable::check_id(program_id) { - if let UpgradeableLoaderState::Program { - programdata_address, - } = first_account.state()? - { - let programdata = next_keyed_account(account_iter)?; - if programdata_address != *programdata.unsigned_key() { - log!(logger, "Wrong ProgramData account for this Program account"); - return Err(InstructionError::InvalidArgument); - } - ( - programdata, - &keyed_accounts[1..], - UpgradeableLoaderState::programdata_data_offset()?, - ) - } else { - log!(logger, "Invalid Program account"); - return Err(InstructionError::InvalidAccountData); - } - } else { - (first_account, keyed_accounts, 0) - }; + if first_account.unsigned_key() != program_id { + log!(logger, "Program id mismatch"); + return Err(InstructionError::IncorrectProgramId); + } - if program.owner()? != *program_id { + let (program, keyed_accounts, offset) = + if bpf_loader_upgradeable::check_id(&first_account.owner()?) { + if let UpgradeableLoaderState::Program { + programdata_address, + } = first_account.state()? + { + let programdata = next_keyed_account(account_iter)?; + if programdata_address != *programdata.unsigned_key() { + log!(logger, "Wrong ProgramData account for this Program account"); + return Err(InstructionError::InvalidArgument); + } + ( + programdata, + &keyed_accounts[1..], + UpgradeableLoaderState::programdata_data_offset()?, + ) + } else { + log!(logger, "Invalid Program account"); + return Err(InstructionError::InvalidAccountData); + } + } else { + (first_account, keyed_accounts, 0) + }; + + let loader_id = &program.owner()?; + + if !check_loader_id(loader_id) { log!(logger, "Executable account not owned by the BPF loader"); return Err(InstructionError::IncorrectProgramId); } @@ -201,16 +207,28 @@ pub fn process_instruction( invoke_context, )?, }; - executor.execute(program_id, keyed_accounts, instruction_data, invoke_context)? - } else if bpf_loader_upgradeable::check_id(program_id) { - process_loader_upgradeable_instruction( - program_id, - keyed_accounts, - instruction_data, - invoke_context, - )?; + executor.execute(loader_id, keyed_accounts, instruction_data, invoke_context)? } else { - process_loader_instruction(program_id, keyed_accounts, instruction_data, invoke_context)?; + if !check_loader_id(program_id) { + log!(logger, "Invalid BPF loader id"); + return Err(InstructionError::IncorrectProgramId); + } + + if bpf_loader_upgradeable::check_id(program_id) { + process_loader_upgradeable_instruction( + program_id, + keyed_accounts, + instruction_data, + invoke_context, + )?; + } else { + process_loader_instruction( + program_id, + keyed_accounts, + instruction_data, + invoke_context, + )?; + } } Ok(()) } @@ -568,7 +586,7 @@ impl Debug for BPFExecutor { impl Executor for BPFExecutor { fn execute( &self, - program_id: &Pubkey, + loader_id: &Pubkey, keyed_accounts: &[KeyedAccount], instruction_data: &[u8], invoke_context: &mut dyn InvokeContext, @@ -581,7 +599,7 @@ impl Executor for BPFExecutor { let parameter_accounts = keyed_accounts_iter.as_slice(); let parameter_bytes = serialize_parameters( - program_id, + loader_id, program.unsigned_key(), parameter_accounts, &instruction_data, @@ -589,7 +607,7 @@ impl Executor for BPFExecutor { { let compute_meter = invoke_context.get_compute_meter(); let (mut vm, heap_region) = match create_vm( - program_id, + loader_id, self.executable.as_ref(), ¶meter_accounts, invoke_context, @@ -648,7 +666,7 @@ impl Executor for BPFExecutor { } } } - deserialize_parameters(program_id, parameter_accounts, ¶meter_bytes)?; + deserialize_parameters(loader_id, parameter_accounts, ¶meter_bytes)?; stable_log::program_success(&logger, program.unsigned_key()); Ok(()) } @@ -871,20 +889,20 @@ mod tests { // Case: Empty keyed accounts assert_eq!( Err(InstructionError::NotEnoughAccountKeys), - process_instruction(&bpf_loader::id(), &[], &[], &mut invoke_context) + process_instruction(&program_id, &[], &[], &mut invoke_context) ); // Case: Only a program account assert_eq!( Ok(()), - process_instruction(&bpf_loader::id(), &keyed_accounts, &[], &mut invoke_context) + process_instruction(&program_key, &keyed_accounts, &[], &mut invoke_context) ); - // Case: Account not program + // Case: Account not a program keyed_accounts[0].account.borrow_mut().executable = false; assert_eq!( Err(InstructionError::InvalidInstructionData), - process_instruction(&bpf_loader::id(), &keyed_accounts, &[], &mut invoke_context) + process_instruction(&program_id, &keyed_accounts, &[], &mut invoke_context) ); keyed_accounts[0].account.borrow_mut().executable = true; @@ -893,7 +911,7 @@ mod tests { keyed_accounts.push(KeyedAccount::new(&program_key, false, ¶meter_account)); assert_eq!( Ok(()), - process_instruction(&bpf_loader::id(), &keyed_accounts, &[], &mut invoke_context) + process_instruction(&program_key, &keyed_accounts, &[], &mut invoke_context) ); // Case: limited budget @@ -924,7 +942,7 @@ mod tests { ); assert_eq!( Err(InstructionError::ProgramFailedToComplete), - process_instruction(&bpf_loader::id(), &keyed_accounts, &[], &mut invoke_context) + process_instruction(&program_key, &keyed_accounts, &[], &mut invoke_context) ); // Case: With duplicate accounts @@ -936,7 +954,7 @@ mod tests { assert_eq!( Ok(()), process_instruction( - &bpf_loader::id(), + &program_key, &keyed_accounts, &[], &mut MockInvokeContext::default() @@ -964,7 +982,7 @@ mod tests { assert_eq!( Ok(()), process_instruction( - &bpf_loader_deprecated::id(), + &program_key, &keyed_accounts, &[], &mut MockInvokeContext::default() @@ -980,7 +998,7 @@ mod tests { assert_eq!( Ok(()), process_instruction( - &bpf_loader_deprecated::id(), + &program_key, &keyed_accounts, &[], &mut MockInvokeContext::default() @@ -1008,7 +1026,7 @@ mod tests { assert_eq!( Ok(()), process_instruction( - &bpf_loader::id(), + &program_key, &keyed_accounts, &[], &mut MockInvokeContext::default() @@ -1024,7 +1042,7 @@ mod tests { assert_eq!( Ok(()), process_instruction( - &bpf_loader::id(), + &program_key, &keyed_accounts, &[], &mut MockInvokeContext::default() diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 68bae86d27..85b33a6270 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -466,18 +466,19 @@ impl MessageProcessor { /// This method calls the instruction's program entrypoint method fn process_instruction( &self, + program_id: &Pubkey, keyed_accounts: &[KeyedAccount], instruction_data: &[u8], invoke_context: &mut dyn InvokeContext, ) -> Result<(), InstructionError> { if let Some(root_account) = keyed_accounts.iter().next() { + let root_id = root_account.unsigned_key(); if native_loader::check_id(&root_account.owner()?) { - let root_id = root_account.unsigned_key(); for (id, process_instruction) in &self.programs { if id == root_id { // Call the builtin program return process_instruction( - &root_id, + &program_id, &keyed_accounts[1..], instruction_data, invoke_context, @@ -486,7 +487,7 @@ impl MessageProcessor { } // Call the program via the native loader return self.native_loader.process_instruction( - &native_loader::id(), + program_id, keyed_accounts, instruction_data, invoke_context, @@ -497,7 +498,7 @@ impl MessageProcessor { if id == owner_id { // Call the program via a builtin loader return process_instruction( - &owner_id, + &program_id, keyed_accounts, instruction_data, invoke_context, @@ -703,6 +704,8 @@ impl MessageProcessor { invoke_context: &mut dyn InvokeContext, ) -> Result<(), InstructionError> { if let Some(instruction) = message.instructions.get(0) { + let program_id = instruction.program_id(&message.account_keys); + // Verify the calling program hasn't misbehaved invoke_context.verify_and_update(message, instruction, accounts)?; @@ -711,7 +714,7 @@ impl MessageProcessor { Self::create_keyed_accounts(message, instruction, executable_accounts, accounts)?; // Invoke callee - invoke_context.push(instruction.program_id(&message.account_keys))?; + invoke_context.push(program_id)?; let mut message_processor = MessageProcessor::default(); for (program_id, process_instruction) in invoke_context.get_programs().iter() { @@ -719,6 +722,7 @@ impl MessageProcessor { } let mut result = message_processor.process_instruction( + program_id, &keyed_accounts, &instruction.data, invoke_context, @@ -887,8 +891,9 @@ impl MessageProcessor { } let pre_accounts = Self::create_pre_accounts(message, instruction, accounts); + let program_id = instruction.program_id(&message.account_keys); let mut invoke_context = ThisInvokeContext::new( - instruction.program_id(&message.account_keys), + program_id, rent_collector.rent, pre_accounts, account_deps, @@ -901,7 +906,12 @@ impl MessageProcessor { ); let keyed_accounts = Self::create_keyed_accounts(message, instruction, executable_accounts, accounts)?; - self.process_instruction(&keyed_accounts, &instruction.data, &mut invoke_context)?; + self.process_instruction( + program_id, + &keyed_accounts, + &instruction.data, + &mut invoke_context, + )?; Self::verify( message, instruction,