From 8a14638d15106dac2bfd751a1843d7de7ffefbe5 Mon Sep 17 00:00:00 2001 From: Jack May Date: Fri, 15 Jan 2021 17:23:23 -0800 Subject: [PATCH] Track account writable deescalation --- programs/bpf/c/src/invoke/invoke.c | 16 +++++- programs/bpf/c/src/invoked/instruction.h | 1 + programs/bpf/c/src/invoked/invoked.c | 14 ++++++ programs/bpf/rust/invoke/src/lib.rs | 10 ++++ programs/bpf/rust/invoked/src/instruction.rs | 1 + programs/bpf/rust/invoked/src/processor.rs | 6 +++ programs/bpf/tests/programs.rs | 30 ++++++++++- runtime/benches/message_processor.rs | 21 +++----- runtime/src/message_processor.rs | 53 +++++++++----------- 9 files changed, 105 insertions(+), 47 deletions(-) diff --git a/programs/bpf/c/src/invoke/invoke.c b/programs/bpf/c/src/invoke/invoke.c index 2eaef8386abd03..f065a120f9f7a5 100644 --- a/programs/bpf/c/src/invoke/invoke.c +++ b/programs/bpf/c/src/invoke/invoke.c @@ -15,6 +15,7 @@ static const uint8_t TEST_ALLOC_ACCESS_VIOLATION = 8; static const uint8_t TEST_INSTRUCTION_DATA_TOO_LARGE = 9; static const uint8_t TEST_INSTRUCTION_META_TOO_LARGE = 10; static const uint8_t TEST_RETURN_ERROR = 11; +static const uint8_t TEST_WRITE_DEESCALATION = 12; static const int MINT_INDEX = 0; static const int ARGUMENT_INDEX = 1; @@ -427,7 +428,8 @@ extern uint64_t entrypoint(const uint8_t *input) { break; } case TEST_RETURN_ERROR: { - SolAccountMeta arguments[] = {{accounts[ARGUMENT_INDEX].key, true, true}}; + sol_log("Test return error"); + SolAccountMeta arguments[] = {{accounts[ARGUMENT_INDEX].key, false, true}}; uint8_t data[] = {RETURN_ERROR}; const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, arguments, SOL_ARRAY_SIZE(arguments), @@ -436,6 +438,18 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts)); break; } + case TEST_WRITE_DEESCALATION: { + sol_log("Test writable deescalation"); + + SolAccountMeta arguments[] = { + {accounts[INVOKED_ARGUMENT_INDEX].key, false, false}}; + uint8_t data[] = {WRITE_ACCOUNT, 10}; + const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, + arguments, SOL_ARRAY_SIZE(arguments), + data, SOL_ARRAY_SIZE(data)}; + sol_invoke(&instruction, accounts, SOL_ARRAY_SIZE(accounts)); + break; + } default: sol_panic(); } diff --git a/programs/bpf/c/src/invoked/instruction.h b/programs/bpf/c/src/invoked/instruction.h index 69e0dfa1c1d944..25ac0f6ead55a1 100644 --- a/programs/bpf/c/src/invoked/instruction.h +++ b/programs/bpf/c/src/invoked/instruction.h @@ -12,3 +12,4 @@ const uint8_t VERIFY_WRITER = 4; const uint8_t VERIFY_PRIVILEGE_ESCALATION = 5; const uint8_t NESTED_INVOKE = 6; const uint8_t RETURN_OK = 7; +const uint8_t WRITE_ACCOUNT = 8; diff --git a/programs/bpf/c/src/invoked/invoked.c b/programs/bpf/c/src/invoked/invoked.c index 4da30a9adda152..69ee5bf6fd4f73 100644 --- a/programs/bpf/c/src/invoked/invoked.c +++ b/programs/bpf/c/src/invoked/invoked.c @@ -156,10 +156,12 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_assert(accounts[ARGUMENT_INDEX].is_writable); break; } + case VERIFY_PRIVILEGE_ESCALATION: { sol_log("Success"); break; } + case NESTED_INVOKE: { sol_log("invoke"); @@ -198,6 +200,18 @@ extern uint64_t entrypoint(const uint8_t *input) { } break; } + + case WRITE_ACCOUNT: { + sol_log("write account"); + static const int INVOKED_ARGUMENT_INDEX = 0; + sol_assert(sol_deserialize(input, ¶ms, 1)); + + for (int i = 0; i < params.data[1]; i++) { + accounts[INVOKED_ARGUMENT_INDEX].data[i] = params.data[1]; + } + break; + } + default: return ERROR_INVALID_INSTRUCTION_DATA; } diff --git a/programs/bpf/rust/invoke/src/lib.rs b/programs/bpf/rust/invoke/src/lib.rs index 81613f44706c89..2ec0a0a98d4e50 100644 --- a/programs/bpf/rust/invoke/src/lib.rs +++ b/programs/bpf/rust/invoke/src/lib.rs @@ -27,6 +27,7 @@ const TEST_ALLOC_ACCESS_VIOLATION: u8 = 8; const TEST_INSTRUCTION_DATA_TOO_LARGE: u8 = 9; const TEST_INSTRUCTION_META_TOO_LARGE: u8 = 10; const TEST_RETURN_ERROR: u8 = 11; +const TEST_WRITE_DEESCALATION: u8 = 12; // const MINT_INDEX: usize = 0; const ARGUMENT_INDEX: usize = 1; @@ -492,6 +493,15 @@ fn process_instruction( ); let _ = invoke(&instruction, accounts); } + TEST_WRITE_DEESCALATION => { + msg!("Test writable deescalation"); + let instruction = create_instruction( + *accounts[INVOKED_PROGRAM_INDEX].key, + &[(accounts[INVOKED_ARGUMENT_INDEX].key, false, false)], + vec![WRITE_ACCOUNT, 10], + ); + let _ = invoke(&instruction, accounts); + } _ => panic!(), } diff --git a/programs/bpf/rust/invoked/src/instruction.rs b/programs/bpf/rust/invoked/src/instruction.rs index 0236f08bb2f014..ca0ee12d668a36 100644 --- a/programs/bpf/rust/invoked/src/instruction.rs +++ b/programs/bpf/rust/invoked/src/instruction.rs @@ -13,6 +13,7 @@ pub const VERIFY_WRITER: u8 = 4; pub const VERIFY_PRIVILEGE_ESCALATION: u8 = 5; pub const NESTED_INVOKE: u8 = 6; pub const RETURN_OK: u8 = 7; +pub const WRITE_ACCOUNT: u8 = 8; pub fn create_instruction( program_id: Pubkey, diff --git a/programs/bpf/rust/invoked/src/processor.rs b/programs/bpf/rust/invoked/src/processor.rs index 77b1c9eee277c6..48c42fc6424481 100644 --- a/programs/bpf/rust/invoked/src/processor.rs +++ b/programs/bpf/rust/invoked/src/processor.rs @@ -195,6 +195,12 @@ fn process_instruction( } } } + WRITE_ACCOUNT => { + msg!("write account"); + for i in 0..instruction_data[1] { + accounts[0].data.borrow_mut()[i as usize] = instruction_data[1]; + } + } _ => panic!(), } diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 29059014538a2c..076903581d7e78 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -556,7 +556,7 @@ fn test_program_bpf_error_handling() { } #[test] -fn test_program_bpf_invoke() { +fn test_program_bpf_invoke_sanity() { solana_logger::setup(); const TEST_SUCCESS: u8 = 1; @@ -570,6 +570,7 @@ fn test_program_bpf_invoke() { const TEST_INSTRUCTION_DATA_TOO_LARGE: u8 = 9; const TEST_INSTRUCTION_META_TOO_LARGE: u8 = 10; const TEST_RETURN_ERROR: u8 = 11; + const TEST_WRITE_DEESCALATION: u8 = 12; #[allow(dead_code)] #[derive(Debug)] @@ -988,6 +989,33 @@ fn test_program_bpf_invoke() { TransactionError::InstructionError(0, InstructionError::Custom(42)) ); + let instruction = Instruction::new( + invoke_program_id, + &[TEST_WRITE_DEESCALATION, bump_seed1, bump_seed2, bump_seed3], + account_metas.clone(), + ); + let message = Message::new(&[instruction], Some(&mint_pubkey)); + let tx = Transaction::new( + &[ + &mint_keypair, + &argument_keypair, + &invoked_argument_keypair, + &from_keypair, + ], + message.clone(), + bank.last_blockhash(), + ); + let (result, inner_instructions) = process_transaction_and_record_inner(&bank, tx); + let invoked_programs: Vec = inner_instructions[0] + .iter() + .map(|ix| message.account_keys[ix.program_id_index as usize].clone()) + .collect(); + assert_eq!(invoked_programs, vec![invoked_program_id.clone()]); + assert_eq!( + result.unwrap_err(), + TransactionError::InstructionError(0, InstructionError::ReadonlyDataModified) + ); + // Check final state assert_eq!(43, bank.get_balance(&derived_key1)); diff --git a/runtime/benches/message_processor.rs b/runtime/benches/message_processor.rs index bf4d16d256e562..c0aec1a805080a 100644 --- a/runtime/benches/message_processor.rs +++ b/runtime/benches/message_processor.rs @@ -13,18 +13,13 @@ fn bench_verify_account_changes_data(bencher: &mut Bencher) { let owner = pubkey::new_rand(); let non_owner = pubkey::new_rand(); - let pre = PreAccount::new( - &pubkey::new_rand(), - &Account::new(0, BUFSIZE, &owner), - true, - false, - ); + let pre = PreAccount::new(&pubkey::new_rand(), &Account::new(0, BUFSIZE, &owner)); let post = Account::new(0, BUFSIZE, &owner); - assert_eq!(pre.verify(&owner, &Rent::default(), &post), Ok(())); + assert_eq!(pre.verify(&owner, false, &Rent::default(), &post), Ok(())); // this one should be faster bencher.iter(|| { - pre.verify(&owner, &Rent::default(), &post).unwrap(); + pre.verify(&owner, false, &Rent::default(), &post).unwrap(); }); let summary = bencher.bench(|_bencher| {}).unwrap(); info!("data no change by owner: {} ns/iter", summary.median); @@ -35,14 +30,10 @@ fn bench_verify_account_changes_data(bencher: &mut Bencher) { let summary = bencher.bench(|_bencher| {}).unwrap(); info!("data compare {} ns/iter", summary.median); - let pre = PreAccount::new( - &pubkey::new_rand(), - &Account::new(0, BUFSIZE, &owner), - true, - false, - ); + let pre = PreAccount::new(&pubkey::new_rand(), &Account::new(0, BUFSIZE, &owner)); bencher.iter(|| { - pre.verify(&non_owner, &Rent::default(), &post).unwrap(); + pre.verify(&non_owner, false, &Rent::default(), &post) + .unwrap(); }); let summary = bencher.bench(|_bencher| {}).unwrap(); info!("data no change by non owner: {} ns/iter", summary.median); diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 9cbf2c5ec479dd..5d0d854951c9d2 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -51,16 +51,12 @@ impl Executors { #[derive(Clone, Debug, Default)] pub struct PreAccount { key: Pubkey, - is_signer: bool, - is_writable: bool, account: RefCell, } impl PreAccount { - pub fn new(key: &Pubkey, account: &Account, is_signer: bool, is_writable: bool) -> Self { + pub fn new(key: &Pubkey, account: &Account) -> Self { Self { key: *key, - is_signer, - is_writable, account: RefCell::new(account.clone()), } } @@ -68,6 +64,7 @@ impl PreAccount { pub fn verify( &self, program_id: &Pubkey, + is_writable: bool, rent: &Rent, post: &Account, ) -> Result<(), InstructionError> { @@ -78,7 +75,7 @@ impl PreAccount { // only if the account is not executable and // only if the data is zero-initialized or empty if pre.owner != post.owner - && (!self.is_writable // line coverage used to get branch coverage + && (!is_writable // line coverage used to get branch coverage || pre.executable || *program_id != pre.owner || !Self::is_zeroed(&post.data)) @@ -95,7 +92,7 @@ impl PreAccount { // The balance of read-only and executable accounts may not change if pre.lamports != post.lamports { - if !self.is_writable { + if !is_writable { return Err(InstructionError::ReadonlyLamportChange); } if pre.executable { @@ -116,13 +113,13 @@ impl PreAccount { // and if the account is writable // and if the account is not executable if !(*program_id == pre.owner - && self.is_writable // line coverage used to get branch coverage + && is_writable // line coverage used to get branch coverage && !pre.executable) && pre.data != post.data { if pre.executable { return Err(InstructionError::ExecutableDataModified); - } else if self.is_writable { + } else if is_writable { return Err(InstructionError::ExternalAccountDataModified); } else { return Err(InstructionError::ReadonlyDataModified); @@ -134,7 +131,7 @@ impl PreAccount { if !rent.is_exempt(post.lamports, post.data.len()) { return Err(InstructionError::ExecutableAccountNotRentExempt); } - if !self.is_writable // line coverage used to get branch coverage + if !is_writable // line coverage used to get branch coverage || pre.executable || *program_id != pre.owner { @@ -278,6 +275,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { key, &self.rent, ), + None => Err(InstructionError::GenericError), // Should never happen } } @@ -706,10 +704,8 @@ impl MessageProcessor { { let mut work = |_unique_index: usize, account_index: usize| { let key = &message.account_keys[account_index]; - let is_signer = account_index < message.header.num_required_signatures as usize; - let is_writable = message.is_writable(account_index); let account = accounts[account_index].borrow(); - pre_accounts.push(PreAccount::new(key, &account, is_signer, is_writable)); + pre_accounts.push(PreAccount::new(key, &account)); Ok(()) }; let _ = instruction.visit_each_account(&mut work); @@ -750,7 +746,8 @@ impl MessageProcessor { let account = accounts[account_index] .try_borrow_mut() .map_err(|_| InstructionError::AccountBorrowOutstanding)?; - pre_accounts[unique_index].verify(&program_id, rent, &account)?; + let is_writable = message.is_writable(account_index); + pre_accounts[unique_index].verify(&program_id, is_writable, rent, &account)?; pre_sum += u128::from(pre_accounts[unique_index].lamports()); post_sum += u128::from(account.lamports); Ok(()) @@ -780,6 +777,7 @@ impl MessageProcessor { if account_index < message.account_keys.len() && account_index < accounts.len() { let key = &message.account_keys[account_index]; let account = &accounts[account_index]; + let is_writable = message.is_writable(account_index); // Find the matching PreAccount for pre_account in pre_accounts.iter_mut() { if *key == pre_account.key() { @@ -788,7 +786,7 @@ impl MessageProcessor { .try_borrow_mut() .map_err(|_| InstructionError::AccountBorrowOutstanding)?; - pre_account.verify(&program_id, &rent, &account)?; + pre_account.verify(&program_id, is_writable, &rent, &account)?; pre_sum += u128::from(pre_account.lamports()); post_sum += u128::from(account.lamports); @@ -943,16 +941,11 @@ mod tests { 1, &program_ids[i], )))); - pre_accounts.push(PreAccount::new( - &keys[i], - &accounts[i].borrow(), - false, - true, - )) + pre_accounts.push(PreAccount::new(&keys[i], &accounts[i].borrow())) } let account = Account::new(1, 1, &solana_sdk::pubkey::Pubkey::default()); for program_id in program_ids.iter() { - pre_accounts.push(PreAccount::new(program_id, &account.clone(), false, true)); + pre_accounts.push(PreAccount::new(program_id, &account.clone())); } let mut invoke_context = ThisInvokeContext::new( @@ -1074,6 +1067,7 @@ mod tests { struct Change { program_id: Pubkey, + is_writable: bool, rent: Rent, pre: PreAccount, post: Account, @@ -1083,6 +1077,7 @@ mod tests { Self { program_id: *program_id, rent: Rent::default(), + is_writable: true, pre: PreAccount::new( &solana_sdk::pubkey::new_rand(), &Account { @@ -1091,8 +1086,6 @@ mod tests { data: vec![], ..Account::default() }, - false, - true, ), post: Account { owner: *owner, @@ -1102,7 +1095,7 @@ mod tests { } } pub fn read_only(mut self) -> Self { - self.pre.is_writable = false; + self.is_writable = false; self } pub fn executable(mut self, pre: bool, post: bool) -> Self { @@ -1130,7 +1123,8 @@ mod tests { self } pub fn verify(&self) -> Result<(), InstructionError> { - self.pre.verify(&self.program_id, &self.rent, &self.post) + self.pre + .verify(&self.program_id, self.is_writable, &self.rent, &self.post) } } @@ -1802,17 +1796,16 @@ mod tests { let mut program_account = Account::new(1, 0, &native_loader::id()); program_account.executable = true; - let executable_preaccount = - PreAccount::new(&callee_program_id, &program_account, false, true); + let executable_preaccount = PreAccount::new(&callee_program_id, &program_account); let executable_accounts = vec![(callee_program_id, RefCell::new(program_account.clone()))]; let owned_key = solana_sdk::pubkey::new_rand(); let owned_account = Account::new(42, 1, &callee_program_id); - let owned_preaccount = PreAccount::new(&owned_key, &owned_account, false, true); + let owned_preaccount = PreAccount::new(&owned_key, &owned_account); let not_owned_key = solana_sdk::pubkey::new_rand(); let not_owned_account = Account::new(84, 1, &solana_sdk::pubkey::new_rand()); - let not_owned_preaccount = PreAccount::new(¬_owned_key, ¬_owned_account, false, true); + let not_owned_preaccount = PreAccount::new(¬_owned_key, ¬_owned_account); #[allow(unused_mut)] let mut accounts = vec![