diff --git a/README.md b/README.md index cc41776..443962e 100644 --- a/README.md +++ b/README.md @@ -258,7 +258,8 @@ allocating in proportion to any declared length. A rejection is `ByteStorageError::DeserializationFailed` with the message prefix `decode pre-scan: `. The same walk is public as `check_msgpack_structure(bytes, max_depth)` for callers that decode untrusted -MessagePack themselves. See [`SECURITY.md`](SECURITY.md#envelope-decode-bounds). +MessagePack themselves; it returns a `MsgpackStructureError` whose `Display` is +the bare reason, with no prefix. See [`SECURITY.md`](SECURITY.md#envelope-decode-bounds). diff --git a/SECURITY.md b/SECURITY.md index f183ae5..cc30301 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -154,10 +154,10 @@ the call fails. As with the size bound above, a caller that deserializes The walk itself is public as `check_msgpack_structure(bytes, max_depth)`, so a caller that decodes untrusted MessagePack outside `ByteStorage` can apply the -same rules at its own depth bound. It needs no optional feature. It returns the -bare reason for a rejection, with no prefix, and it does not enforce the -protocol's `32..=1024` range on `max_depth`: choosing the bound is the caller's -job. +same rules at its own depth bound. It needs no optional feature. It returns a +`MsgpackStructureError` whose `Display` is the bare reason, with no prefix, and +it does not enforce the protocol's `32..=1024` range on `max_depth`: choosing +the bound is the caller's job. ### Dependencies diff --git a/src/byte_storage.rs b/src/byte_storage.rs index 974c915..56d48b5 100644 --- a/src/byte_storage.rs +++ b/src/byte_storage.rs @@ -325,16 +325,14 @@ impl Default for ByteStorage { } } -/// Nesting bound for the envelope decode. The protocol requires 32..=1024; 100 -/// matches cachekit-rs and cachekit-ts. A legitimate envelope nests 2 deep. -#[cfg(all(feature = "compression", feature = "checksum", feature = "messagepack"))] -const MAX_DEPTH: usize = 100; - /// Decode untrusted envelope bytes: structural pre-scan first, then the typed /// decode. Serde's derive skips an unknown map key with `IgnoredAny`, which /// recurses, so the depth bound has to hold before `rmp_serde` sees the bytes. #[cfg(all(feature = "compression", feature = "checksum", feature = "messagepack"))] fn decode_envelope(envelope_bytes: &[u8]) -> Result { + // Nesting bound for the envelope decode. The protocol requires 32..=1024; + // 100 matches cachekit-rs and cachekit-ts. A legitimate envelope nests 2 deep. + const MAX_DEPTH: usize = 100; crate::check_msgpack_structure(envelope_bytes, MAX_DEPTH).map_err(|what| { ByteStorageError::DeserializationFailed(format!("decode pre-scan: {what}")) })?; diff --git a/src/lib.rs b/src/lib.rs index fc9ddcd..50d8f6d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -77,7 +77,7 @@ pub use byte_storage::{ByteStorage, StorageEnvelope}; // Structural pre-scan for untrusted MessagePack (no optional dependency) mod msgpack_bounds; -pub use msgpack_bounds::check_msgpack_structure; +pub use msgpack_bounds::{check_msgpack_structure, MsgpackStructureError}; // Encryption module (feature-gated) #[cfg(feature = "encryption")] diff --git a/src/msgpack_bounds.rs b/src/msgpack_bounds.rs index 527675d..7128ab0 100644 --- a/src/msgpack_bounds.rs +++ b/src/msgpack_bounds.rs @@ -1,23 +1,30 @@ //! Structural pre-scan for untrusted MessagePack. //! -//! This is the one shared structural walk. `ByteStorage::retrieve` runs it over -//! the envelope bytes before `rmp_serde` materialises a `StorageEnvelope` -//! (protocol `spec/wire-format.md` → Retrieve Flow, step 2), and SDK bindings -//! call it through the [`crate::check_msgpack_structure`] re-export before they -//! decode untrusted values. The bounds are `spec/interop-mode.md` → Decode -//! bounds, pinned by `tests/vectors/decode-bounds.json`. Depth counts every -//! collection header, an empty one included, which is how the spec defines it. -//! -//! The walk needs no optional dependency, so it is available under every -//! feature set. +//! `ByteStorage::retrieve` runs it before the envelope decode; it is exported +//! for SDK bindings that decode untrusted MessagePack themselves. The bounds are +//! `spec/interop-mode.md` → Decode bounds, pinned by +//! `tests/vectors/decode-bounds.json`. + +/// Why [`check_msgpack_structure`] rejected its input. +/// +/// `Display` is the bare reason with no prefix (for example +/// `nests deeper than 100 levels`), so each caller adds its own. The wording is +/// stable within a minor version: callers may match on its leading words. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +#[error("{0}")] +pub struct MsgpackStructureError(String); + +fn reject(reason: impl Into) -> MsgpackStructureError { + MsgpackStructureError(reason.into()) +} /// Checks that `bytes` hold one MessagePack document whose declared structure /// the input can actually back, before any decoder sees them. /// /// A header-only walk: str/bin/ext payloads are skipped by offset, never read, -/// and nothing is decoded. It allocates one `u64` per open non-empty -/// collection, so at most `max_depth` of them (8 KiB at a depth of 1024), and -/// nothing proportional to the input or to any declared length. +/// and nothing is decoded. It keeps one `u64` per open non-empty collection, so +/// at most `min(max_depth, bytes.len())` of them (8 KiB at a depth of 1024), and +/// allocates nothing proportional to any declared length. /// /// Trailing bytes after the root element are left to the decoder. /// @@ -27,8 +34,7 @@ /// /// # Errors /// -/// Returns the violated bound as a bare reason with no prefix, so each caller -/// adds its own. It rejects: +/// Returns a [`MsgpackStructureError`] naming the violated bound. It rejects: /// - nesting deeper than `max_depth`, counting every array or map header on /// the path (an empty one included); /// - a header declaring more payload bytes than the input holds; @@ -47,22 +53,27 @@ /// let doc = [0x91, 0x91, 0x90]; /// assert_eq!(check_msgpack_structure(&doc, 3), Ok(())); /// assert_eq!( -/// check_msgpack_structure(&doc, 2), -/// Err("nests deeper than 2 levels".to_owned()) +/// check_msgpack_structure(&doc, 2).unwrap_err().to_string(), +/// "nests deeper than 2 levels" /// ); /// /// // An array32 header claiming 2^32 - 1 elements, in five bytes. /// assert_eq!( -/// check_msgpack_structure(&[0xdd, 0xff, 0xff, 0xff, 0xff], 100), -/// Err("declares more elements than the input can back".to_owned()) +/// check_msgpack_structure(&[0xdd, 0xff, 0xff, 0xff, 0xff], 100) +/// .unwrap_err() +/// .to_string(), +/// "declares more elements than the input can back" /// ); /// ``` -pub fn check_msgpack_structure(bytes: &[u8], max_depth: usize) -> Result<(), String> { - fn be(bytes: &[u8], pos: usize, width: usize) -> Result { +pub fn check_msgpack_structure( + bytes: &[u8], + max_depth: usize, +) -> Result<(), MsgpackStructureError> { + fn be(bytes: &[u8], pos: usize, width: usize) -> Result { let end = pos .checked_add(width) .filter(|e| *e <= bytes.len()) - .ok_or_else(|| "ends inside a length prefix".to_owned())?; + .ok_or_else(|| reject("ends inside a length prefix"))?; Ok(bytes[pos..end] .iter() .fold(0u64, |acc, b| (acc << 8) | u64::from(*b))) @@ -77,7 +88,7 @@ pub fn check_msgpack_structure(bytes: &[u8], max_depth: usize) -> Result<(), Str } let marker = *bytes .get(pos) - .ok_or_else(|| "ends before the document is complete".to_owned())?; + .ok_or_else(|| reject("ends before the document is complete"))?; pos += 1; pending -= 1; if let Some(innermost) = open.last_mut() { @@ -89,7 +100,7 @@ pub fn check_msgpack_structure(bytes: &[u8], max_depth: usize) -> Result<(), Str 0x80..=0x8f => (0, 0, 2 * u64::from(marker & 0x0f)), 0x90..=0x9f => (0, 0, u64::from(marker & 0x0f)), 0xa0..=0xbf => (0, u64::from(marker & 0x1f), 0), - 0xc1 => return Err("contains the reserved marker 0xc1".to_owned()), + 0xc1 => return Err(reject("contains the reserved marker 0xc1")), 0xc4 | 0xd9 => (1, be(bytes, pos, 1)?, 0), 0xc5 | 0xda => (2, be(bytes, pos, 2)?, 0), 0xc6 | 0xdb => (4, be(bytes, pos, 4)?, 0), @@ -106,23 +117,23 @@ pub fn check_msgpack_structure(bytes: &[u8], max_depth: usize) -> Result<(), Str // An empty collection is still a level (spec: depth counts collection // headers), so the bound is checked before the `children > 0` push. if matches!(marker, 0x80..=0x9f | 0xdc..=0xdf) && open.len() >= max_depth { - return Err(format!("nests deeper than {max_depth} levels")); + return Err(reject(format!("nests deeper than {max_depth} levels"))); } pos += prefix; let remaining = (bytes.len() - pos) as u64; if payload > remaining { - return Err("declares more bytes than the input holds".to_owned()); + return Err(reject("declares more bytes than the input holds")); } // Unreachable while the check above holds (`remaining` came from a // usize); checked rather than cast so a future edit cannot truncate. pos += usize::try_from(payload) - .map_err(|_| "declares more bytes than the input holds".to_owned())?; + .map_err(|_| reject("declares more bytes than the input holds"))?; if children > 0 { open.push(children); } pending += children; if pending > remaining - payload { - return Err("declares more elements than the input can back".to_owned()); + return Err(reject("declares more elements than the input can back")); } } Ok(()) @@ -135,8 +146,14 @@ mod tests { /// The envelope's bound; any bound in the protocol's range would do here. const MAX_DEPTH: usize = 100; + #[test] + fn the_error_is_a_std_error() { + fn is_error() {} + is_error::(); + } + fn check(bytes: &[u8]) -> Result<(), String> { - check_msgpack_structure(bytes, MAX_DEPTH) + check_msgpack_structure(bytes, MAX_DEPTH).map_err(|e| e.to_string()) } /// `count` copies of a collection header, then `tail`.