diff --git a/src/access_permissions.rs b/src/access_permissions.rs index 2b54b75..6d88ff9 100644 --- a/src/access_permissions.rs +++ b/src/access_permissions.rs @@ -5,8 +5,12 @@ use solana_transaction_error::TransactionError; use crate::account_loader::LoadedTransaction; +const MAGIC_PROGRAM_ID: Pubkey = + Pubkey::from_str_const("Magic11111111111111111111111111111111111111"); +const PRIVILEGED_MAGIC_DISCRIMINANTS: [u32; 9] = [0, 8, 9, 16, 17, 18, 19, 20, 22]; + // NOTE: -// this impl is kept separately to simplify synchoronization with upstream +// this impl is kept separately to simplify synchronization with upstream impl LoadedTransaction { /// Validates that a transaction does not attempt to write to non-delegated accounts. /// @@ -14,15 +18,15 @@ impl LoadedTransaction { /// account modifications are restricted to accounts explicitly delegated to the /// validator node. /// - /// ## Logic - /// This function enforces a security rule with a key exception: **if the fee payer - /// has privileged access, this check is bypassed entirely.** + /// Privileged fee payers may bypass this check only for the Magic program's + /// allowlisted control instructions. /// /// For standard, non-privileged transactions, it enforces that any account /// marked as writable (excluding the fee payer) must be either: /// 1. delegated /// 2. undelegating /// 3. ephemeral + /// 4. confined /// /// Read-only accounts are ignored. The fee payer's writability is handled in /// separate validation logic. @@ -31,13 +35,36 @@ impl LoadedTransaction { message: &impl SVMMessage, ) -> Result<(), (TransactionError, Pubkey)> { let payer = self.accounts.first().map(|(_, acc)| acc); - if payer.map(|p| p.privileged()).unwrap_or_default() { - // Payer has privileged access, so we can skip the validation. + let mut privileged = payer.is_some_and(AccountSharedData::privileged); + if privileged { + for i in message.instructions_iter() { + let Some(program) = message.account_keys().get(i.program_id_index as usize) else { + privileged = false; + break; + }; + if *program != MAGIC_PROGRAM_ID { + privileged = false; + break; + } + let discriminant = i + .data + .get(0..4) + .and_then(|b| <[u8; 4]>::try_from(b).ok()) + .map(u32::from_le_bytes) + .unwrap_or(u32::MAX); + if !PRIVILEGED_MAGIC_DISCRIMINANTS.contains(&discriminant) { + privileged = false; + break; + } + } + } + if privileged { return Ok(()); } - let mutation_allowed = - |acc: &AccountSharedData| acc.delegated() || acc.undelegating() || acc.ephemeral(); + let mutation_allowed = |acc: &AccountSharedData| { + acc.delegated() || acc.undelegating() || acc.ephemeral() || acc.confined() + }; // For non-privileged payers, validate the rest of the accounts. // Skip the fee payer (index 0), as its writability is validated elsewhere. @@ -49,3 +76,111 @@ impl LoadedTransaction { Ok(()) } } + +#[cfg(test)] +mod tests { + use { + super::*, + crate::{account_loader::LoadedTransaction, rollback_accounts::RollbackAccounts}, + solana_account::{ + test_utils::{create_borrowed_account_shared_data, BorrowedAccountBufferArea}, + AccountSharedData, + }, + solana_compute_budget::compute_budget_limits::ComputeBudgetLimits, + solana_fee_structure::FeeDetails, + solana_hash::Hash, + solana_message::{ + compiled_instruction::CompiledInstruction, LegacyMessage, Message, MessageHeader, + SanitizedMessage, + }, + solana_rent_debits::RentDebits, + solana_reserved_account_keys::ReservedAccountKeys, + }; + + fn privileged_account() -> (BorrowedAccountBufferArea, AccountSharedData) { + let account = AccountSharedData::default(); + let (buffer, mut account) = create_borrowed_account_shared_data(&account, 0); + account.as_borrowed_mut().unwrap().set_privileged(true); + (buffer, account) + } + + fn loaded_transaction( + payer: Pubkey, + payer_account: AccountSharedData, + writable: Pubkey, + ) -> LoadedTransaction { + LoadedTransaction { + accounts: vec![ + (payer, payer_account), + (writable, AccountSharedData::default()), + (MAGIC_PROGRAM_ID, AccountSharedData::default()), + ], + program_indices: vec![], + fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), + compute_budget_limits: ComputeBudgetLimits::default(), + rent: 0, + rent_debits: RentDebits::default(), + loaded_accounts_data_size: 0, + } + } + + fn message(program: Pubkey, data: Vec) -> SanitizedMessage { + SanitizedMessage::Legacy(LegacyMessage::new( + Message { + account_keys: vec![Pubkey::new_unique(), Pubkey::new_unique(), program], + header: MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 1, + }, + instructions: vec![CompiledInstruction { + program_id_index: 2, + accounts: vec![1], + data, + }], + recent_blockhash: Hash::default(), + }, + &ReservedAccountKeys::empty_key_set(), + )) + } + + #[test] + fn privileged_payer_allows_magic_control_instruction() { + let payer = Pubkey::new_unique(); + let writable = Pubkey::new_unique(); + let (_buffer, payer_account) = privileged_account(); + let tx = loaded_transaction(payer, payer_account, writable); + let message = message(MAGIC_PROGRAM_ID, 8u32.to_le_bytes().to_vec()); + + assert_eq!(tx.validate_accounts_access(&message), Ok(())); + } + + #[test] + fn privileged_payer_rejects_non_magic_write() { + let payer = Pubkey::new_unique(); + let writable = Pubkey::new_unique(); + let (_buffer, payer_account) = privileged_account(); + let tx = loaded_transaction(payer, payer_account, writable); + let message = message(Pubkey::new_unique(), 8u32.to_le_bytes().to_vec()); + + assert_eq!( + tx.validate_accounts_access(&message), + Err((TransactionError::InvalidWritableAccount, writable)) + ); + } + + #[test] + fn privileged_payer_rejects_unlisted_magic_instruction() { + let payer = Pubkey::new_unique(); + let writable = Pubkey::new_unique(); + let (_buffer, payer_account) = privileged_account(); + let tx = loaded_transaction(payer, payer_account, writable); + let message = message(MAGIC_PROGRAM_ID, 1u32.to_le_bytes().to_vec()); + + assert_eq!( + tx.validate_accounts_access(&message), + Err((TransactionError::InvalidWritableAccount, writable)) + ); + } +} diff --git a/tests/integration_test.rs b/tests/integration_test.rs index fec8e4d..aedf7be 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -310,11 +310,7 @@ impl SvmTestEnvironment<'_> { // check that all the account states we care about are present and correct for (pubkey, expected_account_data) in self.test_entry.final_accounts.iter() { let actual_account_data = final_accounts_actual.get(pubkey); - assert!( - actual_account_data.is_some(), - "missing account {}", - pubkey - ); + assert!(actual_account_data.is_some(), "missing account {}", pubkey); assert!( solana_account::accounts_equal(expected_account_data, actual_account_data.unwrap()), "mismatch on account {}\n expected: {:?}\n actual: {:?}", @@ -2422,7 +2418,7 @@ fn svm_inspect_account() { Hash::default(), ); - initial_test_entry.push_transaction(transaction); + initial_test_entry.push_transaction_with_status(transaction, ExecutionStatus::ExecutedFailed); let mut recipient_account = AccountSharedData::default(); recipient_account.set_lamports(transfer_amount); @@ -2493,7 +2489,7 @@ fn svm_inspect_account() { Hash::default(), ); - final_test_entry.push_transaction(transaction); + final_test_entry.push_transaction_with_status(transaction, ExecutionStatus::ExecutedFailed); final_test_entry.decrease_expected_lamports(&fee_payer, LAMPORTS_PER_SIGNATURE * 2); final_test_entry.decrease_expected_lamports(&sender, transfer_amount); @@ -3110,11 +3106,11 @@ fn enforce_access_permissions_false_with_multiple_non_delegated_accounts() { } // ----------------- -// Privileged Accounts Override Access Checks +// Privileged Accounts // ----------------- #[test] -fn test_privileged_fee_payer_with_non_delegated_writable_accounts() { - // Test that privileged fee payer can write to non-delegated writable accounts +fn privileged_fee_payer_with_non_magic_program_cannot_write_non_delegated_accounts() { + // Privileged fee payers only bypass access checks for allowlisted Magic instructions. let fee_payer_keypair = Keypair::new(); let fee_payer = fee_payer_keypair.pubkey(); let (_guard, mut fee_payer_account) = create_privileged_account(LAMPORTS_PER_SOL); @@ -3143,9 +3139,9 @@ fn test_privileged_fee_payer_with_non_delegated_writable_accounts() { LAST_BLOCKHASH, ); - test_entry.push_transaction(transaction); + test_entry.push_transaction_with_status(transaction, ExecutionStatus::ExecutedFailed); test_entry.decrease_expected_lamports(&fee_payer, LAMPORTS_PER_SIGNATURE); let env = SvmTestEnvironment::create(test_entry); env.execute(); -} \ No newline at end of file +}