From e2491c6322192bb974e6a369193cf6809d99fe29 Mon Sep 17 00:00:00 2001 From: Jack May Date: Fri, 27 Mar 2020 16:43:25 -0700 Subject: [PATCH] Prevent add/subtract from executable account (#9132) --- runtime/src/bank.rs | 9 +- runtime/src/message_processor.rs | 146 ++++++++++++++++++++++--------- sdk/src/instruction.rs | 8 +- 3 files changed, 118 insertions(+), 45 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 4e753c65f1..f243c83574 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2974,11 +2974,14 @@ mod tests { genesis_config.hash(), ); - assert_eq!(bank.process_transaction(&tx), Ok(())); assert_eq!( - bank.get_balance(&account_pubkey), - account_balance + transfer_lamports + bank.process_transaction(&tx), + Err(TransactionError::InstructionError( + 0, + InstructionError::ExecutableLamportChange + )) ); + assert_eq!(bank.get_balance(&account_pubkey), account_balance); } #[test] diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 77e7a6b900..c5913fc0e2 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -20,6 +20,7 @@ use libloading::os::windows::*; // The relevant state of an account before an Instruction executes, used // to verify account integrity after the Instruction completes +#[derive(Debug)] pub struct PreAccount { pub is_writable: bool, pub lamports: u64, @@ -84,11 +85,14 @@ impl PreAccount { return Err(InstructionError::ExternalAccountLamportSpend); } - // The balance of read-only accounts may not change. - if !self.is_writable // line coverage used to get branch coverage - && self.lamports != post.lamports - { - return Err(InstructionError::ReadonlyLamportChange); + // The balance of read-only and executable accounts may not change + if self.lamports != post.lamports { + if !self.is_writable { + return Err(InstructionError::ReadonlyLamportChange); + } + if self.executable { + return Err(InstructionError::ExecutableLamportChange); + } } // Only the system program can change the size of the data @@ -487,76 +491,138 @@ mod tests { #[test] fn test_verify_account_changes_executable() { let owner = Pubkey::new_rand(); - let change_executable = |program_id: &Pubkey, - is_writable: bool, - pre_executable: bool, - post_executable: bool, - pre_data: Vec, - post_data: Vec| - -> Result<(), InstructionError> { - let pre = PreAccount::new( - &Account { - owner, - executable: pre_executable, - data: pre_data, - ..Account::default() - }, - is_writable, - &program_id, - ); - - let post = Account { - owner, - executable: post_executable, - data: post_data, - ..Account::default() - }; - pre.verify(&program_id, &post) - }; - let mallory_program_id = Pubkey::new_rand(); let system_program_id = system_program::id(); + struct Change { + pre: PreAccount, + post: Account, + program_id: Pubkey, + } + impl Change { + pub fn new(owner: &Pubkey, program_id: &Pubkey) -> Self { + Self { + pre: PreAccount::new( + &Account { + owner: *owner, + data: vec![], + ..Account::default() + }, + true, + &Pubkey::new_rand(), // Force some data, ignored if not needed + ), + post: Account { + owner: *owner, + ..Account::default() + }, + program_id: *program_id, + } + } + pub fn read_only(mut self) -> Self { + self.pre.is_writable = false; + self + } + pub fn executable(mut self, pre: bool, post: bool) -> Self { + self.pre.executable = pre; + self.post.executable = post; + self + } + pub fn lamports(mut self, pre: u64, post: u64) -> Self { + self.pre.lamports = pre; + self.post.lamports = post; + self + } + pub fn data(mut self, pre: Vec, post: Vec) -> Self { + self.pre.data_len = pre.len(); + self.pre.data = Some(pre); + self.post.data = post; + self + } + pub fn verify(self) -> Result<(), InstructionError> { + self.pre.verify(&self.program_id, &self.post) + } + } + assert_eq!( - change_executable(&system_program_id, true, false, true, vec![1], vec![1]), + Change::new(&owner, &system_program_id) + .executable(false, true) + .verify(), Err(InstructionError::ExecutableModified), "system program can't change executable if system doesn't own the account" ); assert_eq!( - change_executable(&system_program_id, true, true, true, vec![1], vec![2]), + Change::new(&owner, &system_program_id) + .executable(true, true) + .data(vec![1], vec![2]) + .verify(), Err(InstructionError::ExecutableDataModified), "system program can't change executable data if system doesn't own the account" ); assert_eq!( - change_executable(&owner, true, false, true, vec![1], vec![1]), + Change::new(&owner, &owner).executable(false, true).verify(), Ok(()), "owner should be able to change executable" ); assert_eq!( - change_executable(&owner, false, false, true, vec![1], vec![1]), + Change::new(&owner, &owner) + .executable(false, true) + .read_only() + .verify(), Err(InstructionError::ExecutableModified), "owner can't modify executable of read-only accounts" ); assert_eq!( - change_executable(&owner, true, true, false, vec![1], vec![1]), + Change::new(&owner, &owner).executable(true, false).verify(), Err(InstructionError::ExecutableModified), "owner program can't reverse executable" ); assert_eq!( - change_executable(&mallory_program_id, true, false, true, vec![1], vec![1]), + Change::new(&owner, &mallory_program_id) + .executable(false, true) + .verify(), Err(InstructionError::ExecutableModified), "malicious Mallory should not be able to change the account executable" ); assert_eq!( - change_executable(&owner, true, false, true, vec![1], vec![2]), + Change::new(&owner, &owner) + .executable(false, true) + .data(vec![1], vec![2]) + .verify(), Ok(()), "account data can change in the same instruction that sets the bit" ); assert_eq!( - change_executable(&owner, true, true, true, vec![1], vec![2]), + Change::new(&owner, &owner) + .executable(true, true) + .data(vec![1], vec![2]) + .verify(), Err(InstructionError::ExecutableDataModified), "owner should not be able to change an account's data once its marked executable" ); + assert_eq!( + Change::new(&owner, &owner) + .executable(true, true) + .lamports(1, 2) + .verify(), + Err(InstructionError::ExecutableLamportChange), + "owner should not be able to add lamports once makred executable" + ); + assert_eq!( + Change::new(&owner, &owner) + .executable(true, true) + .lamports(1, 2) + .verify(), + Err(InstructionError::ExecutableLamportChange), + "owner should not be able to add lamports once marked executable" + ); + assert_eq!( + Change::new(&owner, &owner) + .executable(true, true) + .lamports(2, 1) + .verify(), + Err(InstructionError::ExecutableLamportChange), + "owner should not be able to subtract lamports once marked executable" + ); } #[test] diff --git a/sdk/src/instruction.rs b/sdk/src/instruction.rs index 48eabad663..0651beeb5e 100644 --- a/sdk/src/instruction.rs +++ b/sdk/src/instruction.rs @@ -65,8 +65,8 @@ pub enum InstructionError { #[error("instruction modified data of an account it does not own")] ExternalAccountDataModified, - /// Read-only account modified lamports - #[error("instruction changed balance of a read-only account")] + /// Read-only account's lamports modified + #[error("instruction changed the balance of a read-only account")] ReadonlyLamportChange, /// Read-only account's data was modified @@ -126,6 +126,10 @@ pub enum InstructionError { /// Executable account's data was modified #[error("instruction changed executable accounts data")] ExecutableDataModified, + + /// Executable account's lamports modified + #[error("instruction changed the balance of a executable account")] + ExecutableLamportChange, } impl InstructionError {