From d015f7bbbd0e75f2685063b3a3c614816692bf3c Mon Sep 17 00:00:00 2001 From: Joe Caulfield Date: Thu, 31 Oct 2024 21:00:23 +0400 Subject: [PATCH 1/4] program: safe deserialize config keys --- program/src/processor.rs | 44 +++++++++++++++++++------- program/tests/functional.rs | 62 +++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 11 deletions(-) diff --git a/program/src/processor.rs b/program/src/processor.rs index 72c79b3..516f914 100644 --- a/program/src/processor.rs +++ b/program/src/processor.rs @@ -12,21 +12,43 @@ use { std::collections::BTreeSet, }; -// [Core BPF]: Locally-implemented -// `solana_sdk::program_utils::limited_deserialize`. -fn limited_deserialize(input: &[u8]) -> Result -where - T: serde::de::DeserializeOwned, -{ - solana_program::program_utils::limited_deserialize( - input, 1232, // [Core BPF]: See `solana_sdk::packet::PACKET_DATA_SIZE` - ) - .map_err(|_| ProgramError::InvalidInstructionData) +// [Core BPF]: The original Config builtin leverages the +// `solana_sdk::program_utils::limited_deserialize` method to cap the length of +// the input buffer at 1232 (`solana_sdk::packet::PACKET_DATA_SIZE`). As a +// result, any input buffer larger than 1232 will abort deserialization and +// return `InstructionError::InvalidInstructionData`. +// +// Howevever, since `ConfigKeys` contains a vector of `(Pubkey, bool)`, the +// `limited_deserialize` method will still read the vector's length and attempt +// to allocate a vector of the designated size. For extremely large length +// values, this can cause the initial allocation of a large vector to exhuast +// the BPF program's heap before deserialization can proceed. +// +// To mitigate this memory issue, the BPF version of the Config program has +// been designed to "peek" the length value, and ensure it cannot allocate a +// vector that would otherwise violate the input buffer length restriction of +// 1232. +// +// Taking the maximum input length of 1232 and subtracting (up to) 3 bytes for +// the `ShortU16`, then dividing that by the size of a `(Pubkey, bool)` entry +// (33), we get a maximum vector size of 37. A `ShortU16` value for 37 fits in +// just one byte (`[0x25]`), so this function can simply check the first +// provided byte. +fn safe_deserialize_config_keys(input: &[u8]) -> Result { + match input.first() { + Some(first_byte) if *first_byte <= 0x25 => { + solana_program::program_utils::limited_deserialize::( + input, 1232, // [Core BPF]: See `solana_sdk::packet::PACKET_DATA_SIZE` + ) + .map_err(|_| ProgramError::InvalidInstructionData) + } + _ => Err(ProgramError::InvalidInstructionData), + } } /// Config program processor. pub fn process(program_id: &Pubkey, accounts: &[AccountInfo], input: &[u8]) -> ProgramResult { - let key_list: ConfigKeys = limited_deserialize(input)?; + let key_list = safe_deserialize_config_keys(input)?; let mut accounts_iter = accounts.iter(); let config_account = next_account_info(&mut accounts_iter)?; diff --git a/program/tests/functional.rs b/program/tests/functional.rs index 2cd5d85..6ab2a03 100644 --- a/program/tests/functional.rs +++ b/program/tests/functional.rs @@ -587,3 +587,65 @@ fn test_maximum_keys_input() { &[Check::err(ProgramError::InvalidInstructionData)], ); } + +#[test] +fn test_safe_deserialize() { + let mollusk = setup(); + + // Accounts don't matter for this test. + + // First try to spoof the program with just `ShortU16` length values. + let build_instruction = + |data: &[u8]| Instruction::new_with_bytes(solana_config_program::id(), data, vec![]); + + mollusk.process_and_validate_instruction( + // Empty buffer. Not a valid `ShortU16`. + &build_instruction(&[]), + &[], + &[Check::err(ProgramError::InvalidInstructionData)], + ); + + mollusk.process_and_validate_instruction( + // `ShortU16` value of 38. One byte too large. + &build_instruction(&[0x26]), + &[], + &[Check::err(ProgramError::InvalidInstructionData)], + ); + + mollusk.process_and_validate_instruction( + // `ShortU16` value of 37. OK for vector size, but no keys following. + &build_instruction(&[0x25]), + &[], + &[Check::err(ProgramError::InvalidInstructionData)], + ); + + // Now try with some actual `ConfigKeys` inputs. + let mut keys = Vec::new(); + let serialized_config_keys = |keys: &[(Pubkey, bool)]| { + let config_keys = ConfigKeys { + keys: keys.to_vec(), + }; + bincode::serialize(&config_keys).unwrap() + }; + + // First build out to an acceptable size of 37. + (0..37).for_each(|i| keys.push((Pubkey::new_unique(), i % 2 == 0))); + + mollusk.process_and_validate_instruction( + // `ShortU16` value of 37. OK. + &build_instruction(&serialized_config_keys(&keys)), + &[], + // Falls through to account keys failure. + &[Check::err(ProgramError::NotEnoughAccountKeys)], + ); + + // Add one more key, pushing the size to 38. + keys.push((Pubkey::new_unique(), true)); + + mollusk.process_and_validate_instruction( + // `ShortU16` value of 38. Err. + &build_instruction(&serialized_config_keys(&keys)), + &[], + &[Check::err(ProgramError::InvalidInstructionData)], + ); +} From 7a309479dee9ca1849508d53110d616125f780ad Mon Sep 17 00:00:00 2001 From: Joe Caulfield Date: Thu, 31 Oct 2024 21:15:37 +0400 Subject: [PATCH 2/4] adjust CUs in tests --- program/tests/functional.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/program/tests/functional.rs b/program/tests/functional.rs index 6ab2a03..89c89a4 100644 --- a/program/tests/functional.rs +++ b/program/tests/functional.rs @@ -83,7 +83,7 @@ fn test_process_create_ok() { &[(config, config_account)], &[ Check::success(), - Check::compute_units(581), + Check::compute_units(584), Check::account(&config) .data( &bincode::serialize(&(ConfigKeys { keys: vec![] }, MyConfig::default())) @@ -111,7 +111,7 @@ fn test_process_store_ok() { &[(config, config_account)], &[ Check::success(), - Check::compute_units(581), + Check::compute_units(584), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, my_config)).unwrap()) .build(), @@ -186,7 +186,7 @@ fn test_process_store_with_additional_signers() { ], &[ Check::success(), - Check::compute_units(3_228), + Check::compute_units(3_253), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, my_config)).unwrap()) .build(), @@ -285,7 +285,7 @@ fn test_config_updates() { (signer0, AccountSharedData::default()), (signer1, AccountSharedData::default()), ], - &[Check::success(), Check::compute_units(3_228)], + &[Check::success(), Check::compute_units(3_253)], ); // Use this for next invoke. @@ -303,7 +303,7 @@ fn test_config_updates() { ], &[ Check::success(), - Check::compute_units(3_229), + Check::compute_units(3_254), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, new_config)).unwrap()) .build(), @@ -399,7 +399,7 @@ fn test_config_update_contains_duplicates_fails() { (signer0, AccountSharedData::default()), (signer1, AccountSharedData::default()), ], - &[Check::success(), Check::compute_units(3_228)], + &[Check::success(), Check::compute_units(3_253)], ); // Attempt update with duplicate signer inputs. @@ -443,7 +443,7 @@ fn test_config_updates_requiring_config() { ], &[ Check::success(), - Check::compute_units(3_324), + Check::compute_units(3_352), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys: keys.clone() }, my_config)).unwrap()) .build(), @@ -464,7 +464,7 @@ fn test_config_updates_requiring_config() { ], &[ Check::success(), - Check::compute_units(3_324), + Check::compute_units(3_352), Check::account(&config) .data(&bincode::serialize(&(ConfigKeys { keys }, new_config)).unwrap()) .build(), @@ -558,7 +558,7 @@ fn test_maximum_keys_input() { let result = mollusk.process_and_validate_instruction( &instruction, &[(config, config_account)], - &[Check::success(), Check::compute_units(25_020)], + &[Check::success(), Check::compute_units(25_247)], ); // Use this for next invoke. @@ -571,7 +571,7 @@ fn test_maximum_keys_input() { let result = mollusk.process_and_validate_instruction( &instruction, &[(config, updated_config_account)], - &[Check::success(), Check::compute_units(25_020)], + &[Check::success(), Check::compute_units(25_247)], ); // Use this for next invoke. From 5b000cfd1cef2d01a6e47d057e594d0b4d2dc0e4 Mon Sep 17 00:00:00 2001 From: Joe Caulfield Date: Thu, 31 Oct 2024 21:24:35 +0400 Subject: [PATCH 3/4] check in new compute unit benchmark --- program/benches/compute_units.md | 41 ++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/program/benches/compute_units.md b/program/benches/compute_units.md index 5ffd045..b95870b 100644 --- a/program/benches/compute_units.md +++ b/program/benches/compute_units.md @@ -1,3 +1,44 @@ +#### Compute Units: 2024-10-31 17:22:37.667457 UTC + +| Name | CUs | Delta | +|------|------|-------| +| config_small_init_0_keys | 584 | +3 | +| config_small_init_1_keys | 1215 | +11 | +| config_small_init_5_keys | 2834 | +35 | +| config_small_init_10_keys | 4904 | +65 | +| config_small_init_25_keys | 11746 | +155 | +| config_small_init_37_keys | 16749 | +227 | +| config_small_store_0_keys | 584 | +3 | +| config_small_store_1_keys | 1469 | +11 | +| config_small_store_5_keys | 4004 | +35 | +| config_small_store_10_keys | 7219 | +65 | +| config_small_store_25_keys | 17496 | +155 | +| config_small_store_37_keys | 25247 | +227 | +| config_medium_init_0_keys | 575 | +3 | +| config_medium_init_1_keys | 1162 | +11 | +| config_medium_init_5_keys | 2834 | +35 | +| config_medium_init_10_keys | 4904 | +65 | +| config_medium_init_25_keys | 11746 | +155 | +| config_medium_init_37_keys | 16749 | +227 | +| config_medium_store_0_keys | 575 | +3 | +| config_medium_store_1_keys | 1416 | +11 | +| config_medium_store_5_keys | 4004 | +35 | +| config_medium_store_10_keys | 7219 | +65 | +| config_medium_store_25_keys | 17496 | +155 | +| config_medium_store_37_keys | 25247 | +227 | +| config_large_init_0_keys | 696 | +3 | +| config_large_init_1_keys | 1283 | +11 | +| config_large_init_5_keys | 2955 | +35 | +| config_large_init_10_keys | 5026 | +65 | +| config_large_init_25_keys | 11870 | +155 | +| config_large_init_37_keys | 16874 | +227 | +| config_large_store_0_keys | 696 | +3 | +| config_large_store_1_keys | 1537 | +11 | +| config_large_store_5_keys | 4125 | +35 | +| config_large_store_10_keys | 7341 | +65 | +| config_large_store_25_keys | 17620 | +155 | +| config_large_store_37_keys | 25372 | +227 | + #### Compute Units: 2024-10-23 12:46:27.264402 UTC | Name | CUs | Delta | From 3ed2ba613513465e883b872616a3fa95f637c5cd Mon Sep 17 00:00:00 2001 From: Joe Caulfield Date: Fri, 1 Nov 2024 10:31:51 +0400 Subject: [PATCH 4/4] use constants --- program/src/processor.rs | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/program/src/processor.rs b/program/src/processor.rs index 516f914..c25c31a 100644 --- a/program/src/processor.rs +++ b/program/src/processor.rs @@ -12,11 +12,21 @@ use { std::collections::BTreeSet, }; +// Maximum input buffer length that can be deserialized. +// See `solana_sdk::packet::PACKET_DATA_SIZE`. +const MAX_INPUT_LEN: usize = 1232; +// Maximum vector length for a `ConfigKeys` struct's `keys` list. +// See comments below for `safe_deserialize_config_keys`. +// +// Take the maximum input length and subtract (up to) 3 bytes for the +// `ShortU16`, then divide that by the size of a `(Pubkey, bool)` entry. +const MAX_VECTOR_LEN: usize = (MAX_INPUT_LEN - 3) / (32 + 1); + // [Core BPF]: The original Config builtin leverages the // `solana_sdk::program_utils::limited_deserialize` method to cap the length of -// the input buffer at 1232 (`solana_sdk::packet::PACKET_DATA_SIZE`). As a -// result, any input buffer larger than 1232 will abort deserialization and -// return `InstructionError::InvalidInstructionData`. +// the input buffer at `MAX_INPUT_LEN` (1232). As a result, any input buffer +// larger than `MAX_INPUT_LEN` will abort deserialization and return +// `InstructionError::InvalidInstructionData`. // // Howevever, since `ConfigKeys` contains a vector of `(Pubkey, bool)`, the // `limited_deserialize` method will still read the vector's length and attempt @@ -26,19 +36,15 @@ use { // // To mitigate this memory issue, the BPF version of the Config program has // been designed to "peek" the length value, and ensure it cannot allocate a -// vector that would otherwise violate the input buffer length restriction of -// 1232. -// -// Taking the maximum input length of 1232 and subtracting (up to) 3 bytes for -// the `ShortU16`, then dividing that by the size of a `(Pubkey, bool)` entry -// (33), we get a maximum vector size of 37. A `ShortU16` value for 37 fits in -// just one byte (`[0x25]`), so this function can simply check the first -// provided byte. +// vector that would otherwise violate the input buffer length restriction. +// Since a `ShortU16` value for `MAX_VECTOR_LEN` fits in just one byte, we can +// simply peek the first byte before attempting deserialization. fn safe_deserialize_config_keys(input: &[u8]) -> Result { match input.first() { - Some(first_byte) if *first_byte <= 0x25 => { + Some(first_byte) if *first_byte as usize <= MAX_VECTOR_LEN => { solana_program::program_utils::limited_deserialize::( - input, 1232, // [Core BPF]: See `solana_sdk::packet::PACKET_DATA_SIZE` + input, + MAX_INPUT_LEN as u64, ) .map_err(|_| ProgramError::InvalidInstructionData) }