From b75d0c38a1682c0ccecd0fabf2d77150f12089f7 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 3 Sep 2021 12:01:27 -0600 Subject: [PATCH 1/7] Return error if Transaction locks an executable as writable --- runtime/src/accounts.rs | 94 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 5fade1b981eee9..f12c0e97ec7ca9 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -278,6 +278,15 @@ impl Accounts { }) .unwrap_or_default(); + if account.executable() + && demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) + && !is_upgradeable_loader_present(message) + { + error_counters.invalid_account_index += 1; + return Err(TransactionError::InvalidAccountIndex); + } + if account.executable() && bpf_loader_upgradeable::check_id(account.owner()) { // The upgradeable loader requires the derived ProgramData account @@ -1106,6 +1115,12 @@ pub fn prepare_if_nonce_account( false } +fn is_upgradeable_loader_present(message: &SanitizedMessage) -> bool { + message + .account_keys_iter() + .any(|&key| key == bpf_loader_upgradeable::id()) +} + pub fn create_test_accounts( accounts: &Accounts, pubkeys: &mut Vec, @@ -1682,6 +1697,85 @@ mod tests { assert_eq!(loaded, vec![]); } + #[test] + fn test_load_accounts_executable_with_write_lock() { + let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new(); + let mut error_counters = ErrorCounters::default(); + + let keypair = Keypair::new(); + let key0 = keypair.pubkey(); + let key1 = Pubkey::new(&[5u8; 32]); + let key2 = Pubkey::new(&[6u8; 32]); + + let mut account = AccountSharedData::new(1, 0, &Pubkey::default()); + account.set_rent_epoch(1); + accounts.push((key0, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key1, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key2, account)); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let mut message = Message::new_with_compiled_instructions( + 1, + 0, + 1, // only one executable marked as readonly + vec![key0, key1, key2], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(loaded_accounts.len(), 1); + assert_eq!( + loaded_accounts[0], + (Err(TransactionError::InvalidAccountIndex), None) + ); + + // Solution 1: include bpf_loader_upgradeable account + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable + account.set_executable(true); + account.set_rent_epoch(1); + let accounts_with_upgradeable_loader = vec![ + accounts[0].clone(), + accounts[1].clone(), + (bpf_loader_upgradeable::id(), account), + ]; + message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()]; + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = + load_accounts(tx, &accounts_with_upgradeable_loader, &mut error_counters); + + assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts_with_upgradeable_loader[..2]); + assert_eq!( + result.loaders[0], + vec![accounts_with_upgradeable_loader[2].clone()] + ); + + // Solution 2: mark programdata as readonly + message.account_keys = vec![key0, key1, key2]; // revert key change + message.header.num_readonly_unsigned_accounts = 2; // ark both executables as readonly + let tx = Transaction::new(&[&keypair], message, Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts[..2]); + assert_eq!(result.loaders[0], vec![accounts[2].clone()]); + } + #[test] fn test_accounts_account_not_found() { let accounts = Accounts::new_with_config_for_tests( From 0eca269dfad86b838c2ee67be9827194ddd237a5 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 3 Sep 2021 12:09:40 -0600 Subject: [PATCH 2/7] Return error if a ProgramData account is writable but the upgradable loader isn't present --- runtime/src/accounts.rs | 131 ++++++++++++++++++++++++++++++++++------ 1 file changed, 114 insertions(+), 17 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index f12c0e97ec7ca9..8119f872c2c945 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -287,26 +287,40 @@ impl Accounts { return Err(TransactionError::InvalidAccountIndex); } - if account.executable() && bpf_loader_upgradeable::check_id(account.owner()) - { - // The upgradeable loader requires the derived ProgramData account - if let Ok(UpgradeableLoaderState::Program { - programdata_address, - }) = account.state() - { - if let Some(account) = self - .accounts_db - .load_with_fixed_root(ancestors, &programdata_address) - .map(|(account, _)| account) + if bpf_loader_upgradeable::check_id(account.owner()) { + if account.executable() { + // The upgradeable loader requires the derived ProgramData account + if let Ok(UpgradeableLoaderState::Program { + programdata_address, + }) = account.state() { - account_deps.push((programdata_address, account)); + if let Some(account) = self + .accounts_db + .load_with_fixed_root(ancestors, &programdata_address) + .map(|(account, _)| account) + { + account_deps.push((programdata_address, account)); + } else { + error_counters.account_not_found += 1; + return Err(TransactionError::ProgramAccountNotFound); + } } else { - error_counters.account_not_found += 1; - return Err(TransactionError::ProgramAccountNotFound); + error_counters.invalid_program_for_execution += 1; + return Err(TransactionError::InvalidProgramForExecution); + } + } + + // If a ProgramData account is present, it should only be writable + // if the bpf_loader_upgradeable account is also present + if let Ok(UpgradeableLoaderState::ProgramData { .. }) = account.state() + { + if demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) + && !is_upgradeable_loader_present(message) + { + error_counters.invalid_account_index += 1; + return Err(TransactionError::InvalidAccountIndex); } - } else { - error_counters.invalid_program_for_execution += 1; - return Err(TransactionError::InvalidProgramForExecution); } } @@ -1776,6 +1790,89 @@ mod tests { assert_eq!(result.loaders[0], vec![accounts[2].clone()]); } + #[test] + fn test_load_accounts_programdata_with_write_lock() { + let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new(); + let mut error_counters = ErrorCounters::default(); + + let keypair = Keypair::new(); + let key0 = keypair.pubkey(); + let key1 = Pubkey::new(&[5u8; 32]); + let key2 = Pubkey::new(&[6u8; 32]); + + let mut account = AccountSharedData::new(1, 0, &Pubkey::default()); + account.set_rent_epoch(1); + accounts.push((key0, account)); + + let program_data = UpgradeableLoaderState::ProgramData { + slot: 42, + upgrade_authority_address: None, + }; + let mut account = + AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap(); + account.set_rent_epoch(1); + accounts.push((key1, account)); + + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key2, account)); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let mut message = Message::new_with_compiled_instructions( + 1, + 0, + 1, // only the program marked as readonly + vec![key0, key1, key2], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(loaded_accounts.len(), 1); + assert_eq!( + loaded_accounts[0], + (Err(TransactionError::InvalidAccountIndex), None) + ); + + // Solution 1: include bpf_loader_upgradeable account + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable + account.set_executable(true); + account.set_rent_epoch(1); + let accounts_with_upgradeable_loader = vec![ + accounts[0].clone(), + accounts[1].clone(), + (bpf_loader_upgradeable::id(), account), + ]; + message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()]; + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = + load_accounts(tx, &accounts_with_upgradeable_loader, &mut error_counters); + + assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts_with_upgradeable_loader[..2]); + assert_eq!( + result.loaders[0], + vec![accounts_with_upgradeable_loader[2].clone()] + ); + + // Solution 2: mark programdata as readonly + message.account_keys = vec![key0, key1, key2]; // revert key change + message.header.num_readonly_unsigned_accounts = 2; // extend readonly set to include programdata + let tx = Transaction::new(&[&keypair], message, Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts[..2]); + assert_eq!(result.loaders[0], vec![accounts[2].clone()]); + } + #[test] fn test_accounts_account_not_found() { let accounts = Accounts::new_with_config_for_tests( From bb85c6b79b3a8f08671bc1a25371f9bece7d60af Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 3 Sep 2021 16:55:00 -0600 Subject: [PATCH 3/7] Remove unreachable clause --- runtime/src/accounts.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 8119f872c2c945..2d2c1c0b5c5d9a 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -251,9 +251,6 @@ impl Accounts { } if solana_sdk::sysvar::instructions::check_id(key) { - if message.is_writable(i, demote_program_write_locks) { - return Err(TransactionError::InvalidAccountIndex); - } Self::construct_instructions_account( message, feature_set From c29316f64bf82b948c22834fa11d1f6026b5f0b6 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 3 Sep 2021 23:28:37 -0600 Subject: [PATCH 4/7] Fixup bpf tests --- programs/bpf/tests/programs.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index cc337f48683572..31979d4937eb63 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1865,9 +1865,9 @@ fn test_program_bpf_invoke_upgradeable_via_cpi() { invoke_and_return, &[0], vec![ - AccountMeta::new(program_id, false), - AccountMeta::new(program_id, false), - AccountMeta::new(clock::id(), false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(clock::id(), false), ], ); @@ -2054,9 +2054,9 @@ fn test_program_bpf_upgrade_via_cpi() { invoke_and_return, &[0], vec![ - AccountMeta::new(program_id, false), - AccountMeta::new(program_id, false), - AccountMeta::new(clock::id(), false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(clock::id(), false), ], ); From ee28a5271d5de80b28d9d6e81ae5f4249b913649 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Tue, 7 Sep 2021 13:11:34 -0500 Subject: [PATCH 5/7] Review comments --- runtime/src/accounts.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 2d2c1c0b5c5d9a..84e440493d3ea5 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -243,6 +243,7 @@ impl Accounts { let rent_for_sysvars = feature_set.is_active(&feature_set::rent_for_sysvars::id()); let demote_program_write_locks = feature_set.is_active(&feature_set::demote_program_write_locks::id()); + let is_upgradeable_loader_present = is_upgradeable_loader_present(message); for (i, key) in message.account_keys_iter().enumerate() { let account = if message.is_non_loader_key(i) { @@ -278,7 +279,7 @@ impl Accounts { if account.executable() && demote_program_write_locks && message.is_writable(i, demote_program_write_locks) - && !is_upgradeable_loader_present(message) + && !is_upgradeable_loader_present { error_counters.invalid_account_index += 1; return Err(TransactionError::InvalidAccountIndex); @@ -305,15 +306,14 @@ impl Accounts { error_counters.invalid_program_for_execution += 1; return Err(TransactionError::InvalidProgramForExecution); } - } - - // If a ProgramData account is present, it should only be writable - // if the bpf_loader_upgradeable account is also present - if let Ok(UpgradeableLoaderState::ProgramData { .. }) = account.state() + } else if demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) + && !is_upgradeable_loader_present { - if demote_program_write_locks - && message.is_writable(i, demote_program_write_locks) - && !is_upgradeable_loader_present(message) + // If a ProgramData account is present, it should only be writable + // if the bpf_loader_upgradeable account is also present + if let Ok(UpgradeableLoaderState::ProgramData { .. }) = + account.state() { error_counters.invalid_account_index += 1; return Err(TransactionError::InvalidAccountIndex); From a9fdc52968cde453271d6e711f552d9061bc0a9a Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Tue, 7 Sep 2021 13:56:41 -0500 Subject: [PATCH 6/7] Add new TransactionError --- runtime/src/accounts.rs | 24 +++++++++---------- runtime/src/accounts_db.rs | 1 + runtime/src/bank.rs | 8 ++++++- sdk/src/transaction/mod.rs | 4 ++++ storage-proto/proto/transaction_by_addr.proto | 1 + storage-proto/src/convert.rs | 4 ++++ 6 files changed, 29 insertions(+), 13 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 84e440493d3ea5..4686c2ffb06794 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -281,8 +281,8 @@ impl Accounts { && message.is_writable(i, demote_program_write_locks) && !is_upgradeable_loader_present { - error_counters.invalid_account_index += 1; - return Err(TransactionError::InvalidAccountIndex); + error_counters.invalid_writable_account += 1; + return Err(TransactionError::InvalidWritableAccount); } if bpf_loader_upgradeable::check_id(account.owner()) { @@ -315,8 +315,8 @@ impl Accounts { if let Ok(UpgradeableLoaderState::ProgramData { .. }) = account.state() { - error_counters.invalid_account_index += 1; - return Err(TransactionError::InvalidAccountIndex); + error_counters.invalid_writable_account += 1; + return Err(TransactionError::InvalidWritableAccount); } } } @@ -1744,11 +1744,11 @@ mod tests { let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); - assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(error_counters.invalid_writable_account, 1); assert_eq!(loaded_accounts.len(), 1); assert_eq!( loaded_accounts[0], - (Err(TransactionError::InvalidAccountIndex), None) + (Err(TransactionError::InvalidWritableAccount), None) ); // Solution 1: include bpf_loader_upgradeable account @@ -1765,7 +1765,7 @@ mod tests { let loaded_accounts = load_accounts(tx, &accounts_with_upgradeable_loader, &mut error_counters); - assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(error_counters.invalid_writable_account, 1); assert_eq!(loaded_accounts.len(), 1); let result = loaded_accounts[0].0.as_ref().unwrap(); assert_eq!(result.accounts[..2], accounts_with_upgradeable_loader[..2]); @@ -1780,7 +1780,7 @@ mod tests { let tx = Transaction::new(&[&keypair], message, Hash::default()); let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); - assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(error_counters.invalid_writable_account, 1); assert_eq!(loaded_accounts.len(), 1); let result = loaded_accounts[0].0.as_ref().unwrap(); assert_eq!(result.accounts[..2], accounts[..2]); @@ -1827,11 +1827,11 @@ mod tests { let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); - assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(error_counters.invalid_writable_account, 1); assert_eq!(loaded_accounts.len(), 1); assert_eq!( loaded_accounts[0], - (Err(TransactionError::InvalidAccountIndex), None) + (Err(TransactionError::InvalidWritableAccount), None) ); // Solution 1: include bpf_loader_upgradeable account @@ -1848,7 +1848,7 @@ mod tests { let loaded_accounts = load_accounts(tx, &accounts_with_upgradeable_loader, &mut error_counters); - assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(error_counters.invalid_writable_account, 1); assert_eq!(loaded_accounts.len(), 1); let result = loaded_accounts[0].0.as_ref().unwrap(); assert_eq!(result.accounts[..2], accounts_with_upgradeable_loader[..2]); @@ -1863,7 +1863,7 @@ mod tests { let tx = Transaction::new(&[&keypair], message, Hash::default()); let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); - assert_eq!(error_counters.invalid_account_index, 1); + assert_eq!(error_counters.invalid_writable_account, 1); assert_eq!(loaded_accounts.len(), 1); let result = loaded_accounts[0].0.as_ref().unwrap(); assert_eq!(result.accounts[..2], accounts[..2]); diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index 0b64513aa3873f..6b59d02300a532 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -175,6 +175,7 @@ pub struct ErrorCounters { pub invalid_account_index: usize, pub invalid_program_for_execution: usize, pub not_allowed_during_cluster_maintenance: usize, + pub invalid_writable_account: usize, } #[derive(Default, Debug)] diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index d3f1b1eda2f5ed..da7933f98278b8 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -189,7 +189,7 @@ impl ExecuteTimings { } type BankStatusCache = StatusCache>; -#[frozen_abi(digest = "GT81Hdwrh73i55ScQvFqmzeHfUL42yxuavZods8VyzGc")] +#[frozen_abi(digest = "5Br3PNyyX1L7XoS4jYLt5JTeMXowLSsu7v9LhokC8vnq")] pub type BankSlotDelta = SlotDelta>; type TransactionAccountRefCells = Vec<(Pubkey, Rc>)>; type TransactionLoaderRefCells = Vec>)>>; @@ -3119,6 +3119,12 @@ impl Bank { error_counters.not_allowed_during_cluster_maintenance ); } + if 0 != error_counters.invalid_writable_account { + inc_new_counter_info!( + "bank-process_transactions-error-invalid_writable_account", + error_counters.invalid_writable_account + ); + } } /// Converts Accounts into RefCell, this involves moving diff --git a/sdk/src/transaction/mod.rs b/sdk/src/transaction/mod.rs index c4cdcdcf6c71cd..57a6877acfe594 100644 --- a/sdk/src/transaction/mod.rs +++ b/sdk/src/transaction/mod.rs @@ -116,6 +116,10 @@ pub enum TransactionError { /// Transaction version is unsupported #[error("Transaction version is unsupported")] UnsupportedVersion, + + /// Transaction loads a writable account that cannot be written + #[error("Transaction loads a writable account that cannot be written")] + InvalidWritableAccount, } pub type Result = result::Result; diff --git a/storage-proto/proto/transaction_by_addr.proto b/storage-proto/proto/transaction_by_addr.proto index b8d93ccb325b01..0278d81d7c84c8 100644 --- a/storage-proto/proto/transaction_by_addr.proto +++ b/storage-proto/proto/transaction_by_addr.proto @@ -43,6 +43,7 @@ enum TransactionErrorType { ACCOUNT_BORROW_OUTSTANDING_TX = 16; WOULD_EXCEED_MAX_BLOCK_COST_LIMIT = 17; UNSUPPORTED_VERSION = 18; + INVALID_WRITABLE_ACCOUNT = 19; } message InstructionError { diff --git a/storage-proto/src/convert.rs b/storage-proto/src/convert.rs index c4361c12e5cc97..b0def567f445db 100644 --- a/storage-proto/src/convert.rs +++ b/storage-proto/src/convert.rs @@ -550,6 +550,7 @@ impl TryFrom for TransactionError { 16 => TransactionError::AccountBorrowOutstanding, 17 => TransactionError::WouldExceedMaxBlockCostLimit, 18 => TransactionError::UnsupportedVersion, + 19 => TransactionError::InvalidWritableAccount, _ => return Err("Invalid TransactionError"), }) } @@ -614,6 +615,9 @@ impl From for tx_by_addr::TransactionError { TransactionError::UnsupportedVersion => { tx_by_addr::TransactionErrorType::UnsupportedVersion } + TransactionError::InvalidWritableAccount => { + tx_by_addr::TransactionErrorType::InvalidWritableAccount + } } as i32, instruction_error: match transaction_error { TransactionError::InstructionError(index, ref instruction_error) => { From 9a5213eacc31460acc3eb6c0a9beaee06784473a Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Wed, 8 Sep 2021 09:47:44 -0500 Subject: [PATCH 7/7] Disallow writes to any upgradeable-loader account when loader not present; remove is_upgradeable_loader_present exception for all other executables --- runtime/src/accounts.rs | 144 ++++++++++++++++++++++++++++++---------- 1 file changed, 108 insertions(+), 36 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 4686c2ffb06794..ce912160331abc 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -276,16 +276,15 @@ impl Accounts { }) .unwrap_or_default(); - if account.executable() - && demote_program_write_locks - && message.is_writable(i, demote_program_write_locks) - && !is_upgradeable_loader_present - { - error_counters.invalid_writable_account += 1; - return Err(TransactionError::InvalidWritableAccount); - } - if bpf_loader_upgradeable::check_id(account.owner()) { + if demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) + && !is_upgradeable_loader_present + { + error_counters.invalid_writable_account += 1; + return Err(TransactionError::InvalidWritableAccount); + } + if account.executable() { // The upgradeable loader requires the derived ProgramData account if let Ok(UpgradeableLoaderState::Program { @@ -306,19 +305,13 @@ impl Accounts { error_counters.invalid_program_for_execution += 1; return Err(TransactionError::InvalidProgramForExecution); } - } else if demote_program_write_locks - && message.is_writable(i, demote_program_write_locks) - && !is_upgradeable_loader_present - { - // If a ProgramData account is present, it should only be writable - // if the bpf_loader_upgradeable account is also present - if let Ok(UpgradeableLoaderState::ProgramData { .. }) = - account.state() - { - error_counters.invalid_writable_account += 1; - return Err(TransactionError::InvalidWritableAccount); - } } + } else if account.executable() + && demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) + { + error_counters.invalid_writable_account += 1; + return Err(TransactionError::InvalidWritableAccount); } tx_rent += rent; @@ -1751,32 +1744,104 @@ mod tests { (Err(TransactionError::InvalidWritableAccount), None) ); - // Solution 1: include bpf_loader_upgradeable account + // Mark executables as readonly + message.account_keys = vec![key0, key1, key2]; // revert key change + message.header.num_readonly_unsigned_accounts = 2; // mark both executables as readonly + let tx = Transaction::new(&[&keypair], message, Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts[..2]); + assert_eq!(result.loaders[0], vec![accounts[2].clone()]); + } + + #[test] + fn test_load_accounts_upgradeable_with_write_lock() { + let mut accounts: Vec<(Pubkey, AccountSharedData)> = Vec::new(); + let mut error_counters = ErrorCounters::default(); + + let keypair = Keypair::new(); + let key0 = keypair.pubkey(); + let key1 = Pubkey::new(&[5u8; 32]); + let key2 = Pubkey::new(&[6u8; 32]); + let programdata_key1 = Pubkey::new(&[7u8; 32]); + let programdata_key2 = Pubkey::new(&[8u8; 32]); + + let mut account = AccountSharedData::new(1, 0, &Pubkey::default()); + account.set_rent_epoch(1); + accounts.push((key0, account)); + + let program_data = UpgradeableLoaderState::ProgramData { + slot: 42, + upgrade_authority_address: None, + }; + + let program = UpgradeableLoaderState::Program { + programdata_address: programdata_key1, + }; + let mut account = + AccountSharedData::new_data(40, &program, &bpf_loader_upgradeable::id()).unwrap(); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key1, account)); + let mut account = + AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap(); + account.set_rent_epoch(1); + accounts.push((programdata_key1, account)); + + let program = UpgradeableLoaderState::Program { + programdata_address: programdata_key2, + }; + let mut account = + AccountSharedData::new_data(40, &program, &bpf_loader_upgradeable::id()).unwrap(); + account.set_executable(true); + account.set_rent_epoch(1); + accounts.push((key2, account)); + let mut account = + AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap(); + account.set_rent_epoch(1); + accounts.push((programdata_key2, account)); + let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable account.set_executable(true); account.set_rent_epoch(1); - let accounts_with_upgradeable_loader = vec![ - accounts[0].clone(), - accounts[1].clone(), - (bpf_loader_upgradeable::id(), account), - ]; - message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()]; + accounts.push((bpf_loader_upgradeable::id(), account)); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let mut message = Message::new_with_compiled_instructions( + 1, + 0, + 1, // only one executable marked as readonly + vec![key0, key1, key2], + Hash::default(), + instructions, + ); let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); - let loaded_accounts = - load_accounts(tx, &accounts_with_upgradeable_loader, &mut error_counters); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); assert_eq!(error_counters.invalid_writable_account, 1); assert_eq!(loaded_accounts.len(), 1); - let result = loaded_accounts[0].0.as_ref().unwrap(); - assert_eq!(result.accounts[..2], accounts_with_upgradeable_loader[..2]); assert_eq!( - result.loaders[0], - vec![accounts_with_upgradeable_loader[2].clone()] + loaded_accounts[0], + (Err(TransactionError::InvalidWritableAccount), None) ); + // Solution 1: include bpf_loader_upgradeable account + message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()]; + let tx = Transaction::new(&[&keypair], message.clone(), Hash::default()); + let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); + + assert_eq!(error_counters.invalid_writable_account, 1); + assert_eq!(loaded_accounts.len(), 1); + let result = loaded_accounts[0].0.as_ref().unwrap(); + assert_eq!(result.accounts[..2], accounts[..2]); + assert_eq!(result.loaders[0], vec![accounts[5].clone()]); + // Solution 2: mark programdata as readonly message.account_keys = vec![key0, key1, key2]; // revert key change - message.header.num_readonly_unsigned_accounts = 2; // ark both executables as readonly + message.header.num_readonly_unsigned_accounts = 2; // mark both executables as readonly let tx = Transaction::new(&[&keypair], message, Hash::default()); let loaded_accounts = load_accounts(tx, &accounts, &mut error_counters); @@ -1784,7 +1849,14 @@ mod tests { assert_eq!(loaded_accounts.len(), 1); let result = loaded_accounts[0].0.as_ref().unwrap(); assert_eq!(result.accounts[..2], accounts[..2]); - assert_eq!(result.loaders[0], vec![accounts[2].clone()]); + assert_eq!( + result.loaders[0], + vec![ + accounts[5].clone(), + accounts[3].clone(), + accounts[4].clone() + ] + ); } #[test]