From 8ef097bf6f730fcb0bd8c63a2884df32375c8588 Mon Sep 17 00:00:00 2001 From: anatoly yakovenko Date: Mon, 27 Apr 2020 11:06:00 -0700 Subject: [PATCH] Input values are not sanitized after they are deserialized, making it far too easy for Leo to earn SOL (#9706) * sanitize gossip protocol messages * sanitize transactions * crds protocol sanitize --- core/src/cluster_info.rs | 34 ++++++++++++++ core/src/contact_info.rs | 11 +++++ core/src/crds_gossip_pull.rs | 7 +++ core/src/crds_value.rs | 85 +++++++++++++++++++++++++++++----- core/src/epoch_slots.rs | 88 ++++++++++++++++++++++++++++++++++++ runtime/src/bank.rs | 3 +- runtime/src/bloom.rs | 2 + sdk/src/lib.rs | 1 + sdk/src/message.rs | 26 +++++++++++ sdk/src/pubkey.rs | 2 + sdk/src/sanitize.rs | 21 +++++++++ sdk/src/signature.rs | 2 + sdk/src/transaction.rs | 82 +++++++++++++++++++++++++-------- 13 files changed, 333 insertions(+), 31 deletions(-) create mode 100644 sdk/src/sanitize.rs diff --git a/core/src/cluster_info.rs b/core/src/cluster_info.rs index 16c86b7f8c..f8a92e1547 100644 --- a/core/src/cluster_info.rs +++ b/core/src/cluster_info.rs @@ -19,6 +19,7 @@ use crate::{ crds_gossip_pull::{CrdsFilter, CRDS_GOSSIP_PULL_CRDS_TIMEOUT_MS}, crds_value::{ self, CrdsData, CrdsValue, CrdsValueLabel, EpochSlotsIndex, LowestSlot, SnapshotHash, Vote, + MAX_WALLCLOCK, }, epoch_slots::EpochSlots, result::{Error, Result}, @@ -28,6 +29,7 @@ use crate::{ use rand::distributions::{Distribution, WeightedIndex}; use rand::SeedableRng; use rand_chacha::ChaChaRng; +use solana_sdk::sanitize::{Sanitize, SanitizeError}; use bincode::{serialize, serialized_size}; use core::cmp; @@ -156,6 +158,15 @@ pub struct PruneData { pub wallclock: u64, } +impl Sanitize for PruneData { + fn sanitize(&self) -> std::result::Result<(), SanitizeError> { + if self.wallclock >= MAX_WALLCLOCK { + return Err(SanitizeError::ValueOutOfRange); + } + Ok(()) + } +} + impl Signable for PruneData { fn pubkey(&self) -> Pubkey { self.pubkey @@ -212,6 +223,20 @@ enum Protocol { PruneMessage(Pubkey, PruneData), } +impl Sanitize for Protocol { + fn sanitize(&self) -> std::result::Result<(), SanitizeError> { + match self { + Protocol::PullRequest(filter, val) => { + filter.sanitize()?; + val.sanitize() + } + Protocol::PullResponse(_, val) => val.sanitize(), + Protocol::PushMessage(_, val) => val.sanitize(), + Protocol::PruneMessage(_, val) => val.sanitize(), + } + } +} + // Rating for pull requests // A response table is generated as a // 2-d table arranged by target nodes and a @@ -1364,6 +1389,7 @@ impl ClusterInfo { let from_addr = packet.meta.addr(); limited_deserialize(&packet.data[..packet.meta.size]) .into_iter() + .filter(|r: &Protocol| r.sanitize().is_ok()) .for_each(|request| match request { Protocol::PullRequest(filter, caller) => { let start = allocated.get(); @@ -2791,6 +2817,14 @@ mod tests { assert_eq!(MAX_PROTOCOL_HEADER_SIZE, max_protocol_size); } + #[test] + fn test_protocol_sanitize() { + let mut pd = PruneData::default(); + pd.wallclock = MAX_WALLCLOCK; + let msg = Protocol::PruneMessage(Pubkey::default(), pd); + assert_eq!(msg.sanitize(), Err(SanitizeError::ValueOutOfRange)); + } + // computes the maximum size for pull request blooms fn max_bloom_size() -> usize { let filter_size = serialized_size(&CrdsFilter::default()) diff --git a/core/src/contact_info.rs b/core/src/contact_info.rs index 44fa9dd793..d77ba52d80 100644 --- a/core/src/contact_info.rs +++ b/core/src/contact_info.rs @@ -1,6 +1,8 @@ +use crate::crds_value::MAX_WALLCLOCK; use solana_sdk::pubkey::Pubkey; #[cfg(test)] use solana_sdk::rpc_port; +use solana_sdk::sanitize::{Sanitize, SanitizeError}; #[cfg(test)] use solana_sdk::signature::{Keypair, Signer}; use solana_sdk::timing::timestamp; @@ -37,6 +39,15 @@ pub struct ContactInfo { pub shred_version: u16, } +impl Sanitize for ContactInfo { + fn sanitize(&self) -> std::result::Result<(), SanitizeError> { + if self.wallclock >= MAX_WALLCLOCK { + return Err(SanitizeError::Failed); + } + Ok(()) + } +} + impl Ord for ContactInfo { fn cmp(&self, other: &Self) -> Ordering { self.id.cmp(&other.id) diff --git a/core/src/crds_gossip_pull.rs b/core/src/crds_gossip_pull.rs index 5da4c4f5db..2d985606d0 100644 --- a/core/src/crds_gossip_pull.rs +++ b/core/src/crds_gossip_pull.rs @@ -37,6 +37,13 @@ pub struct CrdsFilter { mask_bits: u32, } +impl solana_sdk::sanitize::Sanitize for CrdsFilter { + fn sanitize(&self) -> std::result::Result<(), solana_sdk::sanitize::SanitizeError> { + self.filter.sanitize()?; + Ok(()) + } +} + impl CrdsFilter { pub fn new_rand(num_items: usize, max_bytes: usize) -> Self { let max_bits = (max_bytes * 8) as f64; diff --git a/core/src/crds_value.rs b/core/src/crds_value.rs index 42640f58d7..82ad19010c 100644 --- a/core/src/crds_value.rs +++ b/core/src/crds_value.rs @@ -2,6 +2,7 @@ use crate::contact_info::ContactInfo; use crate::deprecated; use crate::epoch_slots::EpochSlots; use bincode::{serialize, serialized_size}; +use solana_sdk::sanitize::{Sanitize, SanitizeError}; use solana_sdk::timing::timestamp; use solana_sdk::{ clock::Slot, @@ -16,6 +17,9 @@ use std::{ fmt, }; +pub const MAX_WALLCLOCK: u64 = 1_000_000_000_000_000; +pub const MAX_SLOT: u64 = 1_000_000_000_000_000; + pub type VoteIndex = u8; pub const MAX_VOTES: VoteIndex = 32; @@ -29,6 +33,13 @@ pub struct CrdsValue { pub data: CrdsData, } +impl Sanitize for CrdsValue { + fn sanitize(&self) -> Result<(), SanitizeError> { + self.signature.sanitize()?; + self.data.sanitize() + } +} + impl Signable for CrdsValue { fn pubkey(&self) -> Pubkey { self.pubkey() @@ -47,15 +58,8 @@ impl Signable for CrdsValue { } fn verify(&self) -> bool { - let sig_check = self - .get_signature() - .verify(&self.pubkey().as_ref(), self.signable_data().borrow()); - let data_check = match &self.data { - CrdsData::Vote(ix, _) => *ix < MAX_VOTES, - CrdsData::EpochSlots(ix, _) => *ix < MAX_EPOCH_SLOTS, - _ => true, - }; - sig_check && data_check + self.get_signature() + .verify(&self.pubkey().as_ref(), self.signable_data().borrow()) } } @@ -73,6 +77,29 @@ pub enum CrdsData { EpochSlots(EpochSlotsIndex, EpochSlots), } +impl Sanitize for CrdsData { + fn sanitize(&self) -> Result<(), SanitizeError> { + match self { + CrdsData::ContactInfo(val) => val.sanitize(), + CrdsData::Vote(ix, val) => { + if *ix >= MAX_VOTES { + return Err(SanitizeError::Failed); + } + val.sanitize() + } + CrdsData::LowestSlot(_, val) => val.sanitize(), + CrdsData::SnapshotHashes(val) => val.sanitize(), + CrdsData::AccountsHashes(val) => val.sanitize(), + CrdsData::EpochSlots(ix, val) => { + if *ix as usize >= MAX_EPOCH_SLOTS as usize { + return Err(SanitizeError::Failed); + } + val.sanitize() + } + } + } +} + #[derive(Serialize, Deserialize, Clone, Debug, PartialEq)] pub struct SnapshotHash { pub from: Pubkey, @@ -80,6 +107,20 @@ pub struct SnapshotHash { pub wallclock: u64, } +impl Sanitize for SnapshotHash { + fn sanitize(&self) -> Result<(), SanitizeError> { + if self.wallclock >= MAX_WALLCLOCK { + return Err(SanitizeError::Failed); + } + for (slot, _) in &self.hashes { + if *slot >= MAX_SLOT { + return Err(SanitizeError::Failed); + } + } + self.from.sanitize() + } +} + impl SnapshotHash { pub fn new(from: Pubkey, hashes: Vec<(Slot, Hash)>) -> Self { Self { @@ -112,6 +153,18 @@ impl LowestSlot { } } +impl Sanitize for LowestSlot { + fn sanitize(&self) -> Result<(), SanitizeError> { + if self.wallclock >= MAX_WALLCLOCK { + return Err(SanitizeError::Failed); + } + if self.lowest >= MAX_SLOT { + return Err(SanitizeError::Failed); + } + self.from.sanitize() + } +} + #[derive(Serialize, Deserialize, Clone, Debug, PartialEq)] pub struct Vote { pub from: Pubkey, @@ -119,6 +172,16 @@ pub struct Vote { pub wallclock: u64, } +impl Sanitize for Vote { + fn sanitize(&self) -> Result<(), SanitizeError> { + if self.wallclock >= MAX_WALLCLOCK { + return Err(SanitizeError::Failed); + } + self.from.sanitize()?; + self.transaction.sanitize() + } +} + impl Vote { pub fn new(from: &Pubkey, transaction: Transaction, wallclock: u64) -> Self { Self { @@ -389,7 +452,7 @@ mod test { ), &keypair, ); - assert!(!vote.verify()); + assert!(vote.sanitize().is_err()); } #[test] @@ -402,7 +465,7 @@ mod test { ), &keypair, ); - assert!(!item.verify()); + assert!(item.sanitize().is_err()); } #[test] fn test_compute_vote_index_empty() { diff --git a/core/src/epoch_slots.rs b/core/src/epoch_slots.rs index c0e89f1543..159dee46ac 100644 --- a/core/src/epoch_slots.rs +++ b/core/src/epoch_slots.rs @@ -1,10 +1,14 @@ use crate::cluster_info::MAX_CRDS_OBJECT_SIZE; +use crate::crds_value::MAX_SLOT; +use crate::crds_value::MAX_WALLCLOCK; use bincode::serialized_size; use bv::BitVec; use flate2::{Compress, Compression, Decompress, FlushCompress, FlushDecompress}; use solana_sdk::clock::Slot; use solana_sdk::pubkey::Pubkey; +use solana_sdk::sanitize::{Sanitize, SanitizeError}; +const MAX_SLOTS_PER_ENTRY: usize = 2048 * 8; #[derive(Serialize, Deserialize, Clone, Debug, PartialEq)] pub struct Uncompressed { pub first_slot: Slot, @@ -12,6 +16,18 @@ pub struct Uncompressed { pub slots: BitVec, } +impl Sanitize for Uncompressed { + fn sanitize(&self) -> std::result::Result<(), SanitizeError> { + if self.first_slot >= MAX_SLOT { + return Err(SanitizeError::ValueOutOfRange); + } + if self.num >= MAX_SLOTS_PER_ENTRY { + return Err(SanitizeError::ValueOutOfRange); + } + Ok(()) + } +} + #[derive(Deserialize, Serialize, Clone, Debug, PartialEq)] pub struct Flate2 { pub first_slot: Slot, @@ -19,6 +35,18 @@ pub struct Flate2 { pub compressed: Vec, } +impl Sanitize for Flate2 { + fn sanitize(&self) -> std::result::Result<(), SanitizeError> { + if self.first_slot >= MAX_SLOT { + return Err(SanitizeError::ValueOutOfRange); + } + if self.num >= MAX_SLOTS_PER_ENTRY { + return Err(SanitizeError::ValueOutOfRange); + } + Ok(()) + } +} + #[derive(Debug, PartialEq)] pub enum Error { CompressError, @@ -98,6 +126,9 @@ impl Uncompressed { if self.num == 0 { self.first_slot = *s; } + if self.num >= MAX_SLOTS_PER_ENTRY { + return i; + } if *s < self.first_slot { return i; } @@ -117,6 +148,15 @@ pub enum CompressedSlots { Uncompressed(Uncompressed), } +impl Sanitize for CompressedSlots { + fn sanitize(&self) -> std::result::Result<(), SanitizeError> { + match self { + CompressedSlots::Uncompressed(a) => a.sanitize(), + CompressedSlots::Flate2(b) => b.sanitize(), + } + } +} + impl Default for CompressedSlots { fn default() -> Self { CompressedSlots::new(0) @@ -178,6 +218,16 @@ pub struct EpochSlots { pub wallclock: u64, } +impl Sanitize for EpochSlots { + fn sanitize(&self) -> std::result::Result<(), SanitizeError> { + if self.wallclock >= MAX_WALLCLOCK { + return Err(SanitizeError::ValueOutOfRange); + } + self.from.sanitize()?; + self.slots.sanitize() + } +} + use std::fmt; impl fmt::Debug for EpochSlots { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -327,6 +377,44 @@ mod tests { assert_eq!(slots.num, 701); assert_eq!(slots.to_slots(1), vec![1, 2, 701]); } + + #[test] + fn test_epoch_slots_sanitize() { + let mut slots = Uncompressed::new(100); + slots.add(&[1, 701, 2]); + assert_eq!(slots.num, 701); + assert!(slots.sanitize().is_ok()); + + let mut o = slots.clone(); + o.first_slot = MAX_SLOT; + assert_eq!(o.sanitize(), Err(SanitizeError::ValueOutOfRange)); + + let mut o = slots.clone(); + o.num = MAX_SLOTS_PER_ENTRY; + assert_eq!(o.sanitize(), Err(SanitizeError::ValueOutOfRange)); + + let compressed = Flate2::deflate(slots).unwrap(); + assert!(compressed.sanitize().is_ok()); + + let mut o = compressed.clone(); + o.first_slot = MAX_SLOT; + assert_eq!(o.sanitize(), Err(SanitizeError::ValueOutOfRange)); + + let mut o = compressed.clone(); + o.num = MAX_SLOTS_PER_ENTRY; + assert_eq!(o.sanitize(), Err(SanitizeError::ValueOutOfRange)); + + let mut slots = EpochSlots::default(); + let range: Vec = (0..5000).into_iter().collect(); + assert_eq!(slots.fill(&range, 1), 5000); + assert_eq!(slots.wallclock, 1); + assert!(slots.sanitize().is_ok()); + + let mut o = slots.clone(); + o.wallclock = MAX_WALLCLOCK; + assert_eq!(o.sanitize(), Err(SanitizeError::ValueOutOfRange)); + } + #[test] fn test_epoch_slots_fill_range() { let range: Vec = (0..5000).into_iter().collect(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 0bcdaecaf6..c508fc875e 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -45,6 +45,7 @@ use solana_sdk::{ inflation::Inflation, native_loader, nonce, pubkey::Pubkey, + sanitize::Sanitize, signature::{Keypair, Signature}, slot_hashes::SlotHashes, slot_history::SlotHistory, @@ -1077,7 +1078,7 @@ impl Bank { OrderedIterator::new(txs, iteration_order) .zip(lock_results) .map(|(tx, lock_res)| { - if lock_res.is_ok() && !tx.verify_refs() { + if lock_res.is_ok() && tx.sanitize().is_err() { error_counters.invalid_account_index += 1; Err(TransactionError::InvalidAccountIndex) } else { diff --git a/runtime/src/bloom.rs b/runtime/src/bloom.rs index f9a212bdf0..3739fe099d 100644 --- a/runtime/src/bloom.rs +++ b/runtime/src/bloom.rs @@ -19,6 +19,8 @@ pub struct Bloom { _phantom: PhantomData, } +impl solana_sdk::sanitize::Sanitize for Bloom {} + impl Bloom { pub fn new(num_bits: usize, keys: Vec) -> Self { let bits = BitVec::new_fill(false, num_bits as u64); diff --git a/sdk/src/lib.rs b/sdk/src/lib.rs index e78081b2e0..12bbdd1dcc 100644 --- a/sdk/src/lib.rs +++ b/sdk/src/lib.rs @@ -24,6 +24,7 @@ pub mod program_utils; pub mod pubkey; pub mod rent; pub mod rpc_port; +pub mod sanitize; pub mod short_vec; pub mod slot_hashes; pub mod slot_history; diff --git a/sdk/src/message.rs b/sdk/src/message.rs index af9b79f630..11d45aa072 100644 --- a/sdk/src/message.rs +++ b/sdk/src/message.rs @@ -1,5 +1,6 @@ //! A library for generating a message from a sequence of instructions +use crate::sanitize::{Sanitize, SanitizeError}; use crate::{ hash::Hash, instruction::{AccountMeta, CompiledInstruction, Instruction}, @@ -162,6 +163,31 @@ pub struct Message { pub instructions: Vec, } +impl Sanitize for Message { + fn sanitize(&self) -> std::result::Result<(), SanitizeError> { + if self.header.num_required_signatures as usize > self.account_keys.len() { + return Err(SanitizeError::IndexOutOfBounds); + } + if self.header.num_readonly_unsigned_accounts as usize + + self.header.num_readonly_signed_accounts as usize + > self.account_keys.len() + { + return Err(SanitizeError::IndexOutOfBounds); + } + for ci in &self.instructions { + if ci.program_id_index as usize >= self.account_keys.len() { + return Err(SanitizeError::IndexOutOfBounds); + } + for ai in &ci.accounts { + if *ai as usize >= self.account_keys.len() { + return Err(SanitizeError::IndexOutOfBounds); + } + } + } + Ok(()) + } +} + impl Message { pub fn new_with_compiled_instructions( num_required_signatures: u8, diff --git a/sdk/src/pubkey.rs b/sdk/src/pubkey.rs index 9885499732..de89a6bb81 100644 --- a/sdk/src/pubkey.rs +++ b/sdk/src/pubkey.rs @@ -28,6 +28,8 @@ impl DecodeError for PubkeyError { #[derive(Serialize, Deserialize, Clone, Copy, Default, Eq, PartialEq, Ord, PartialOrd, Hash)] pub struct Pubkey([u8; 32]); +impl crate::sanitize::Sanitize for Pubkey {} + #[derive(Error, Debug, Serialize, Clone, PartialEq, FromPrimitive, ToPrimitive)] pub enum ParsePubkeyError { #[error("String is the wrong size")] diff --git a/sdk/src/sanitize.rs b/sdk/src/sanitize.rs new file mode 100644 index 0000000000..89681d05b5 --- /dev/null +++ b/sdk/src/sanitize.rs @@ -0,0 +1,21 @@ +#[derive(PartialEq, Debug)] +pub enum SanitizeError { + Failed, + IndexOutOfBounds, + ValueOutOfRange, +} + +pub trait Sanitize { + fn sanitize(&self) -> Result<(), SanitizeError> { + Ok(()) + } +} + +impl Sanitize for Vec { + fn sanitize(&self) -> Result<(), SanitizeError> { + for x in self.iter() { + x.sanitize()?; + } + Ok(()) + } +} diff --git a/sdk/src/signature.rs b/sdk/src/signature.rs index 6b13f10b4e..7967c30d4e 100644 --- a/sdk/src/signature.rs +++ b/sdk/src/signature.rs @@ -49,6 +49,8 @@ impl Keypair { #[derive(Serialize, Deserialize, Clone, Copy, Default, Eq, PartialEq, Ord, PartialOrd, Hash)] pub struct Signature(GenericArray); +impl crate::sanitize::Sanitize for Signature {} + impl Signature { pub fn new(signature_slice: &[u8]) -> Self { Self(GenericArray::clone_from_slice(&signature_slice)) diff --git a/sdk/src/transaction.rs b/sdk/src/transaction.rs index 7f344b08db..00ffb9d6c4 100644 --- a/sdk/src/transaction.rs +++ b/sdk/src/transaction.rs @@ -1,5 +1,6 @@ //! Defines a Transaction type to package an atomic sequence of instructions. +use crate::sanitize::{Sanitize, SanitizeError}; use crate::{ hash::Hash, instruction::{CompiledInstruction, Instruction, InstructionError}, @@ -86,6 +87,18 @@ pub struct Transaction { pub message: Message, } +impl Sanitize for Transaction { + fn sanitize(&self) -> std::result::Result<(), SanitizeError> { + if self.message.header.num_required_signatures as usize > self.signatures.len() { + return Err(SanitizeError::IndexOutOfBounds); + } + if self.signatures.len() > self.message.account_keys.len() { + return Err(SanitizeError::IndexOutOfBounds); + } + self.message.sanitize() + } +} + impl Transaction { pub fn new_unsigned(message: Message) -> Self { Self { @@ -364,22 +377,6 @@ impl Transaction { .iter() .all(|signature| *signature != Signature::default()) } - - /// Verify that references in the instructions are valid - pub fn verify_refs(&self) -> bool { - let message = self.message(); - for instruction in &message.instructions { - if (instruction.program_id_index as usize) >= message.account_keys.len() { - return false; - } - for account_index in &instruction.accounts { - if (*account_index as usize) >= message.account_keys.len() { - return false; - } - } - } - true - } } #[cfg(test)] @@ -418,7 +415,7 @@ mod tests { vec![prog1, prog2], instructions, ); - assert!(tx.verify_refs()); + assert!(tx.sanitize().is_ok()); assert_eq!(tx.key(0, 0), Some(&key.pubkey())); assert_eq!(tx.signer_key(0, 0), Some(&key.pubkey())); @@ -452,7 +449,7 @@ mod tests { vec![], instructions, ); - assert!(!tx.verify_refs()); + assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds)); } #[test] fn test_refs_invalid_account() { @@ -466,7 +463,54 @@ mod tests { instructions, ); assert_eq!(*get_program_id(&tx, 0), Pubkey::default()); - assert!(!tx.verify_refs()); + assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds)); + } + + #[test] + fn test_sanitize_txs() { + let key = Keypair::new(); + let id0 = Pubkey::default(); + let program_id = Pubkey::new_rand(); + let ix = Instruction::new( + program_id, + &0, + vec![ + AccountMeta::new(key.pubkey(), true), + AccountMeta::new(id0, true), + ], + ); + let ixs = vec![ix]; + let mut tx = Transaction::new_with_payer(ixs, Some(&key.pubkey())); + let o = tx.clone(); + assert_eq!(tx.sanitize(), Ok(())); + assert_eq!(tx.message.account_keys.len(), 3); + + tx = o.clone(); + tx.message.header.num_required_signatures = 3; + assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds)); + + tx = o.clone(); + tx.message.header.num_readonly_signed_accounts = 4; + tx.message.header.num_readonly_unsigned_accounts = 0; + assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds)); + + tx = o.clone(); + tx.message.header.num_readonly_signed_accounts = 2; + tx.message.header.num_readonly_unsigned_accounts = 2; + assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds)); + + tx = o.clone(); + tx.message.header.num_readonly_signed_accounts = 0; + tx.message.header.num_readonly_unsigned_accounts = 4; + assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds)); + + tx = o.clone(); + tx.message.instructions[0].program_id_index = 3; + assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds)); + + tx = o.clone(); + tx.message.instructions[0].accounts[0] = 3; + assert_eq!(tx.sanitize(), Err(SanitizeError::IndexOutOfBounds)); } fn create_sample_transaction() -> Transaction {