From 00d7981f644dc5f2a102d73716551b804ca1c57f Mon Sep 17 00:00:00 2001 From: Jack May Date: Mon, 13 Sep 2021 22:57:37 -0700 Subject: [PATCH] Fix native invoke writable privileges (#19750) * Fix native invoke writable privileges * build downstream spl bpf programs for tests --- program-runtime/src/instruction_processor.rs | 84 ++++--- runtime/src/message_processor.rs | 241 ++++++++++++++++++- scripts/build-downstream-projects.sh | 1 + sdk/src/feature_set.rs | 7 +- 4 files changed, 290 insertions(+), 43 deletions(-) diff --git a/program-runtime/src/instruction_processor.rs b/program-runtime/src/instruction_processor.rs index fda80ad83..26f62f93a 100644 --- a/program-runtime/src/instruction_processor.rs +++ b/program-runtime/src/instruction_processor.rs @@ -4,7 +4,7 @@ use solana_sdk::{ account::{AccountSharedData, ReadableAccount, WritableAccount}, account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, - feature_set::demote_program_write_locks, + feature_set::{demote_program_write_locks, fix_write_privs}, ic_logger_msg, ic_msg, instruction::{CompiledInstruction, Instruction, InstructionError}, keyed_account::{keyed_account_at_index, KeyedAccount}, @@ -474,38 +474,64 @@ impl InstructionProcessor { ) = { let invoke_context = invoke_context.borrow(); - // Translate and verify caller's data - let keyed_accounts = invoke_context.get_keyed_accounts()?; - let keyed_accounts = keyed_account_indices + let caller_keyed_accounts = invoke_context.get_keyed_accounts()?; + let callee_keyed_accounts = keyed_account_indices .iter() - .map(|index| keyed_account_at_index(keyed_accounts, *index)) + .map(|index| keyed_account_at_index(caller_keyed_accounts, *index)) .collect::, InstructionError>>()?; - let (message, callee_program_id, _) = - Self::create_message(&instruction, &keyed_accounts, signers, &invoke_context)?; - let keyed_accounts = invoke_context.get_keyed_accounts()?; - let mut caller_write_privileges = keyed_account_indices - .iter() - .map(|index| keyed_accounts[*index].is_writable()) - .collect::>(); - caller_write_privileges.insert(0, false); - let mut accounts = vec![]; - let mut keyed_account_indices_reordered = vec![]; - let keyed_accounts = invoke_context.get_keyed_accounts()?; - 'root: for account_key in message.account_keys.iter() { - for keyed_account_index in keyed_account_indices { - let keyed_account = &keyed_accounts[*keyed_account_index]; - if account_key == keyed_account.unsigned_key() { - accounts.push((*account_key, Rc::new(keyed_account.account.clone()))); - keyed_account_indices_reordered.push(*keyed_account_index); - continue 'root; + let (message, callee_program_id, _) = Self::create_message( + &instruction, + &callee_keyed_accounts, + signers, + &invoke_context, + )?; + let mut keyed_account_indices_reordered = + Vec::with_capacity(message.account_keys.len()); + let mut accounts = Vec::with_capacity(message.account_keys.len()); + let mut caller_write_privileges = Vec::with_capacity(message.account_keys.len()); + + // Translate and verify caller's data + if invoke_context.is_feature_active(&fix_write_privs::id()) { + 'root: for account_key in message.account_keys.iter() { + for keyed_account_index in keyed_account_indices { + let keyed_account = &caller_keyed_accounts[*keyed_account_index]; + if account_key == keyed_account.unsigned_key() { + accounts.push((*account_key, Rc::new(keyed_account.account.clone()))); + caller_write_privileges.push(keyed_account.is_writable()); + keyed_account_indices_reordered.push(*keyed_account_index); + continue 'root; + } } + ic_msg!( + invoke_context, + "Instruction references an unknown account {}", + account_key + ); + return Err(InstructionError::MissingAccount); + } + } else { + let keyed_accounts = invoke_context.get_keyed_accounts()?; + for index in keyed_account_indices.iter() { + caller_write_privileges.push(keyed_accounts[*index].is_writable()); + } + caller_write_privileges.insert(0, false); + let keyed_accounts = invoke_context.get_keyed_accounts()?; + 'root2: for account_key in message.account_keys.iter() { + for keyed_account_index in keyed_account_indices { + let keyed_account = &keyed_accounts[*keyed_account_index]; + if account_key == keyed_account.unsigned_key() { + accounts.push((*account_key, Rc::new(keyed_account.account.clone()))); + keyed_account_indices_reordered.push(*keyed_account_index); + continue 'root2; + } + } + ic_msg!( + invoke_context, + "Instruction references an unknown account {}", + account_key + ); + return Err(InstructionError::MissingAccount); } - ic_msg!( - invoke_context, - "Instruction references an unknown account {}", - account_key - ); - return Err(InstructionError::MissingAccount); } // Process instruction diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index f037dff6f..1357aad11 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -1162,6 +1162,7 @@ mod tests { NoopFail, ModifyOwned, ModifyNotOwned, + ModifyReadonly, } fn mock_process_instruction( @@ -1186,6 +1187,9 @@ mod tests { MockInstruction::ModifyNotOwned => { keyed_accounts[1].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 } + MockInstruction::ModifyReadonly => { + keyed_accounts[2].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 + } } } else { return Err(InstructionError::InvalidInstructionData); @@ -1196,11 +1200,11 @@ mod tests { let caller_program_id = solana_sdk::pubkey::new_rand(); let callee_program_id = solana_sdk::pubkey::new_rand(); - let mut program_account = AccountSharedData::new(1, 0, &native_loader::id()); - program_account.set_executable(true); - let owned_account = AccountSharedData::new(42, 1, &callee_program_id); let not_owned_account = AccountSharedData::new(84, 1, &solana_sdk::pubkey::new_rand()); + let readonly_account = AccountSharedData::new(168, 1, &caller_program_id); + let mut program_account = AccountSharedData::new(1, 0, &native_loader::id()); + program_account.set_executable(true); #[allow(unused_mut)] let accounts = vec![ @@ -1213,26 +1217,29 @@ mod tests { Rc::new(RefCell::new(not_owned_account)), ), ( - callee_program_id, - Rc::new(RefCell::new(program_account.clone())), + solana_sdk::pubkey::new_rand(), + Rc::new(RefCell::new(readonly_account)), ), + (callee_program_id, Rc::new(RefCell::new(program_account))), ]; - let program_indices = vec![2]; + let program_indices = vec![3]; - let compiled_instruction = CompiledInstruction::new(2, &(), vec![0, 1, 2]); let programs: Vec<(_, ProcessInstructionWithContext)> = vec![(callee_program_id, mock_process_instruction)]; let metas = vec![ AccountMeta::new(accounts[0].0, false), AccountMeta::new(accounts[1].0, false), + AccountMeta::new_readonly(accounts[2].0, false), ]; - let instruction = Instruction::new_with_bincode( + let caller_instruction = CompiledInstruction::new(2, &(), vec![0, 1, 2, 3]); + let callee_instruction = Instruction::new_with_bincode( callee_program_id, &MockInstruction::NoopSuccess, metas.clone(), ); - let message = Message::new(&[instruction], None); + let message = Message::new(&[callee_instruction], None); + let feature_set = FeatureSet::all_enabled(); let demote_program_write_locks = feature_set.is_active(&demote_program_write_locks::id()); @@ -1243,7 +1250,7 @@ mod tests { &caller_program_id, Rent::default(), &message, - &compiled_instruction, + &caller_instruction, &program_indices, &accounts, programs.as_slice(), @@ -1280,6 +1287,20 @@ mod tests { ); accounts[0].1.borrow_mut().data_as_mut_slice()[0] = 0; + // readonly account modified by the invoker + accounts[2].1.borrow_mut().data_as_mut_slice()[0] = 1; + assert_eq!( + InstructionProcessor::process_cross_program_instruction( + &message, + &program_indices, + &accounts, + &caller_write_privileges, + &mut invoke_context, + ), + Err(InstructionError::ReadonlyDataModified) + ); + accounts[2].1.borrow_mut().data_as_mut_slice()[0] = 0; + let cases = vec![ (MockInstruction::NoopSuccess, Ok(())), ( @@ -1294,9 +1315,9 @@ mod tests { ]; for case in cases { - let instruction = + let callee_instruction = Instruction::new_with_bincode(callee_program_id, &case.0, metas.clone()); - let message = Message::new(&[instruction], None); + let message = Message::new(&[callee_instruction], None); let ancestors = Ancestors::default(); let blockhash = Hash::default(); @@ -1305,7 +1326,7 @@ mod tests { &caller_program_id, Rent::default(), &message, - &compiled_instruction, + &caller_instruction, &program_indices, &accounts, programs.as_slice(), @@ -1340,4 +1361,198 @@ mod tests { ); } } + + #[test] + fn test_native_invoke() { + #[derive(Debug, Serialize, Deserialize)] + enum MockInstruction { + NoopSuccess, + NoopFail, + ModifyOwned, + ModifyNotOwned, + ModifyReadonly, + } + + fn mock_process_instruction( + program_id: &Pubkey, + data: &[u8], + invoke_context: &mut dyn InvokeContext, + ) -> Result<(), InstructionError> { + let keyed_accounts = invoke_context.get_keyed_accounts()?; + assert_eq!(*program_id, keyed_accounts[0].owner()?); + assert_ne!( + keyed_accounts[1].owner()?, + *keyed_accounts[0].unsigned_key() + ); + + if let Ok(instruction) = bincode::deserialize(data) { + match instruction { + MockInstruction::NoopSuccess => (), + MockInstruction::NoopFail => return Err(InstructionError::GenericError), + MockInstruction::ModifyOwned => { + keyed_accounts[0].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 + } + MockInstruction::ModifyNotOwned => { + keyed_accounts[1].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 + } + MockInstruction::ModifyReadonly => { + keyed_accounts[2].try_account_ref_mut()?.data_as_mut_slice()[0] = 1 + } + } + } else { + return Err(InstructionError::InvalidInstructionData); + } + Ok(()) + } + + let caller_program_id = solana_sdk::pubkey::new_rand(); + let callee_program_id = solana_sdk::pubkey::new_rand(); + + let owned_account = AccountSharedData::new(42, 1, &callee_program_id); + let not_owned_account = AccountSharedData::new(84, 1, &solana_sdk::pubkey::new_rand()); + let readonly_account = AccountSharedData::new(168, 1, &caller_program_id); + let mut program_account = AccountSharedData::new(1, 0, &native_loader::id()); + program_account.set_executable(true); + + #[allow(unused_mut)] + let accounts = vec![ + ( + solana_sdk::pubkey::new_rand(), + Rc::new(RefCell::new(owned_account)), + ), + ( + solana_sdk::pubkey::new_rand(), + Rc::new(RefCell::new(not_owned_account)), + ), + ( + solana_sdk::pubkey::new_rand(), + Rc::new(RefCell::new(readonly_account)), + ), + (callee_program_id, Rc::new(RefCell::new(program_account))), + ]; + let program_indices = vec![3]; + let programs: Vec<(_, ProcessInstructionWithContext)> = + vec![(callee_program_id, mock_process_instruction)]; + let metas = vec![ + AccountMeta::new(accounts[0].0, false), + AccountMeta::new(accounts[1].0, false), + AccountMeta::new_readonly(accounts[2].0, false), + ]; + + let caller_instruction = CompiledInstruction::new(2, &(), vec![0, 1, 2, 3]); + let callee_instruction = Instruction::new_with_bincode( + callee_program_id, + &MockInstruction::NoopSuccess, + metas.clone(), + ); + let message = Message::new(&[callee_instruction.clone()], None); + + let feature_set = FeatureSet::all_enabled(); + let ancestors = Ancestors::default(); + let blockhash = Hash::default(); + let fee_calculator = FeeCalculator::default(); + let mut invoke_context = ThisInvokeContext::new( + &caller_program_id, + Rent::default(), + &message, + &caller_instruction, + &program_indices, + &accounts, + programs.as_slice(), + None, + ComputeBudget::default(), + Rc::new(RefCell::new(MockComputeMeter::default())), + Rc::new(RefCell::new(Executors::default())), + None, + Arc::new(feature_set), + Arc::new(Accounts::default_for_tests()), + &ancestors, + &blockhash, + &fee_calculator, + ) + .unwrap(); + + // not owned account modified by the invoker + accounts[0].1.borrow_mut().data_as_mut_slice()[0] = 1; + assert_eq!( + InstructionProcessor::native_invoke( + &mut invoke_context, + callee_instruction.clone(), + &[0, 1, 2, 3], + &[] + ), + Err(InstructionError::ExternalAccountDataModified) + ); + accounts[0].1.borrow_mut().data_as_mut_slice()[0] = 0; + + // readonly account modified by the invoker + accounts[2].1.borrow_mut().data_as_mut_slice()[0] = 1; + assert_eq!( + InstructionProcessor::native_invoke( + &mut invoke_context, + callee_instruction, + &[0, 1, 2, 3], + &[] + ), + Err(InstructionError::ReadonlyDataModified) + ); + accounts[2].1.borrow_mut().data_as_mut_slice()[0] = 0; + + // Other test cases + let cases = vec![ + (MockInstruction::NoopSuccess, Ok(())), + ( + MockInstruction::NoopFail, + Err(InstructionError::GenericError), + ), + (MockInstruction::ModifyOwned, Ok(())), + ( + MockInstruction::ModifyNotOwned, + Err(InstructionError::ExternalAccountDataModified), + ), + ( + MockInstruction::ModifyReadonly, + Err(InstructionError::ReadonlyDataModified), + ), + ]; + for case in cases { + let callee_instruction = + Instruction::new_with_bincode(callee_program_id, &case.0, metas.clone()); + let message = Message::new(&[callee_instruction.clone()], None); + + let ancestors = Ancestors::default(); + let blockhash = Hash::default(); + let fee_calculator = FeeCalculator::default(); + let mut invoke_context = ThisInvokeContext::new( + &caller_program_id, + Rent::default(), + &message, + &caller_instruction, + &program_indices, + &accounts, + programs.as_slice(), + None, + ComputeBudget::default(), + Rc::new(RefCell::new(MockComputeMeter::default())), + Rc::new(RefCell::new(Executors::default())), + None, + Arc::new(FeatureSet::all_enabled()), + Arc::new(Accounts::default_for_tests()), + &ancestors, + &blockhash, + &fee_calculator, + ) + .unwrap(); + + assert_eq!( + InstructionProcessor::native_invoke( + &mut invoke_context, + callee_instruction, + &[0, 1, 2, 3], + &[] + ), + case.1 + ); + } + } } diff --git a/scripts/build-downstream-projects.sh b/scripts/build-downstream-projects.sh index ffcd44430..492e2f3b9 100755 --- a/scripts/build-downstream-projects.sh +++ b/scripts/build-downstream-projects.sh @@ -68,6 +68,7 @@ spl() { $cargo build $cargo test + $cargo_build_bpf $cargo_test_bpf ) } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 08ed1a287..4f6d82f8b 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -211,6 +211,10 @@ pub mod return_data_syscall_enabled { solana_sdk::declare_id!("BJVXq6NdLC7jCDGjfqJv7M1XHD4Y13VrpDqRF2U7UBcC"); } +pub mod fix_write_privs { + solana_sdk::declare_id!("7Tr5C1tdcCeBVD8jxtHYnvjL1DGdFboYBHCJkEFdenBb"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -258,7 +262,8 @@ lazy_static! { (ed25519_program_enabled::id(), "enable builtin ed25519 signature verify program"), (allow_native_ids::id(), "allow native program ids in program derived addresses"), (check_seed_length::id(), "Check program address seed lengths"), - (return_data_syscall_enabled::id(), "enable sol_{set,get}_return_data syscall") + (return_data_syscall_enabled::id(), "enable sol_{set,get}_return_data syscall"), + (fix_write_privs::id(), "fix native invoke write privileges"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()