diff --git a/src/serde_bin.rs b/src/serde_bin.rs index 3abbe7b..6b93cbd 100644 --- a/src/serde_bin.rs +++ b/src/serde_bin.rs @@ -254,16 +254,21 @@ impl SerBin for String { impl DeBin for String { fn de_bin(o: &mut usize, d: &[u8]) -> Result { let len: usize = DeBin::de_bin(o, d)?; - if *o + len > d.len() { - return Err(DeBinErr { - o: *o, - msg: DeBinErrReason::Length { - expected_length: 1, - actual_length: d.len(), - }, - }); - } - let r = match core::str::from_utf8(&d[*o..(*o + len)]) { + // Use checked arithmetic: `*o + len` could overflow and bypass this check + // (e.g. *o = 1, len = usize::MAX), panicking on the slice below. + let end = match (*o).checked_add(len) { + Some(end) if end <= d.len() => end, + _ => { + return Err(DeBinErr { + o: *o, + msg: DeBinErrReason::Length { + expected_length: len, + actual_length: d.len(), + }, + }); + } + }; + let r = match core::str::from_utf8(&d[*o..end]) { Ok(r) => r.to_owned(), Err(_) => { return Err(DeBinErr { @@ -299,7 +304,11 @@ where { fn de_bin(o: &mut usize, d: &[u8]) -> Result, DeBinErr> { let len: usize = DeBin::de_bin(o, d)?; - let mut out = Vec::with_capacity(len); + // Do not reserve capacity based on the untrusted declared length: a small + // malformed buffer could otherwise trigger an allocation of len * size_of::() + // (up to ~2^64 elements) before any element is successfully read. + // Grow from actual data instead, mirroring LinkedList/BTreeSet/BTreeMap below. + let mut out = Vec::new(); for _ in 0..len { out.push(DeBin::de_bin(o, d)?) } @@ -355,7 +364,8 @@ where { fn de_bin(o: &mut usize, d: &[u8]) -> Result { let len: usize = DeBin::de_bin(o, d)?; - let mut out = std::collections::HashSet::with_capacity(len); + // Do not reserve capacity based on the untrusted declared length (see Vec impl). + let mut out = std::collections::HashSet::new(); for _ in 0..len { out.insert(DeBin::de_bin(o, d)?); } @@ -602,7 +612,8 @@ where { fn de_bin(o: &mut usize, d: &[u8]) -> Result { let len: usize = DeBin::de_bin(o, d)?; - let mut h = std::collections::HashMap::with_capacity(len); + // Do not reserve capacity based on the untrusted declared length (see Vec impl). + let mut h = std::collections::HashMap::new(); for _ in 0..len { let k = DeBin::de_bin(o, d)?; let v = DeBin::de_bin(o, d)?; diff --git a/tests/untrusted_alloc.rs b/tests/untrusted_alloc.rs new file mode 100644 index 0000000..71f9273 --- /dev/null +++ b/tests/untrusted_alloc.rs @@ -0,0 +1,86 @@ +//! Regression tests: untrusted length prefixes in DeBin must not drive +//! large allocations (CWE-770) or panic via integer overflow (CWE-680). +//! +//! The counting `#[global_allocator]` is process-global, so all checks are +//! merged into a single `#[test]` to keep them serial and unpolluted. +use std::alloc::{GlobalAlloc, Layout, System}; +use std::sync::atomic::{AtomicUsize, Ordering}; + +static PEAK: AtomicUsize = AtomicUsize::new(0); +static CURRENT: AtomicUsize = AtomicUsize::new(0); + +struct Counting; + +unsafe impl GlobalAlloc for Counting { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + let ptr = System.alloc(layout); + if !ptr.is_null() { + CURRENT.fetch_add(layout.size(), Ordering::SeqCst); + PEAK.fetch_max(CURRENT.load(Ordering::SeqCst), Ordering::SeqCst); + } + ptr + } + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + System.dealloc(ptr, layout); + CURRENT.fetch_sub(layout.size(), Ordering::SeqCst); + } + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + let new_ptr = System.realloc(ptr, layout, new_size); + if !new_ptr.is_null() { + CURRENT.fetch_sub(layout.size(), Ordering::SeqCst); + CURRENT.fetch_add(new_size, Ordering::SeqCst); + PEAK.fetch_max(CURRENT.load(Ordering::SeqCst), Ordering::SeqCst); + } + new_ptr + } +} + +#[global_allocator] +static A: Counting = Counting; + +use nanoserde::DeBin; + +/// An 8-byte declared length (64 MiB) with no element data must error without +/// preallocating 64 MiB in Vec / HashSet / HashMap, and an overflowing String +/// length must return Err instead of panicking. +#[test] +fn untrusted_length_prefix_is_bounded() { + let declared: u64 = 64 * 1024 * 1024; + let data = declared.to_le_bytes().to_vec(); // 8 bytes, truncated + + // Vec + PEAK.store(0, Ordering::SeqCst); + assert!(Vec::::de_bin(&mut 0, &data).is_err()); + let vec_peak = PEAK.load(Ordering::SeqCst); + assert!( + vec_peak < 8192, + "Vec must grow from actual data, not from the declared length (peak {vec_peak} bytes)" + ); + + // HashMap + PEAK.store(0, Ordering::SeqCst); + assert!(std::collections::HashMap::::de_bin(&mut 0, &data).is_err()); + let map_peak = PEAK.load(Ordering::SeqCst); + assert!( + map_peak < 8192, + "HashMap must grow from actual data, not from the declared length (peak {map_peak} bytes)" + ); + + // HashSet + PEAK.store(0, Ordering::SeqCst); + assert!(std::collections::HashSet::::de_bin(&mut 0, &data).is_err()); + let set_peak = PEAK.load(Ordering::SeqCst); + assert!( + set_peak < 8192, + "HashSet must grow from actual data, not from the declared length (peak {set_peak} bytes)" + ); + + // String: `*o + len` used to overflow (o=1, len=usize::MAX) and panic on the slice. + let mut data = vec![0xAAu8]; + data.extend((usize::MAX as u64).to_le_bytes()); + let r = std::panic::catch_unwind(|| String::de_bin(&mut 1, &data)); + assert!( + matches!(r, Ok(Err(_))), + "overflowing String length must return Err, not panic" + ); +}