From 470b0029057b4ce34ea11f9ace8f18a6ebf53da7 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Sun, 27 Sep 2026 17:50:18 +0800 Subject: [PATCH] query: check the additions a hostile msgpack length drives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module documents "Returns `None` on truncated/invalid data — never panics", but four sites added a length read from the input to a header width in plain `usize`: the STR32, BIN32 and EXT32 arms of `skip_value`, and `start + len` in the string read path. `5 + 0xffff_ffff` fits in a 64-bit `usize` and the buffer check rejects it; on a 32-bit target the addition overflows first and the process aborts. `checked_advance_len` performs both additions checked, and `checked_advance`, `read_u16_be`, `read_u32_be`, `read_u64_be`, `read_str`, `read_str_advance`, `read_bin_advance`, the field index and the field lookup all route through it. The existing adversarial-length test already covers all four sites; it is red->green on a 32-bit target and green on both before and after on a 64-bit one, which is why this went unnoticed: wasm32-wasip1, fix reverted: FAILED, "attempt to add with overflow" wasm32-wasip1, fix applied: PASS x86_64, either: PASS --- nodedb-query/src/msgpack_scan/field.rs | 16 +++++++--- nodedb-query/src/msgpack_scan/index.rs | 10 ++++-- nodedb-query/src/msgpack_scan/reader/mod.rs | 1 + .../src/msgpack_scan/reader/scalar.rs | 11 ++++--- nodedb-query/src/msgpack_scan/reader/skip.rs | 20 ++++++------ nodedb-query/src/msgpack_scan/reader/tags.rs | 31 ++++++++++++++++--- 6 files changed, 63 insertions(+), 26 deletions(-) diff --git a/nodedb-query/src/msgpack_scan/field.rs b/nodedb-query/src/msgpack_scan/field.rs index 02ed1f908..7186e9fd8 100644 --- a/nodedb-query/src/msgpack_scan/field.rs +++ b/nodedb-query/src/msgpack_scan/field.rs @@ -5,7 +5,7 @@ //! Given a `&[u8]` containing a MessagePack map, extract the byte range //! of a value for a given key — without allocating or decoding. -use crate::msgpack_scan::reader::{map_header, skip_value, str_bounds}; +use crate::msgpack_scan::reader::{checked_advance_len, map_header, skip_value, str_bounds}; /// A byte range `(start, end)` within a MessagePack buffer, pointing to /// a complete value (tag + payload). Use `read_f64`, `read_i64`, `read_str` @@ -28,10 +28,16 @@ pub fn extract_field(buf: &[u8], offset: usize, field: &str) -> Option buf - .get(start..start + len) - .map(|kb| kb == field_bytes) - .unwrap_or(false), + Some((start, len)) => { + // Hostile-length addition: see `checked_advance_len`. + match checked_advance_len(buf, start, 0, len) { + Some(end) => buf + .get(start..end) + .map(|kb| kb == field_bytes) + .unwrap_or(false), + None => false, + } + } None => false, }; diff --git a/nodedb-query/src/msgpack_scan/index.rs b/nodedb-query/src/msgpack_scan/index.rs index 2d6a30619..15a35fc34 100644 --- a/nodedb-query/src/msgpack_scan/index.rs +++ b/nodedb-query/src/msgpack_scan/index.rs @@ -9,7 +9,7 @@ //! Uses a flat array with linear search for small docs (≤ 16 fields) to //! avoid HashMap allocation overhead. Falls back to HashMap for large docs. -use crate::msgpack_scan::reader::{map_header, skip_value, str_bounds}; +use crate::msgpack_scan::reader::{checked_advance_len, map_header, skip_value, str_bounds}; /// Threshold: docs with more fields than this use HashMap, otherwise flat array. const HASH_THRESHOLD: usize = 16; @@ -38,7 +38,9 @@ impl FieldIndex { let mut entries = Vec::with_capacity(count); for _ in 0..count { let key_str = if let Some((start, len)) = str_bounds(buf, pos) { - std::str::from_utf8(buf.get(start..start + len)?).ok() + // Hostile-length addition: see `checked_advance_len`. + let end = checked_advance_len(buf, start, 0, len)?; + std::str::from_utf8(buf.get(start..end)?).ok() } else { None }; @@ -61,7 +63,9 @@ impl FieldIndex { let mut offsets = std::collections::HashMap::with_capacity(cap); for _ in 0..count { let key_str = if let Some((start, len)) = str_bounds(buf, pos) { - std::str::from_utf8(buf.get(start..start + len)?).ok() + // Hostile-length addition: see `checked_advance_len`. + let end = checked_advance_len(buf, start, 0, len)?; + std::str::from_utf8(buf.get(start..end)?).ok() } else { None }; diff --git a/nodedb-query/src/msgpack_scan/reader/mod.rs b/nodedb-query/src/msgpack_scan/reader/mod.rs index 1a19f8c94..dcf2c9b17 100644 --- a/nodedb-query/src/msgpack_scan/reader/mod.rs +++ b/nodedb-query/src/msgpack_scan/reader/mod.rs @@ -16,4 +16,5 @@ pub use scalar::{ read_str_advance, read_u32_advance, }; pub use skip::skip_value; +pub(crate) use tags::checked_advance_len; pub use value::read_value; diff --git a/nodedb-query/src/msgpack_scan/reader/scalar.rs b/nodedb-query/src/msgpack_scan/reader/scalar.rs index 1067b7e04..e4bf0598c 100644 --- a/nodedb-query/src/msgpack_scan/reader/scalar.rs +++ b/nodedb-query/src/msgpack_scan/reader/scalar.rs @@ -65,7 +65,9 @@ pub fn read_i64(buf: &[u8], offset: usize) -> Option { /// or invalid UTF-8. pub fn read_str(buf: &[u8], offset: usize) -> Option<&str> { let (start, len) = str_bounds(buf, offset)?; - let bytes = buf.get(start..start + len)?; + // `start + len` is a hostile-length addition; see `checked_advance_len`. + let end = checked_advance_len(buf, start, 0, len)?; + let bytes = buf.get(start..end)?; str::from_utf8(bytes).ok() } @@ -73,9 +75,10 @@ pub fn read_str(buf: &[u8], offset: usize) -> Option<&str> { /// Returns `None` for non-string types, invalid UTF-8, or truncated input. pub fn read_str_advance<'a>(buf: &'a [u8], off: &mut usize) -> Option<&'a str> { let (start, len) = str_bounds(buf, *off)?; - let bytes = buf.get(start..start + len)?; + let end = checked_advance_len(buf, start, 0, len)?; + let bytes = buf.get(start..end)?; let s = str::from_utf8(bytes).ok()?; - *off = start + len; + *off = end; Some(s) } @@ -91,7 +94,7 @@ pub fn read_bin_advance<'a>(buf: &'a [u8], off: &mut usize) -> Option<&'a [u8]> _ => return None, }; let start = *off + header; - let end = start + len; + let end = checked_advance_len(buf, start, 0, len)?; let data = buf.get(start..end)?; *off = end; Some(data) diff --git a/nodedb-query/src/msgpack_scan/reader/skip.rs b/nodedb-query/src/msgpack_scan/reader/skip.rs index 9f63ee35b..79d1cc35f 100644 --- a/nodedb-query/src/msgpack_scan/reader/skip.rs +++ b/nodedb-query/src/msgpack_scan/reader/skip.rs @@ -57,33 +57,33 @@ fn skip_value_depth(buf: &[u8], offset: usize, depth: u16) -> Option { // fixstr (0xa0..=0xbf) 0xa0..=0xbf => { let len = (tag & 0x1f) as usize; - checked_advance(buf, offset, 1 + len) + checked_advance_len(buf, offset, 1, len) } STR8 => { let len = get(buf, offset + 1)? as usize; - checked_advance(buf, offset, 2 + len) + checked_advance_len(buf, offset, 2, len) } STR16 => { let len = read_u16_be(buf, offset + 1)? as usize; - checked_advance(buf, offset, 3 + len) + checked_advance_len(buf, offset, 3, len) } STR32 => { let len = read_u32_be(buf, offset + 1)? as usize; - checked_advance(buf, offset, 5 + len) + checked_advance_len(buf, offset, 5, len) } // bin BIN8 => { let len = get(buf, offset + 1)? as usize; - checked_advance(buf, offset, 2 + len) + checked_advance_len(buf, offset, 2, len) } BIN16 => { let len = read_u16_be(buf, offset + 1)? as usize; - checked_advance(buf, offset, 3 + len) + checked_advance_len(buf, offset, 3, len) } BIN32 => { let len = read_u32_be(buf, offset + 1)? as usize; - checked_advance(buf, offset, 5 + len) + checked_advance_len(buf, offset, 5, len) } // fixed-width numerics (bounds-check against buffer length) @@ -102,15 +102,15 @@ fn skip_value_depth(buf: &[u8], offset: usize, depth: u16) -> Option { FIXEXT16 => checked_advance(buf, offset, 18), EXT8 => { let len = get(buf, offset + 1)? as usize; - checked_advance(buf, offset, 3 + len) + checked_advance_len(buf, offset, 3, len) } EXT16 => { let len = read_u16_be(buf, offset + 1)? as usize; - checked_advance(buf, offset, 4 + len) + checked_advance_len(buf, offset, 4, len) } EXT32 => { let len = read_u32_be(buf, offset + 1)? as usize; - checked_advance(buf, offset, 6 + len) + checked_advance_len(buf, offset, 6, len) } // 0xc1 is never used in the spec diff --git a/nodedb-query/src/msgpack_scan/reader/tags.rs b/nodedb-query/src/msgpack_scan/reader/tags.rs index 4b1daf45c..5277ee5c0 100644 --- a/nodedb-query/src/msgpack_scan/reader/tags.rs +++ b/nodedb-query/src/msgpack_scan/reader/tags.rs @@ -44,27 +44,50 @@ pub(super) fn get(buf: &[u8], pos: usize) -> Option { #[inline(always)] pub(super) fn read_u16_be(buf: &[u8], pos: usize) -> Option { - let bytes = buf.get(pos..pos + 2)?; + let bytes = buf.get(pos..pos.checked_add(2)?)?; Some(u16::from_be_bytes([bytes[0], bytes[1]])) } #[inline(always)] pub(super) fn read_u32_be(buf: &[u8], pos: usize) -> Option { - let bytes = buf.get(pos..pos + 4)?; + let bytes = buf.get(pos..pos.checked_add(4)?)?; Some(u32::from_be_bytes([bytes[0], bytes[1], bytes[2], bytes[3]])) } #[inline(always)] pub(super) fn read_u64_be(buf: &[u8], pos: usize) -> Option { - let bytes = buf.get(pos..pos + 8)?; + let bytes = buf.get(pos..pos.checked_add(8)?)?; Some(u64::from_be_bytes([ bytes[0], bytes[1], bytes[2], bytes[3], bytes[4], bytes[5], bytes[6], bytes[7], ])) } /// Return `Some(offset + size)` only if the buffer has enough bytes. +/// +/// The addition is checked. `size` reaches this function as a tag's header +/// width plus a payload length read from the input, so on a 32-bit target it +/// can exceed `usize` before the buffer is ever consulted: `5 + 0xffff_ffff` +/// panics rather than returning `None`. Checking here keeps a hostile length +/// field a decode failure on every target instead of an abort on 32-bit. #[inline(always)] pub(super) fn checked_advance(buf: &[u8], offset: usize, size: usize) -> Option { - let end = offset + size; + checked_advance_len(buf, offset, size, 0) +} + +/// Return `Some(offset + header + len)` only if the buffer has enough bytes, +/// with both additions checked. +/// +/// Every length-prefixed tag (`str`, `bin`, `ext`) reaches this shape, and +/// `len` is read straight from the input as up to a `u32`. Folding it into the +/// header width first — `header + len` — is what overflows a 32-bit `usize` +/// before any bounds check can reject the value. +#[inline(always)] +pub(crate) fn checked_advance_len( + buf: &[u8], + offset: usize, + header: usize, + len: usize, +) -> Option { + let end = offset.checked_add(header)?.checked_add(len)?; if end <= buf.len() { Some(end) } else { None } }