Detect and reject invalid AccountInfo reallocations

This commit is contained in:
Justin Starry 2022-06-27 11:01:01 +01:00
parent 826f3312cc
commit f44fcd1880
10 changed files with 216 additions and 28 deletions

View File

@ -14,6 +14,7 @@ pub const ASSIGN_TO_SELF_VIA_SYSTEM_PROGRAM_AND_REALLOC: u8 = 6;
pub const DEALLOC_AND_ASSIGN_TO_CALLER: u8 = 7;
pub const CHECK: u8 = 8;
pub const ZERO_INIT: u8 = 9;
pub const REALLOC_EXTEND_AND_UNDO: u8 = 10;
pub fn realloc(program_id: &Pubkey, address: &Pubkey, size: usize, bump: &mut u8) -> Instruction {
let mut instruction_data = vec![REALLOC, *bump];
@ -69,3 +70,21 @@ pub fn realloc_extend_and_fill(
vec![AccountMeta::new(*address, false)],
)
}
pub fn realloc_extend_and_undo(
program_id: &Pubkey,
address: &Pubkey,
size: usize,
bump: &mut u8,
) -> Instruction {
let mut instruction_data = vec![REALLOC_EXTEND_AND_UNDO, *bump];
instruction_data.extend_from_slice(&size.to_le_bytes());
*bump = bump.saturating_add(1);
Instruction::new_with_bytes(
*program_id,
&instruction_data,
vec![AccountMeta::new(*address, false)],
)
}

View File

@ -41,6 +41,16 @@ fn process_instruction(
account.realloc(new_len, false)?;
assert_eq!(new_len, account.data_len());
}
REALLOC_EXTEND_AND_UNDO => {
let pre_len = account.data_len();
let (bytes, _) = instruction_data[2..].split_at(std::mem::size_of::<usize>());
let new_len = pre_len.saturating_add(usize::from_le_bytes(bytes.try_into().unwrap()));
msg!("realloc extend by {}", new_len);
account.realloc(new_len, false)?;
msg!("undo realloc");
account.realloc(pre_len, false)?;
assert_eq!(pre_len, account.data_len());
}
REALLOC_EXTEND_AND_FILL => {
let pre_len = account.data_len();
let fill = instruction_data[2];

View File

@ -16,3 +16,4 @@ pub const INVOKE_DEALLOC_AND_ASSIGN: u8 = 12;
pub const INVOKE_REALLOC_MAX_TWICE: u8 = 13;
pub const INVOKE_REALLOC_MAX_INVOKE_MAX: u8 = 14;
pub const INVOKE_INVOKE_MAX_TWICE: u8 = 15;
pub const INVOKE_REALLOC_TO_THEN_LOCAL_REALLOC_EXTEND: u8 = 16;

View File

@ -74,6 +74,32 @@ fn process_instruction(
account.data_len()
);
}
INVOKE_REALLOC_TO_THEN_LOCAL_REALLOC_EXTEND => {
let (bytes, remaining_data) =
instruction_data[2..].split_at(std::mem::size_of::<usize>());
let new_len = usize::from_le_bytes(bytes.try_into().unwrap());
msg!("invoke realloc to {} byte(s)", new_len);
let realloc_to_ix = {
let mut instruction_data = vec![INVOKE_REALLOC_TO, 1];
instruction_data.extend_from_slice(&new_len.to_le_bytes());
Instruction::new_with_bytes(
*invoke_program_id,
&instruction_data,
vec![
AccountMeta::new(*account.key, false),
AccountMeta::new_readonly(*invoke_program_id, false),
],
)
};
invoke(&realloc_to_ix, accounts)?;
assert_eq!(new_len, account.data_len());
let (bytes, _) = remaining_data.split_at(std::mem::size_of::<usize>());
let extend_len = usize::from_le_bytes(bytes.try_into().unwrap());
msg!("realloc extend {} byte(s)", extend_len);
account.realloc(new_len.saturating_add(extend_len), false)?;
assert_eq!(new_len.saturating_add(extend_len), account.data_len());
}
INVOKE_REALLOC_MAX_TWICE => {
msg!("invoke realloc max twice");
invoke(

View File

@ -2814,6 +2814,44 @@ fn test_program_bpf_realloc() {
let data = bank_client.get_account_data(&pubkey).unwrap().unwrap();
assert_eq!(0, data.len());
// Realloc account to max then undo
bank_client
.send_and_confirm_message(
signer,
Message::new(
&[realloc_extend_and_undo(
&program_id,
&pubkey,
MAX_PERMITTED_DATA_INCREASE,
&mut bump,
)],
Some(&mint_pubkey),
),
)
.unwrap();
let data = bank_client.get_account_data(&pubkey).unwrap().unwrap();
assert_eq!(0, data.len());
// Realloc account to max + 1 then undo
assert_eq!(
bank_client
.send_and_confirm_message(
signer,
Message::new(
&[realloc_extend_and_undo(
&program_id,
&pubkey,
MAX_PERMITTED_DATA_INCREASE + 1,
&mut bump,
)],
Some(&mint_pubkey),
),
)
.unwrap_err()
.unwrap(),
TransactionError::InstructionError(0, InstructionError::InvalidRealloc)
);
// Realloc to max + 1
assert_eq!(
bank_client
@ -3356,6 +3394,51 @@ fn test_program_bpf_realloc_invoke() {
TransactionError::InstructionError(0, InstructionError::InvalidRealloc)
);
// CPI realloc extend then local realloc extend
for (cpi_extend_bytes, local_extend_bytes, should_succeed) in [
(0, 0, true),
(MAX_PERMITTED_DATA_INCREASE, 0, true),
(0, MAX_PERMITTED_DATA_INCREASE, true),
(MAX_PERMITTED_DATA_INCREASE, 1, false),
(1, MAX_PERMITTED_DATA_INCREASE, false),
] {
let invoke_account = AccountSharedData::new(100_000_000, 0, &realloc_invoke_program_id);
bank.store_account(&invoke_pubkey, &invoke_account);
let mut instruction_data = vec![];
instruction_data.extend_from_slice(&[INVOKE_REALLOC_TO_THEN_LOCAL_REALLOC_EXTEND, 1]);
instruction_data.extend_from_slice(&cpi_extend_bytes.to_le_bytes());
instruction_data.extend_from_slice(&local_extend_bytes.to_le_bytes());
let result = bank_client.send_and_confirm_message(
signer,
Message::new(
&[Instruction::new_with_bytes(
realloc_invoke_program_id,
&instruction_data,
vec![
AccountMeta::new(invoke_pubkey, false),
AccountMeta::new_readonly(realloc_invoke_program_id, false),
],
)],
Some(&mint_pubkey),
),
);
if should_succeed {
assert!(
result.is_ok(),
"cpi: {cpi_extend_bytes} local: {local_extend_bytes}, err: {:?}",
result.err()
);
} else {
assert_eq!(
result.unwrap_err().unwrap(),
TransactionError::InstructionError(0, InstructionError::InvalidRealloc),
"cpi: {cpi_extend_bytes} local: {local_extend_bytes}",
);
}
}
// Realloc invoke max twice
let invoke_account = AccountSharedData::new(42, 0, &realloc_program_id);
bank.store_account(&invoke_pubkey, &invoke_account);

View File

@ -205,7 +205,7 @@ pub fn serialize_parameters_aligned(
size += size_of::<u8>() // is_signer
+ size_of::<u8>() // is_writable
+ size_of::<u8>() // executable
+ 4 // padding to 128-bit aligned
+ size_of::<u32>() // original_data_len
+ size_of::<Pubkey>() // key
+ size_of::<Pubkey>() // owner
+ size_of::<u64>() // lamports
@ -300,7 +300,7 @@ pub fn deserialize_parameters_aligned(
start += size_of::<u8>() // is_signer
+ size_of::<u8>() // is_writable
+ size_of::<u8>() // executable
+ 4 // padding to 128-bit aligned
+ size_of::<u32>() // original_data_len
+ size_of::<Pubkey>(); // key
let _ = borrowed_account.set_owner(
buffer

View File

@ -2,13 +2,14 @@
use {
crate::{
clock::Epoch, debug_account_data::*, program_error::ProgramError,
program_memory::sol_memset, pubkey::Pubkey,
clock::Epoch, debug_account_data::*, entrypoint::MAX_PERMITTED_DATA_INCREASE,
program_error::ProgramError, program_memory::sol_memset, pubkey::Pubkey,
},
std::{
cell::{Ref, RefCell, RefMut},
fmt,
rc::Rc,
slice::from_raw_parts_mut,
},
};
@ -72,6 +73,19 @@ impl<'a> AccountInfo<'a> {
Ok(**self.try_borrow_lamports()?)
}
/// Return the account's original data length when it was serialized for the
/// current program invocation.
///
/// # Safety
///
/// This method assumes that the original data length was serialized as a u32
/// integer in the 4 bytes immediately preceding the serialized account key.
pub unsafe fn original_data_len(&self) -> usize {
let key_ptr = self.key as *const _ as *const u8;
let original_data_len_ptr = key_ptr.offset(-4) as *const u32;
*original_data_len_ptr as usize
}
pub fn data_len(&self) -> usize {
self.data.borrow().len()
}
@ -123,27 +137,45 @@ impl<'a> AccountInfo<'a> {
/// call a program reallocs from larger to smaller and back to larger again
/// the new space could contain stale data. Pass `true` for `zero_init` in
/// this case, otherwise compute units will be wasted re-zero-initializing.
///
/// # Safety
///
/// This method makes assumptions about the layout and location of memory
/// referenced by `AccountInfo` fields. It should only be called for
/// instances of `AccountInfo` that were created by the runtime and received
/// in the `process_instruction` entrypoint of a program.
pub fn realloc(&self, new_len: usize, zero_init: bool) -> Result<(), ProgramError> {
let orig_len = self.data_len();
let mut data = self.try_borrow_mut_data()?;
let old_len = data.len();
// Return early if length hasn't changed
if new_len == old_len {
return Ok(());
}
// Return early if the length increase from the original serialized data
// length is too large and would result in an out of bounds allocation.
let original_data_len = unsafe { self.original_data_len() };
if new_len.saturating_sub(original_data_len) > MAX_PERMITTED_DATA_INCREASE {
return Err(ProgramError::InvalidRealloc);
}
// realloc
unsafe {
// First set new length in the serialized data
let ptr = self.try_borrow_mut_data()?.as_mut_ptr().offset(-8) as *mut u64;
*ptr = new_len as u64;
let data_ptr = data.as_mut_ptr();
// Then set the new length in the local slice
let ptr = &mut *(((self.data.as_ptr() as *const u64).offset(1) as u64) as *mut u64);
*ptr = new_len as u64;
// First set new length in the serialized data
*(data_ptr.offset(-8) as *mut u64) = new_len as u64;
// Then recreate the local slice with the new length
*data = from_raw_parts_mut(data_ptr, new_len)
}
// zero-init if requested
if zero_init && new_len > orig_len {
sol_memset(
&mut self.try_borrow_mut_data()?[orig_len..],
0,
new_len.saturating_sub(orig_len),
);
if zero_init {
let len_increase = new_len.saturating_sub(old_len);
if len_increase > 0 {
sol_memset(&mut data[old_len..], 0, len_increase);
}
}
Ok(())

View File

@ -296,7 +296,11 @@ pub unsafe fn deserialize<'a>(input: *mut u8) -> (&'a Pubkey, Vec<AccountInfo<'a
let executable = *(input.add(offset) as *const u8) != 0;
offset += size_of::<u8>();
offset += size_of::<u32>(); // padding to u64
// The original data length is stored here because these 4 bytes were
// originally only used for padding and served as a good location to
// track the original size of the account data in a compatible way.
let original_data_len_offset = offset;
offset += size_of::<u32>();
let key: &Pubkey = &*(input.add(offset) as *const Pubkey);
offset += size_of::<Pubkey>();
@ -312,6 +316,10 @@ pub unsafe fn deserialize<'a>(input: *mut u8) -> (&'a Pubkey, Vec<AccountInfo<'a
let data_len = *(input.add(offset) as *const u64) as usize;
offset += size_of::<u64>();
// Store the original data length for detecting invalid reallocations and
// requires that MAX_PERMITTED_DATA_LENGTH fits in a u32
*(input.add(original_data_len_offset) as *mut u32) = data_len as u32;
let data = Rc::new(RefCell::new({
from_raw_parts_mut(input.add(offset), data_len)
}));

View File

@ -53,8 +53,8 @@ pub enum ProgramError {
IllegalOwner,
#[error("Account data allocation exceeded the maximum accounts data size limit")]
MaxAccountsDataSizeExceeded,
#[error("Cannot close vote account unless it stopped voting at least one full epoch ago")]
ActiveVoteAccountClose,
#[error("Account data reallocation was invalid")]
InvalidRealloc,
}
pub trait PrintProgramError {
@ -94,7 +94,7 @@ impl PrintProgramError for ProgramError {
Self::UnsupportedSysvar => msg!("Error: UnsupportedSysvar"),
Self::IllegalOwner => msg!("Error: IllegalOwner"),
Self::MaxAccountsDataSizeExceeded => msg!("Error: MaxAccountsDataSizeExceeded"),
Self::ActiveVoteAccountClose => msg!("Error: ActiveVoteAccountClose"),
Self::InvalidRealloc => msg!("Error: InvalidRealloc"),
}
}
}
@ -126,7 +126,7 @@ pub const ACCOUNT_NOT_RENT_EXEMPT: u64 = to_builtin!(16);
pub const UNSUPPORTED_SYSVAR: u64 = to_builtin!(17);
pub const ILLEGAL_OWNER: u64 = to_builtin!(18);
pub const MAX_ACCOUNTS_DATA_SIZE_EXCEEDED: u64 = to_builtin!(19);
pub const ACTIVE_VOTE_ACCOUNT_CLOSE: u64 = to_builtin!(20);
pub const INVALID_ACCOUNT_DATA_REALLOC: u64 = to_builtin!(20);
// Warning: Any new program errors added here must also be:
// - Added to the below conversions
// - Added as an equivalent to InstructionError
@ -154,7 +154,7 @@ impl From<ProgramError> for u64 {
ProgramError::UnsupportedSysvar => UNSUPPORTED_SYSVAR,
ProgramError::IllegalOwner => ILLEGAL_OWNER,
ProgramError::MaxAccountsDataSizeExceeded => MAX_ACCOUNTS_DATA_SIZE_EXCEEDED,
ProgramError::ActiveVoteAccountClose => ACTIVE_VOTE_ACCOUNT_CLOSE,
ProgramError::InvalidRealloc => INVALID_ACCOUNT_DATA_REALLOC,
ProgramError::Custom(error) => {
if error == 0 {
CUSTOM_ZERO
@ -188,7 +188,7 @@ impl From<u64> for ProgramError {
UNSUPPORTED_SYSVAR => Self::UnsupportedSysvar,
ILLEGAL_OWNER => Self::IllegalOwner,
MAX_ACCOUNTS_DATA_SIZE_EXCEEDED => Self::MaxAccountsDataSizeExceeded,
ACTIVE_VOTE_ACCOUNT_CLOSE => Self::ActiveVoteAccountClose,
INVALID_ACCOUNT_DATA_REALLOC => Self::InvalidRealloc,
_ => Self::Custom(error as u32),
}
}
@ -218,7 +218,7 @@ impl TryFrom<InstructionError> for ProgramError {
Self::Error::UnsupportedSysvar => Ok(Self::UnsupportedSysvar),
Self::Error::IllegalOwner => Ok(Self::IllegalOwner),
Self::Error::MaxAccountsDataSizeExceeded => Ok(Self::MaxAccountsDataSizeExceeded),
Self::Error::ActiveVoteAccountClose => Ok(Self::ActiveVoteAccountClose),
Self::Error::InvalidRealloc => Ok(Self::InvalidRealloc),
_ => Err(error),
}
}
@ -250,7 +250,7 @@ where
UNSUPPORTED_SYSVAR => Self::UnsupportedSysvar,
ILLEGAL_OWNER => Self::IllegalOwner,
MAX_ACCOUNTS_DATA_SIZE_EXCEEDED => Self::MaxAccountsDataSizeExceeded,
ACTIVE_VOTE_ACCOUNT_CLOSE => Self::ActiveVoteAccountClose,
INVALID_ACCOUNT_DATA_REALLOC => Self::InvalidRealloc,
_ => {
// A valid custom error has no bits set in the upper 32
if error >> BUILTIN_BIT_SHIFT == 0 {

View File

@ -139,9 +139,18 @@ pub fn instruction_to_nonce_error(
}
}
/// maximum permitted size of data: 10 MB
/// Maximum permitted size of data: 10 MiB
pub const MAX_PERMITTED_DATA_LENGTH: u64 = 10 * 1024 * 1024;
// SBF program entrypoint assumes that the max account data length
// will fit inside a u32. If this constant no longer fits in a u32,
// the entrypoint deserialization code in the SDK must be updated.
#[cfg(test)]
static_assertions::const_assert!(MAX_PERMITTED_DATA_LENGTH <= u32::MAX as u64);
#[cfg(test)]
static_assertions::const_assert_eq!(MAX_PERMITTED_DATA_LENGTH, 10_485_760);
#[frozen_abi(digest = "5e22s2kFu9Do77hdcCyxyhuKHD8ThAB6Q6dNaLTCjL5M")]
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, AbiExample, AbiEnumVisitor)]
pub enum SystemInstruction {