gaspersic/solana
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Return error if Transaction contains writable executable or ProgramData accounts (backport #19629) (#19729)

Browse Source 
* Return error if Transaction contains writable executable or ProgramData accounts (#19629)

* Return error if Transaction locks an executable as writable

* Return error if a ProgramData account is writable but the upgradable loader isn't present

* Remove unreachable clause

* Fixup bpf tests

* Review comments

* Add new TransactionError

* Disallow writes to any upgradeable-loader account when loader not present; remove is_upgradeable_loader_present exception for all other executables

(cherry picked from commit 38bbb77989)

# Conflicts:
#	programs/bpf/tests/programs.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	sdk/src/transaction.rs
#	storage-proto/proto/transaction_by_addr.proto
#	storage-proto/src/convert.rs

* Fix conflicts

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>
Co-authored-by: Tyera Eulberg <tyera@solana.com>
This commit is contained in:
mergify[bot]
2021-09-09 03:06:02 +00:00
committed by GitHub
parent a78c3c07f2
commit 3eee222667
7 changed files with 362 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
programs/bpf/tests/programs.rs
View File

@ -1842,10 +1842,10 @@ 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(fees::id(), false),
AccountMeta::new_readonly(program_id, false),
AccountMeta::new_readonly(program_id, false),
AccountMeta::new_readonly(clock::id(), false),
AccountMeta::new_readonly(fees::id(), false),
],
);
@ -2022,10 +2022,10 @@ 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(fees::id(), false),
AccountMeta::new_readonly(program_id, false),
AccountMeta::new_readonly(program_id, false),
AccountMeta::new_readonly(clock::id(), false),
AccountMeta::new_readonly(fees::id(), false),
],
);

339
runtime/src/accounts.rs
View File

@ -206,6 +206,7 @@ impl Accounts {
let mut rent_debits = RentDebits::default();
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(key, i) {
@ -216,9 +217,6 @@ impl Accounts {
if solana_sdk::sysvar::instructions::check_id(key)
&& feature_set.is_active(&feature_set::instructions_sysvar_enabled::id())
{
if message.is_writable(i, demote_program_write_locks) {
return Err(TransactionError::InvalidAccountIndex);
}
Self::construct_instructions_account(message, demote_program_write_locks)
} else {
let (account, rent) = self
@ -235,26 +233,42 @@ impl Accounts {
})
.unwrap_or_default();
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 bpf_loader_upgradeable::check_id(&account.owner) {
if demote_program_write_locks
&& message.is_writable(i, demote_program_write_locks)
&& !is_upgradeable_loader_present
{
if let Some(account) = self
.accounts_db
.load(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.invalid_program_for_execution += 1;
return Err(TransactionError::InvalidProgramForExecution);
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 {
programdata_address,
}) = account.state()
{
if let Some(account) = self
.accounts_db
.load(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.invalid_program_for_execution += 1;
return Err(TransactionError::InvalidProgramForExecution);
}
}
} 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;
@ -1049,6 +1063,13 @@ pub fn prepare_if_nonce_account(
false
}
fn is_upgradeable_loader_present(message: &Message) -> bool {
message
.account_keys
.iter()
.any(|&key| key == bpf_loader_upgradeable::id())
}
pub fn create_test_accounts(
accounts: &Accounts,
pubkeys: &mut Vec<Pubkey>,
@ -1668,6 +1689,282 @@ 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_writable_account, 1);
assert_eq!(loaded_accounts.len(), 1);
assert_eq!(
loaded_accounts[0],
(Err(TransactionError::InvalidWritableAccount), None)
);
// 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
.iter()
.map(|(_, account)| account)
.cloned()
.collect::<Vec<AccountSharedData>>()[..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);
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, &mut error_counters);
assert_eq!(error_counters.invalid_writable_account, 1);
assert_eq!(loaded_accounts.len(), 1);
assert_eq!(
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
.iter()
.map(|(_, account)| account)
.cloned()
.collect::<Vec<AccountSharedData>>()[..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; // 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
.iter()
.map(|(_, account)| account)
.cloned()
.collect::<Vec<AccountSharedData>>()[..2]
);
assert_eq!(
result.loaders[0],
vec![
accounts[5].clone(),
accounts[3].clone(),
accounts[4].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_writable_account, 1);
assert_eq!(loaded_accounts.len(), 1);
assert_eq!(
loaded_accounts[0],
(Err(TransactionError::InvalidWritableAccount), 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_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
.iter()
.map(|(_, account)| account)
.cloned()
.collect::<Vec<AccountSharedData>>()[..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_writable_account, 1);
assert_eq!(loaded_accounts.len(), 1);
let result = loaded_accounts[0].0.as_ref().unwrap();
assert_eq!(
result.accounts[..2],
accounts
.iter()
.map(|(_, account)| account)
.cloned()
.collect::<Vec<AccountSharedData>>()[..2]
);
assert_eq!(result.loaders[0], vec![accounts[2].clone()]);
}
#[test]
fn test_accounts_account_not_found() {
let accounts = Accounts::new_with_config(

1
runtime/src/accounts_db.rs
View File

@ -122,6 +122,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, PartialEq, Clone)]

8
runtime/src/bank.rs
View File

@ -144,7 +144,7 @@ impl ExecuteTimings {
}
type BankStatusCache = StatusCache<Result<()>>;
#[frozen_abi(digest = "HhY4tMP5KZU9fw9VLpMMUikfvNVCLksocZBUKjt8ZjYH")]
#[frozen_abi(digest = "5Br3PNyyX1L7XoS4jYLt5JTeMXowLSsu7v9LhokC8vnq")]
pub type BankSlotDelta = SlotDelta<Result<()>>;
type TransactionAccountRefCells = Vec<Rc<RefCell<AccountSharedData>>>;
type TransactionAccountDepRefCells = Vec<(Pubkey, Rc<RefCell<AccountSharedData>>)>;
@ -2813,6 +2813,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<AccountSharedData>, this involves moving

13
sdk/src/transaction.rs
View File

@ -99,6 +99,19 @@ pub enum TransactionError {
/// Transaction processing left an account with an outstanding borrowed reference
#[error("Transaction processing left an account with an outstanding borrowed reference")]
AccountBorrowOutstanding,
#[error(
"Transaction could not fit into current block without exceeding the Max Block Cost Limit"
)]
WouldExceedMaxBlockCostLimit,
/// 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<T> = result::Result<T, TransactionError>;

3
storage-proto/proto/transaction_by_addr.proto
View File

@ -41,6 +41,9 @@ enum TransactionErrorType {
SANITIZE_FAILURE = 14;
CLUSTER_MAINTENANCE = 15;
ACCOUNT_BORROW_OUTSTANDING_TX = 16;
WOULD_EXCEED_MAX_BLOCK_COST_LIMIT = 17;
UNSUPPORTED_VERSION = 18;
INVALID_WRITABLE_ACCOUNT = 19;
}
message InstructionError {

12
storage-proto/src/convert.rs
View File

@ -548,6 +548,9 @@ impl TryFrom<tx_by_addr::TransactionError> for TransactionError {
14 => TransactionError::SanitizeFailure,
15 => TransactionError::ClusterMaintenance,
16 => TransactionError::AccountBorrowOutstanding,
17 => TransactionError::WouldExceedMaxBlockCostLimit,
18 => TransactionError::UnsupportedVersion,
19 => TransactionError::InvalidWritableAccount,
_ => return Err("Invalid TransactionError"),
})
}
@ -606,6 +609,15 @@ impl From<TransactionError> for tx_by_addr::TransactionError {
TransactionError::AccountBorrowOutstanding => {
tx_by_addr::TransactionErrorType::AccountBorrowOutstandingTx
}
TransactionError::WouldExceedMaxBlockCostLimit => {
tx_by_addr::TransactionErrorType::WouldExceedMaxBlockCostLimit
}
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) => {