From f06c126c3bb01cf4eba1bcbd541aeb96ce165f08 Mon Sep 17 00:00:00 2001 From: Dave Grantham Date: Thu, 16 Jul 2026 21:14:28 -0600 Subject: [PATCH] hardening Signed-off-by: Dave Grantham --- Cargo.toml | 4 +- src/base_encoded.rs | 20 +++++- src/lib.rs | 8 +-- src/serde/de.rs | 145 ++++++++++++++++++++++++++++++++++++++------ src/serde/mod.rs | 91 +++++++++++++++++++++++++++ src/varbytes.rs | 9 ++- 6 files changed, 249 insertions(+), 28 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index ccefa14..a1ea715 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "multi-util" -version = "1.0.3" +version = "1.0.4" edition = "2024" rust-version = "1.85" authors = ["Dave Huseby "] @@ -18,7 +18,7 @@ default = ["serde"] multi-base = "1.0" multi-codec = "1.0" multi-trait = "1.0" -serde = { version = "1.0", default-features = false, features = ["alloc"], optional = true } +serde = { version = "1.0", default-features = false, features = ["std"], optional = true } thiserror = { version = "2.0" } [dev-dependencies] diff --git a/src/base_encoded.rs b/src/base_encoded.rs index 678a001..ba11f06 100644 --- a/src/base_encoded.rs +++ b/src/base_encoded.rs @@ -12,7 +12,7 @@ use core::{ /// Smart pointer for multibase encoded data. This supports encoding to and /// decoding from multibase encoding strings using [`TryFrom<&str>`] and -/// [`to_string()`](core::fmt::Display::to_string) +/// `to_string()` (via [`fmt::Display`]) #[derive(Clone)] pub struct BaseEncoded where @@ -141,7 +141,14 @@ where Enc: BaseEncoder, { fn hash(&self, state: &mut H) { - self.to_string().hash(state); + // Hash the raw encoded bytes directly instead of going through + // `Display` (which would allocate a `String` and format the base + // encoding). This still allocates a `Vec` for the `Into` + // conversion but avoids the much more expensive `Display` + `String` + // formatting path. + let bytes: Vec = self.t.clone().into(); + bytes.as_slice().hash(state); + self.base.hash(state); } } @@ -175,6 +182,13 @@ where Enc: BaseEncoder, { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + // Note: the pre-fix path was `self.t.clone().into()` which allocates + // a `Vec`. A zero-allocation `AsRef<[u8]>` path would require + // adding that bound to `T`, which is breaking for types like + // `Varuint` whose inner value is not a contiguous byte buffer. + // The `Hash` impl below has been optimised to avoid going through + // `Display`/`String`; this `Display` impl retains the single + // `Vec` allocation required to hand `&[u8]` to the base encoder. write!( f, "{}", @@ -185,7 +199,7 @@ where impl fmt::Debug for BaseEncoded where - T: fmt::Debug + EncodingInfo + Clone + Into>, + T: fmt::Debug + EncodingInfo, Enc: BaseEncoder, { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { diff --git a/src/lib.rs b/src/lib.rs index 9486fd0..eb11fd0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3,13 +3,11 @@ //! //! Multiformat utility functions and types. //! -//! ## `no_std` Note +//! ## `std` Requirement //! //! This crate is currently std-only. The `serde` dependency is configured with -//! `default-features = false, features = ["alloc"]` so that the serde layer -//! itself does not pull in std, but the crate as a whole still requires std -//! via `thiserror` and the lack of a `#![no_std]` crate attribute. A future -//! release may introduce a `std` feature gate and full `no_std` support. +//! `features = ["std"]` to match. A future release may introduce a `std` +//! feature gate and full `no_std` + `alloc` support. #![warn(missing_docs)] #![deny( unsafe_code, diff --git a/src/serde/de.rs b/src/serde/de.rs index 9c3b93a..7505e91 100644 --- a/src/serde/de.rs +++ b/src/serde/de.rs @@ -1,5 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 -use crate::{BaseEncoded, BaseEncoder, EncodingInfo, Varbytes, Varuint}; +use crate::{ + BaseEncoded, BaseEncoder, EncodingInfo, Varbytes, Varuint, varbytes::MAX_DECODED_SIZE, +}; use core::{fmt, marker}; use multi_base::Base; use multi_trait::prelude::TryDecodeFrom; @@ -193,10 +195,7 @@ impl<'de> de::Deserialize<'de> for Varbytes { where E: de::Error, { - let (len, ptr) = usize::try_decode_from(v) - .map_err(|_| de::Error::custom("failed to deserialize varuint len"))?; - let v = ptr[..len].to_vec(); - Ok(Varbytes::new(v)) + decode_varbytes(v, MAX_DECODED_SIZE) } #[inline] @@ -204,10 +203,7 @@ impl<'de> de::Deserialize<'de> for Varbytes { where E: de::Error, { - let (len, ptr) = usize::try_decode_from(v) - .map_err(|_| de::Error::custom("failed to deserialize varuint len"))?; - let v = ptr[..len].to_vec(); - Ok(Varbytes::new(v)) + decode_varbytes(v, MAX_DECODED_SIZE) } // longest lifetime @@ -216,10 +212,7 @@ impl<'de> de::Deserialize<'de> for Varbytes { where E: de::Error, { - let (len, ptr) = usize::try_decode_from(v.as_slice()) - .map_err(|_| de::Error::custom("failed to deserialize varuint len"))?; - let v = ptr[..len].to_vec(); - Ok(Varbytes::new(v)) + decode_varbytes(v.as_slice(), MAX_DECODED_SIZE) } // binary / human readable @@ -235,13 +228,131 @@ impl<'de> de::Deserialize<'de> for Varbytes { while let Some(b) = seq.next_element()? { v.push(b); } - let (len, ptr) = usize::try_decode_from(v.as_slice()) - .map_err(|_| de::Error::custom("failed to deserialize varuint len"))?; - let v = ptr[..len].to_vec(); - Ok(Varbytes::new(v)) + decode_varbytes(v.as_slice(), MAX_DECODED_SIZE) } } deserializer.deserialize_bytes(VarbytesVisitor) } } + +/// Decode a `Varbytes` value from a byte slice with a caller-supplied maximum +/// decoded size. +/// +/// This is the shared bounds-checked decode path used by the [`Varbytes`] +/// [`Deserialize`](de::Deserialize) impl and by +/// [`deserialize_varbytes_with_max`]. It mirrors the safety checks in +/// [`Varbytes::try_decode_from`](crate::Varbytes::try_decode_from): after +/// decoding the length prefix, it rejects lengths that exceed the available +/// buffer (preventing an out-of-bounds read / panic) and lengths that exceed +/// the supplied maximum (preventing unbounded allocation). +fn decode_varbytes(input: &[u8], max: usize) -> Result +where + E: de::Error, +{ + let (len, ptr) = usize::try_decode_from(input) + .map_err(|_| de::Error::custom("failed to deserialize varuint len"))?; + + if len > max { + return Err(de::Error::custom("varbytes length exceeds maximum")); + } + if len > ptr.len() { + return Err(de::Error::custom("varbytes length exceeds buffer")); + } + + let v = ptr[..len].to_vec(); + Ok(Varbytes::new(v)) +} + +/// Deserialize a [`Varbytes`] with a caller-specified maximum decoded size, +/// overriding the default [`MAX_DECODED_SIZE`] cap. +/// +/// Use this with `#[serde(deserialize_with = "...")]` when a field needs a +/// tighter or looser bound than the crate-wide default: +/// +/// ``` +/// # use serde::Deserialize; +/// # use multi_util::Varbytes; +/// # use multi_util::serde::deserialize_varbytes_with_max; +/// #[derive(Deserialize)] +/// struct Small { +/// #[serde(deserialize_with = "deserialize_varbytes_with_max_256")] +/// data: Varbytes, +/// } +/// +/// // Wrapper that fixes the max at 256 bytes for use with +/// // `#[serde(deserialize_with = "...")]` (which requires an `fn(D) -> Result`). +/// fn deserialize_varbytes_with_max_256<'de, D>(deserializer: D) -> Result +/// where +/// D: serde::Deserializer<'de>, +/// { +/// deserialize_varbytes_with_max(deserializer, 256) +/// } +/// ``` +/// +/// The `max` argument is the maximum number of bytes the decoded `Varbytes` +/// value may contain. A length prefix claiming more than `max` bytes (or more +/// bytes than remain in the buffer) is rejected with a clean `Err`, never a +/// panic. +/// +/// # Errors +/// +/// Returns a deserializer error if: +/// - the length prefix cannot be decoded as a varuint, +/// - the decoded length exceeds `max`, +/// - the decoded length exceeds the remaining buffer. +pub fn deserialize_varbytes_with_max<'de, D>( + deserializer: D, + max: usize, +) -> Result +where + D: de::Deserializer<'de>, +{ + struct VarbytesVisitorMax(usize); + + impl<'de> de::Visitor<'de> for VarbytesVisitorMax { + type Value = Varbytes; + + fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "varuint encoded len followed by bytes") + } + + #[inline] + fn visit_borrowed_bytes(self, v: &'de [u8]) -> Result + where + E: de::Error, + { + decode_varbytes(v, self.0) + } + + #[inline] + fn visit_bytes(self, v: &[u8]) -> Result + where + E: de::Error, + { + decode_varbytes(v, self.0) + } + + #[inline] + fn visit_byte_buf(self, v: Vec) -> Result + where + E: de::Error, + { + decode_varbytes(v.as_slice(), self.0) + } + + #[inline] + fn visit_seq(self, mut seq: S) -> Result + where + S: de::SeqAccess<'de>, + { + let mut v = Vec::new(); + while let Some(b) = seq.next_element()? { + v.push(b); + } + decode_varbytes(v.as_slice(), self.0) + } + } + + deserializer.deserialize_bytes(VarbytesVisitorMax(max)) +} diff --git a/src/serde/mod.rs b/src/serde/mod.rs index 615be35..d718143 100644 --- a/src/serde/mod.rs +++ b/src/serde/mod.rs @@ -3,6 +3,8 @@ mod de; mod ser; +pub use de::deserialize_varbytes_with_max; + #[cfg(test)] mod tests { use crate::prelude::*; @@ -285,4 +287,93 @@ mod tests { let v = Varbytes::encoded_new(Base::Base16Lower, vec![0x01, 0x02, 0x03]); assert_tokens(&v.readable(), &[Token::Str("f03010203")]); } + + // ======================================================================== + // H4: serde Varbytes bounds-check tests + // ======================================================================== + + #[test] + fn test_varbytes_serde_len_exceeds_buffer_is_err_not_panic() { + // Crafted payload: length prefix claims 4 bytes but only 3 follow. + // Pre-fix this panicked with an index-out-of-bounds; it must now + // return a clean Err. + let malicious: &[u8] = &[0x04, 0x01, 0x02, 0x03]; + + let result: Result = + serde_json::from_slice(serde_json::to_string(malicious).unwrap().as_bytes()); + // serde_json hands the bytes to the visitor; the bounds check must + // reject the over-long length claim without panicking. + // (Depending on the format the bytes may arrive via visit_bytes or + // visit_seq; either way the shared decode_varbytes helper rejects it.) + assert!(result.is_err()); + } + + #[test] + fn test_varbytes_serde_len_exceeds_buffer_binary() { + // Direct binary deserialization via serde_cbor to exercise the + // visit_bytes / visit_byte_buf paths. + let malicious: &[u8] = &[0x04, 0x01, 0x02, 0x03]; + let result: Result = serde_cbor::from_slice(malicious); + assert!(result.is_err(), "must reject len > buffer, not panic"); + } + + #[test] + fn test_varbytes_serde_len_exceeds_max_is_err() { + // Length prefix claims just over MAX_DECODED_SIZE. The buffer is + // trivially small so this also exceeds the buffer; the key property + // is that it returns Err rather than attempting a huge allocation. + use multi_trait::EncodeInto; + let over_max = MAX_DECODED_SIZE + 1; + let mut payload = Vec::new(); + payload.extend(over_max.encode_into()); + payload.extend(&[0u8; 4]); // a few bytes, nowhere near over_max + + let result: Result = serde_cbor::from_slice(&payload); + assert!(result.is_err(), "must reject len > MAX_DECODED_SIZE"); + } + + #[test] + fn test_varbytes_serde_len_just_under_max_ok() { + // A length just under MAX_DECODED_SIZE with a matching buffer is + // accepted by the cap. We don't actually allocate ~16 MiB here; we + // verify the cap logic via the configurable helper with a small max. + use super::deserialize_varbytes_with_max; + use multi_trait::EncodeInto; + + let max = 8usize; + let data = vec![0xABu8; max]; + let mut payload = Vec::new(); + payload.extend(max.encode_into()); + payload.extend(&data); + + let de = serde::de::value::BytesDeserializer::::new(&payload); + let v: Varbytes = + deserialize_varbytes_with_max(de, max).expect("len == max should be accepted"); + assert_eq!(v.as_bytes(), &data[..]); + } + + #[test] + fn test_varbytes_serde_len_just_over_max_is_err() { + use super::deserialize_varbytes_with_max; + use multi_trait::EncodeInto; + + let max = 8usize; + let over = max + 1; + let mut payload = Vec::new(); + payload.extend(over.encode_into()); + payload.extend(vec![0u8; over]); // buffer is large enough; only the cap rejects + + let de = serde::de::value::BytesDeserializer::::new(&payload); + let result = deserialize_varbytes_with_max(de, max); + assert!(result.is_err(), "len just over max must be rejected"); + } + + #[test] + fn test_varbytes_serde_valid_roundtrip() { + // Sanity: a well-formed varbytes still round-trips through serde. + let v = Varbytes::new(vec![0xDE, 0xAD, 0xBE, 0xEF]); + let encoded = serde_cbor::to_vec(&v).expect("serialize"); + let decoded: Varbytes = serde_cbor::from_slice(&encoded).expect("deserialize"); + assert_eq!(v, decoded); + } } diff --git a/src/varbytes.rs b/src/varbytes.rs index af97bcd..6a0962f 100644 --- a/src/varbytes.rs +++ b/src/varbytes.rs @@ -23,7 +23,7 @@ pub const MAX_DECODED_SIZE: usize = 16 * 1024 * 1024; pub type EncodedVarbytes = BaseEncoded; impl Varbytes { - /// Create a new Varbytes from a Vec + /// Create a new Varbytes from a `Vec` #[must_use] pub const fn new(data: Vec) -> Self { Self(data) @@ -68,6 +68,13 @@ impl ops::Deref for Varbytes { } } +impl AsRef<[u8]> for Varbytes { + #[inline] + fn as_ref(&self) -> &[u8] { + &self.0 + } +} + impl EncodingInfo for Varbytes { fn preferred_encoding() -> Base { Base::Base16Lower