Fix system program blind derefs (#6317)

automerge
This commit is contained in:
Greg Fitzgerald 2019-10-10 17:43:49 -06:00 committed by Grimes
parent c6e4641781
commit ba46bc4624
2 changed files with 154 additions and 128 deletions

View File

@ -1,36 +1,38 @@
use log::*; use log::*;
use solana_sdk::account::KeyedAccount; use solana_sdk::account::KeyedAccount;
use solana_sdk::instruction::InstructionError; use solana_sdk::instruction::InstructionError;
use solana_sdk::instruction_processor_utils::next_keyed_account;
use solana_sdk::pubkey::Pubkey; use solana_sdk::pubkey::Pubkey;
use solana_sdk::system_instruction::{SystemError, SystemInstruction}; use solana_sdk::system_instruction::{SystemError, SystemInstruction};
use solana_sdk::system_program; use solana_sdk::system_program;
use solana_sdk::sysvar; use solana_sdk::sysvar;
const FROM_ACCOUNT_INDEX: usize = 0;
const TO_ACCOUNT_INDEX: usize = 1;
fn create_system_account( fn create_system_account(
keyed_accounts: &mut [KeyedAccount], from: &mut KeyedAccount,
to: &mut KeyedAccount,
lamports: u64, lamports: u64,
space: u64, space: u64,
program_id: &Pubkey, program_id: &Pubkey,
) -> Result<(), SystemError> { ) -> Result<(), InstructionError> {
if !system_program::check_id(&keyed_accounts[FROM_ACCOUNT_INDEX].account.owner) { if from.signer_key().is_none() {
debug!( debug!("from is unsigned");
"CreateAccount: invalid account[from] owner {} ", return Err(InstructionError::MissingRequiredSignature);
&keyed_accounts[FROM_ACCOUNT_INDEX].account.owner
);
return Err(SystemError::SourceNotSystemAccount);
} }
if !keyed_accounts[TO_ACCOUNT_INDEX].account.data.is_empty() if !system_program::check_id(&from.account.owner) {
|| !system_program::check_id(&keyed_accounts[TO_ACCOUNT_INDEX].account.owner) debug!(
{ "CreateAccount: invalid account[from] owner {} ",
&from.account.owner
);
return Err(SystemError::SourceNotSystemAccount.into());
}
if !to.account.data.is_empty() || !system_program::check_id(&to.account.owner) {
debug!( debug!(
"CreateAccount: invalid argument; account {} already in use", "CreateAccount: invalid argument; account {} already in use",
keyed_accounts[TO_ACCOUNT_INDEX].unsigned_key() to.unsigned_key()
); );
return Err(SystemError::AccountAlreadyInUse); return Err(SystemError::AccountAlreadyInUse.into());
} }
if sysvar::check_id(&program_id) { if sysvar::check_id(&program_id) {
@ -38,52 +40,67 @@ fn create_system_account(
"CreateAccount: invalid argument; program id {} invalid", "CreateAccount: invalid argument; program id {} invalid",
program_id program_id
); );
return Err(SystemError::InvalidProgramId); return Err(SystemError::InvalidProgramId.into());
} }
if sysvar::is_sysvar_id(&keyed_accounts[TO_ACCOUNT_INDEX].unsigned_key()) { if sysvar::is_sysvar_id(&to.unsigned_key()) {
debug!( debug!(
"CreateAccount: invalid argument; account id {} invalid", "CreateAccount: invalid argument; account id {} invalid",
program_id program_id
); );
return Err(SystemError::InvalidAccountId); return Err(SystemError::InvalidAccountId.into());
} }
if lamports > keyed_accounts[FROM_ACCOUNT_INDEX].account.lamports { if lamports > from.account.lamports {
debug!( debug!(
"CreateAccount: insufficient lamports ({}, need {})", "CreateAccount: insufficient lamports ({}, need {})",
keyed_accounts[FROM_ACCOUNT_INDEX].account.lamports, lamports from.account.lamports, lamports
); );
return Err(SystemError::ResultWithNegativeLamports); return Err(SystemError::ResultWithNegativeLamports.into());
} }
keyed_accounts[FROM_ACCOUNT_INDEX].account.lamports -= lamports; from.account.lamports -= lamports;
keyed_accounts[TO_ACCOUNT_INDEX].account.lamports += lamports; to.account.lamports += lamports;
keyed_accounts[TO_ACCOUNT_INDEX].account.owner = *program_id; to.account.owner = *program_id;
keyed_accounts[TO_ACCOUNT_INDEX].account.data = vec![0; space as usize]; to.account.data = vec![0; space as usize];
keyed_accounts[TO_ACCOUNT_INDEX].account.executable = false; to.account.executable = false;
Ok(()) Ok(())
} }
fn assign_account_to_program( fn assign_account_to_program(
keyed_accounts: &mut [KeyedAccount], account: &mut KeyedAccount,
program_id: &Pubkey, program_id: &Pubkey,
) -> Result<(), SystemError> { ) -> Result<(), InstructionError> {
keyed_accounts[FROM_ACCOUNT_INDEX].account.owner = *program_id; if !system_program::check_id(&account.account.owner) {
return Err(InstructionError::IncorrectProgramId);
}
if account.signer_key().is_none() {
debug!("account is unsigned");
return Err(InstructionError::MissingRequiredSignature);
}
account.account.owner = *program_id;
Ok(()) Ok(())
} }
fn transfer_lamports( fn transfer_lamports(
keyed_accounts: &mut [KeyedAccount], from: &mut KeyedAccount,
to: &mut KeyedAccount,
lamports: u64, lamports: u64,
) -> Result<(), SystemError> { ) -> Result<(), InstructionError> {
if lamports > keyed_accounts[FROM_ACCOUNT_INDEX].account.lamports { if from.signer_key().is_none() {
debug!("from is unsigned");
return Err(InstructionError::MissingRequiredSignature);
}
if lamports > from.account.lamports {
debug!( debug!(
"Transfer: insufficient lamports ({}, need {})", "Transfer: insufficient lamports ({}, need {})",
keyed_accounts[FROM_ACCOUNT_INDEX].account.lamports, lamports from.account.lamports, lamports
); );
return Err(SystemError::ResultWithNegativeLamports); return Err(SystemError::ResultWithNegativeLamports.into());
} }
keyed_accounts[FROM_ACCOUNT_INDEX].account.lamports -= lamports; from.account.lamports -= lamports;
keyed_accounts[TO_ACCOUNT_INDEX].account.lamports += lamports; to.account.lamports += lamports;
Ok(()) Ok(())
} }
@ -92,43 +109,33 @@ pub fn process_instruction(
keyed_accounts: &mut [KeyedAccount], keyed_accounts: &mut [KeyedAccount],
data: &[u8], data: &[u8],
) -> Result<(), InstructionError> { ) -> Result<(), InstructionError> {
if let Ok(instruction) = bincode::deserialize(data) { let instruction =
trace!("process_instruction: {:?}", instruction); bincode::deserialize(data).map_err(|_| InstructionError::InvalidInstructionData)?;
trace!("keyed_accounts: {:?}", keyed_accounts);
if keyed_accounts.is_empty() { trace!("process_instruction: {:?}", instruction);
debug!("Invalid instruction data: {:?}", data); trace!("keyed_accounts: {:?}", keyed_accounts);
return Err(InstructionError::NotEnoughAccountKeys);
}
// All system instructions require that accounts_keys[0] be a signer
if keyed_accounts[FROM_ACCOUNT_INDEX].signer_key().is_none() {
debug!("account[from] is unsigned");
return Err(InstructionError::MissingRequiredSignature);
}
match instruction { let keyed_accounts_iter = &mut keyed_accounts.iter_mut();
SystemInstruction::CreateAccount {
lamports, match instruction {
space, SystemInstruction::CreateAccount {
program_id, lamports,
} if keyed_accounts.len() >= 2 => { space,
create_system_account(keyed_accounts, lamports, space, &program_id) program_id,
} } => {
SystemInstruction::Assign { program_id } => { let from = next_keyed_account(keyed_accounts_iter)?;
if !system_program::check_id(&keyed_accounts[FROM_ACCOUNT_INDEX].account.owner) { let to = next_keyed_account(keyed_accounts_iter)?;
return Err(InstructionError::IncorrectProgramId); create_system_account(from, to, lamports, space, &program_id)
} }
assign_account_to_program(keyed_accounts, &program_id) SystemInstruction::Assign { program_id } => {
} let account = next_keyed_account(keyed_accounts_iter)?;
SystemInstruction::Transfer { lamports } if keyed_accounts.len() >= 2 => { assign_account_to_program(account, &program_id)
transfer_lamports(keyed_accounts, lamports) }
} SystemInstruction::Transfer { lamports } => {
_ => return Err(InstructionError::NotEnoughAccountKeys), let from = next_keyed_account(keyed_accounts_iter)?;
let to = next_keyed_account(keyed_accounts_iter)?;
transfer_lamports(from, to, lamports)
} }
.map_err(|e| InstructionError::CustomError(e as u32))
} else {
debug!("Invalid instruction data: {:?}", data);
Err(InstructionError::InvalidInstructionData)
} }
} }
@ -155,11 +162,14 @@ mod tests {
let to = Pubkey::new_rand(); let to = Pubkey::new_rand();
let mut to_account = Account::new(0, 0, &Pubkey::default()); let mut to_account = Account::new(0, 0, &Pubkey::default());
let mut keyed_accounts = [ create_system_account(
KeyedAccount::new(&from, true, &mut from_account), &mut KeyedAccount::new(&from, true, &mut from_account),
KeyedAccount::new(&to, false, &mut to_account), &mut KeyedAccount::new(&to, false, &mut to_account),
]; 50,
create_system_account(&mut keyed_accounts, 50, 2, &new_program_owner).unwrap(); 2,
&new_program_owner,
)
.unwrap();
let from_lamports = from_account.lamports; let from_lamports = from_account.lamports;
let to_lamports = to_account.lamports; let to_lamports = to_account.lamports;
let to_owner = to_account.owner; let to_owner = to_account.owner;
@ -181,12 +191,14 @@ mod tests {
let mut to_account = Account::new(0, 0, &Pubkey::default()); let mut to_account = Account::new(0, 0, &Pubkey::default());
let unchanged_account = to_account.clone(); let unchanged_account = to_account.clone();
let mut keyed_accounts = [ let result = create_system_account(
KeyedAccount::new(&from, true, &mut from_account), &mut KeyedAccount::new(&from, true, &mut from_account),
KeyedAccount::new(&to, false, &mut to_account), &mut KeyedAccount::new(&to, false, &mut to_account),
]; 150,
let result = create_system_account(&mut keyed_accounts, 150, 2, &new_program_owner); 2,
assert_eq!(result, Err(SystemError::ResultWithNegativeLamports)); &new_program_owner,
);
assert_eq!(result, Err(SystemError::ResultWithNegativeLamports.into()));
let from_lamports = from_account.lamports; let from_lamports = from_account.lamports;
assert_eq!(from_lamports, 100); assert_eq!(from_lamports, 100);
assert_eq!(to_account, unchanged_account); assert_eq!(to_account, unchanged_account);
@ -204,12 +216,14 @@ mod tests {
let mut owned_account = Account::new(0, 0, &original_program_owner); let mut owned_account = Account::new(0, 0, &original_program_owner);
let unchanged_account = owned_account.clone(); let unchanged_account = owned_account.clone();
let mut keyed_accounts = [ let result = create_system_account(
KeyedAccount::new(&from, true, &mut from_account), &mut KeyedAccount::new(&from, true, &mut from_account),
KeyedAccount::new(&owned_key, false, &mut owned_account), &mut KeyedAccount::new(&owned_key, false, &mut owned_account),
]; 50,
let result = create_system_account(&mut keyed_accounts, 50, 2, &new_program_owner); 2,
assert_eq!(result, Err(SystemError::AccountAlreadyInUse)); &new_program_owner,
);
assert_eq!(result, Err(SystemError::AccountAlreadyInUse.into()));
let from_lamports = from_account.lamports; let from_lamports = from_account.lamports;
assert_eq!(from_lamports, 100); assert_eq!(from_lamports, 100);
assert_eq!(owned_account, unchanged_account); assert_eq!(owned_account, unchanged_account);
@ -224,24 +238,28 @@ mod tests {
let to = Pubkey::new_rand(); let to = Pubkey::new_rand();
let mut to_account = Account::default(); let mut to_account = Account::default();
let mut keyed_accounts = [
KeyedAccount::new(&from, true, &mut from_account),
KeyedAccount::new(&to, false, &mut to_account),
];
// fail to create a sysvar::id() owned account // fail to create a sysvar::id() owned account
let result = create_system_account(&mut keyed_accounts, 50, 2, &sysvar::id()); let result = create_system_account(
assert_eq!(result, Err(SystemError::InvalidProgramId)); &mut KeyedAccount::new(&from, true, &mut from_account),
&mut KeyedAccount::new(&to, false, &mut to_account),
50,
2,
&sysvar::id(),
);
assert_eq!(result, Err(SystemError::InvalidProgramId.into()));
let to = sysvar::fees::id(); let to = sysvar::fees::id();
let mut to_account = Account::default(); let mut to_account = Account::default();
let mut keyed_accounts = [
KeyedAccount::new(&from, true, &mut from_account),
KeyedAccount::new(&to, false, &mut to_account),
];
// fail to create an account with a sysvar id // fail to create an account with a sysvar id
let result = create_system_account(&mut keyed_accounts, 50, 2, &system_program::id()); let result = create_system_account(
assert_eq!(result, Err(SystemError::InvalidAccountId)); &mut KeyedAccount::new(&from, true, &mut from_account),
&mut KeyedAccount::new(&to, false, &mut to_account),
50,
2,
&system_program::id(),
);
assert_eq!(result, Err(SystemError::InvalidAccountId.into()));
let from_lamports = from_account.lamports; let from_lamports = from_account.lamports;
assert_eq!(from_lamports, 100); assert_eq!(from_lamports, 100);
@ -261,12 +279,14 @@ mod tests {
}; };
let unchanged_account = populated_account.clone(); let unchanged_account = populated_account.clone();
let mut keyed_accounts = [ let result = create_system_account(
KeyedAccount::new(&from, true, &mut from_account), &mut KeyedAccount::new(&from, true, &mut from_account),
KeyedAccount::new(&populated_key, false, &mut populated_account), &mut KeyedAccount::new(&populated_key, false, &mut populated_account),
]; 50,
let result = create_system_account(&mut keyed_accounts, 50, 2, &new_program_owner); 2,
assert_eq!(result, Err(SystemError::AccountAlreadyInUse)); &new_program_owner,
);
assert_eq!(result, Err(SystemError::AccountAlreadyInUse.into()));
assert_eq!(from_account.lamports, 100); assert_eq!(from_account.lamports, 100);
assert_eq!(populated_account, unchanged_account); assert_eq!(populated_account, unchanged_account);
} }
@ -279,12 +299,14 @@ mod tests {
let mut from_account = Account::new(100, 0, &other_program); let mut from_account = Account::new(100, 0, &other_program);
let to = Pubkey::new_rand(); let to = Pubkey::new_rand();
let mut to_account = Account::new(0, 0, &Pubkey::default()); let mut to_account = Account::new(0, 0, &Pubkey::default());
let mut keyed_accounts = [ let result = create_system_account(
KeyedAccount::new(&from, true, &mut from_account), &mut KeyedAccount::new(&from, true, &mut from_account),
KeyedAccount::new(&to, false, &mut to_account), &mut KeyedAccount::new(&to, false, &mut to_account),
]; 50,
let result = create_system_account(&mut keyed_accounts, 50, 2, &other_program); 2,
assert_eq!(result, Err(SystemError::SourceNotSystemAccount)); &other_program,
);
assert_eq!(result, Err(SystemError::SourceNotSystemAccount.into()));
} }
#[test] #[test]
@ -293,14 +315,17 @@ mod tests {
let from = Pubkey::new_rand(); let from = Pubkey::new_rand();
let mut from_account = Account::new(100, 0, &system_program::id()); let mut from_account = Account::new(100, 0, &system_program::id());
let mut keyed_accounts = [KeyedAccount::new(&from, true, &mut from_account)]; assign_account_to_program(
assign_account_to_program(&mut keyed_accounts, &new_program_owner).unwrap(); &mut KeyedAccount::new(&from, true, &mut from_account),
&new_program_owner,
)
.unwrap();
let from_owner = from_account.owner; let from_owner = from_account.owner;
assert_eq!(from_owner, new_program_owner); assert_eq!(from_owner, new_program_owner);
// Attempt to assign account not owned by system program // Attempt to assign account not owned by system program
let another_program_owner = Pubkey::new(&[8; 32]); let another_program_owner = Pubkey::new(&[8; 32]);
keyed_accounts = [KeyedAccount::new(&from, true, &mut from_account)]; let mut keyed_accounts = [KeyedAccount::new(&from, true, &mut from_account)];
let instruction = SystemInstruction::Assign { let instruction = SystemInstruction::Assign {
program_id: another_program_owner, program_id: another_program_owner,
}; };
@ -339,23 +364,24 @@ mod tests {
let mut from_account = Account::new(100, 0, &Pubkey::new(&[2; 32])); // account owner should not matter let mut from_account = Account::new(100, 0, &Pubkey::new(&[2; 32])); // account owner should not matter
let to = Pubkey::new_rand(); let to = Pubkey::new_rand();
let mut to_account = Account::new(1, 0, &Pubkey::new(&[3; 32])); // account owner should not matter let mut to_account = Account::new(1, 0, &Pubkey::new(&[3; 32])); // account owner should not matter
let mut keyed_accounts = [ transfer_lamports(
KeyedAccount::new(&from, true, &mut from_account), &mut KeyedAccount::new(&from, true, &mut from_account),
KeyedAccount::new_credit_only(&to, false, &mut to_account), &mut KeyedAccount::new_credit_only(&to, false, &mut to_account),
]; 50,
transfer_lamports(&mut keyed_accounts, 50).unwrap(); )
.unwrap();
let from_lamports = from_account.lamports; let from_lamports = from_account.lamports;
let to_lamports = to_account.lamports; let to_lamports = to_account.lamports;
assert_eq!(from_lamports, 50); assert_eq!(from_lamports, 50);
assert_eq!(to_lamports, 51); assert_eq!(to_lamports, 51);
// Attempt to move more lamports than remaining in from_account // Attempt to move more lamports than remaining in from_account
keyed_accounts = [ let result = transfer_lamports(
KeyedAccount::new(&from, true, &mut from_account), &mut KeyedAccount::new(&from, true, &mut from_account),
KeyedAccount::new_credit_only(&to, false, &mut to_account), &mut KeyedAccount::new_credit_only(&to, false, &mut to_account),
]; 100,
let result = transfer_lamports(&mut keyed_accounts, 100); );
assert_eq!(result, Err(SystemError::ResultWithNegativeLamports)); assert_eq!(result, Err(SystemError::ResultWithNegativeLamports.into()));
assert_eq!(from_account.lamports, 50); assert_eq!(from_account.lamports, 50);
assert_eq!(to_account.lamports, 51); assert_eq!(to_account.lamports, 51);
} }

View File

@ -2,9 +2,9 @@ use crate::instruction::{AccountMeta, Instruction};
use crate::instruction_processor_utils::DecodeError; use crate::instruction_processor_utils::DecodeError;
use crate::pubkey::Pubkey; use crate::pubkey::Pubkey;
use crate::system_program; use crate::system_program;
use num_derive::FromPrimitive; use num_derive::{FromPrimitive, ToPrimitive};
#[derive(Serialize, Debug, Clone, PartialEq, FromPrimitive)] #[derive(Serialize, Debug, Clone, PartialEq, FromPrimitive, ToPrimitive)]
pub enum SystemError { pub enum SystemError {
AccountAlreadyInUse, AccountAlreadyInUse,
ResultWithNegativeLamports, ResultWithNegativeLamports,