diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index f092ef5757..77e7a6b900 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -35,7 +35,12 @@ impl PreAccount { is_writable, lamports: account.lamports, data_len: account.data.len(), - data: if Self::should_verify_data(&account.owner, program_id, is_writable) { + data: if Self::should_verify_data( + &account.owner, + program_id, + is_writable, + account.executable, + ) { Some(account.data.clone()) } else { None @@ -46,11 +51,18 @@ impl PreAccount { } } - fn should_verify_data(owner: &Pubkey, program_id: &Pubkey, is_writable: bool) -> bool { + fn should_verify_data( + owner: &Pubkey, + program_id: &Pubkey, + is_writable: bool, + is_executable: bool, + ) -> bool { // For accounts not assigned to the program, the data may not change. program_id != owner // Read-only account data may not change. || !is_writable + // Executable account data may not change. + || is_executable } pub fn verify(&self, program_id: &Pubkey, post: &Account) -> Result<(), InstructionError> { @@ -88,14 +100,16 @@ impl PreAccount { return Err(InstructionError::AccountDataSizeChanged); } - if Self::should_verify_data(&self.owner, program_id, self.is_writable) { + if Self::should_verify_data(&self.owner, program_id, self.is_writable, self.executable) { match &self.data { Some(data) if *data == post.data => (), _ => { - if !self.is_writable { - return Err(InstructionError::ReadonlyDataModified); - } else { + if self.executable { + return Err(InstructionError::ExecutableDataModified); + } else if self.is_writable { return Err(InstructionError::ExternalAccountDataModified); + } else { + return Err(InstructionError::ReadonlyDataModified); } } } @@ -476,12 +490,15 @@ mod tests { let change_executable = |program_id: &Pubkey, is_writable: bool, pre_executable: bool, - post_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, @@ -491,6 +508,7 @@ mod tests { let post = Account { owner, executable: post_executable, + data: post_data, ..Account::default() }; pre.verify(&program_id, &post) @@ -500,30 +518,45 @@ mod tests { let system_program_id = system_program::id(); assert_eq!( - change_executable(&system_program_id, true, false, true), + change_executable(&system_program_id, true, false, true, vec![1], vec![1]), Err(InstructionError::ExecutableModified), "system program can't change executable if system doesn't own the account" ); assert_eq!( - change_executable(&owner, true, false, true), + change_executable(&system_program_id, true, true, true, vec![1], vec![2]), + 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]), Ok(()), - "alice program should be able to change executable" + "owner should be able to change executable" ); assert_eq!( - change_executable(&owner, false, false, true), + change_executable(&owner, false, false, true, vec![1], vec![1]), Err(InstructionError::ExecutableModified), - "system program can't modify executable of read-only accounts" + "owner can't modify executable of read-only accounts" ); assert_eq!( - change_executable(&owner, true, true, false), + change_executable(&owner, true, true, false, vec![1], vec![1]), Err(InstructionError::ExecutableModified), - "system program can't reverse executable" + "owner program can't reverse executable" ); assert_eq!( - change_executable(&mallory_program_id, true, false, true), + change_executable(&mallory_program_id, true, false, true, vec![1], vec![1]), 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]), + 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]), + Err(InstructionError::ExecutableDataModified), + "owner should not be able to change an account's data once its marked executable" + ); } #[test] diff --git a/sdk/src/instruction.rs b/sdk/src/instruction.rs index 8ae3375c07..48eabad663 100644 --- a/sdk/src/instruction.rs +++ b/sdk/src/instruction.rs @@ -69,7 +69,7 @@ pub enum InstructionError { #[error("instruction changed balance of a read-only account")] ReadonlyLamportChange, - /// Read-only account modified data + /// Read-only account's data was modified #[error("instruction modified data of a read-only account")] ReadonlyDataModified, @@ -122,6 +122,10 @@ pub enum InstructionError { /// error value or a user-defined error in the lower 32 bits. #[error("program returned invalid error code")] InvalidError, + + /// Executable account's data was modified + #[error("instruction changed executable accounts data")] + ExecutableDataModified, } impl InstructionError {