From 7b2bb3eea12475e5b16f429872d4ce67126824d9 Mon Sep 17 00:00:00 2001 From: Joel DSouza Date: Sat, 4 Jul 2026 21:49:16 +0200 Subject: [PATCH] fix(#76)!: checked size arithmetic in MatterBuilder variable-size parse The qb64/qb2 variable-size parse paths computed the frame size from the attacker-controlled decoded soft `size` with unchecked arithmetic (`fs = size * 4 + cs`, `bfs = ceil(fs * 3 / 4)`), violating the arithmetic-safety mandatory rule. In debug this panics on overflow (DoS on untrusted input); in release it wraps to a small bogus `fs` that then slices a truncated frame as valid (silently-wrong parse). Extract the three sites (from_qualified_base64 :132, from_qualified_base2 :263/:265) into checked helpers `compute_full_size` / `compute_qb2_byte_size` using checked_mul/checked_add, returning a new typed `ValidationError::SizeOverflow` on overflow. BREAKING CHANGE: `ValidationError` gains a `SizeOverflow` variant; as the enum is public and not `#[non_exhaustive]`, downstream exhaustive matches must handle it. MINOR under the 0.x SemVer convention. Latent, not reachable through the parse API today: variable codes cap the soft field at ss=4, so size <= 2^24-1 and size*4 fits even 32-bit usize (wasm32). Fixed as a rule violation + defense-in-depth. Tests probe the checked helpers directly (RED: bare arithmetic panics on overflow; GREEN: typed Err) since the overflow is unreachable via from_qualified_base64/base2. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 12 ++++++ src/core/matter/builder.rs | 87 ++++++++++++++++++++++++++++++++++++-- src/core/matter/error.rs | 8 ++++ 3 files changed, 103 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e2d5b31..fd14076 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **core (#76):** `ValidationError` gains a new `SizeOverflow` variant, returned + when computing a variable-size primitive's full size from its decoded soft field + overflows `usize`. As `ValidationError` is public and not `#[non_exhaustive]`, + this is **breaking** (MINOR under 0.x) for downstream exhaustive `match` on it. - **error ergonomics (#33):** removed the `terrors::OneOf` error-union layer in favour of purpose-built `thiserror` enums. **Breaking** (MINOR under 0.x): - `MatterBuilder::{from_qualified_base64, from_qualified_base2, build}` now return @@ -28,6 +32,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **core (#76):** `MatterBuilder::{from_qualified_base64, from_qualified_base2}` no + longer compute the frame size (`fs = size * 4 + cs`, `bfs = ceil(fs * 3 / 4)`) from + the attacker-controlled soft field with unchecked arithmetic. The three sites now + use `checked_mul`/`checked_add` and return `ValidationError::SizeOverflow` on + overflow, per the arithmetic-safety rule — previously a large declared size would + panic in debug or silently wrap in release (a truncated frame slicing as valid). + Latent, not reachable through the parse API today (variable codes cap the soft + field at `ss=4`, so `size <= 2^24-1`); fixed as a defense-in-depth rule violation. - **serder (#33):** a malformed-but-unparseable field value no longer collapses a `ParsingError` into `ValidationError::UnknownMatterCode(..)` via string formatting; a new `SerderError::UnparseablePrimitive { field, source }` variant diff --git a/src/core/matter/builder.rs b/src/core/matter/builder.rs index 5b29e40..2c87d14 100644 --- a/src/core/matter/builder.rs +++ b/src/core/matter/builder.rs @@ -129,7 +129,7 @@ impl MatterBuilder { } else { let size: usize = decode_int(soft_str) .map_err(|err| MatterBuildError::from(ParsingError::Conversion(err)))?; - (size * 4) + cs + compute_full_size(size, cs)? }; if stream.len() < fs { return Err(MatterBuildError::from(ValidationError::IncorrectRawSize { @@ -260,9 +260,9 @@ impl MatterBuilder { } else { let size: usize = decode_int(soft_tail) .map_err(|err| MatterBuildError::from(ParsingError::Conversion(err)))?; - (size * 4) + cs + compute_full_size(size, cs)? }; - let bfs = (fs * 3).div_ceil(4); + let bfs = compute_qb2_byte_size(fs)?; if stream.len() < bfs { return Err(MatterBuildError::from(ValidationError::IncorrectRawSize { code: code.to_string(), @@ -495,6 +495,32 @@ const fn get_size(raw_len: usize, lead_len: usize) -> usize { (raw_len + lead_len) / 3 } +/// Computes the full character size `fs` of a variable-size primitive from its +/// decoded soft `size` (`fs = size * 4 + cs`). +/// +/// `size` is decoded from the attacker-controlled soft field, so the arithmetic +/// is checked: an overflow yields [`ValidationError::SizeOverflow`] rather than a +/// debug panic or a silently-wrapped (truncated) frame. +#[inline] +fn compute_full_size(size: usize, cs: usize) -> Result { + size.checked_mul(4) + .and_then(|quad| quad.checked_add(cs)) + .ok_or(ValidationError::SizeOverflow) +} + +/// Computes the full binary (qb2) byte size `bfs` from the character size `fs` +/// (`bfs = ceil(fs * 3 / 4)`). +/// +/// `fs` derives from the attacker-controlled soft field via [`compute_full_size`], +/// so the multiplication is checked; an overflow yields +/// [`ValidationError::SizeOverflow`]. +#[inline] +fn compute_qb2_byte_size(fs: usize) -> Result { + fs.checked_mul(3) + .map(|tripled| tripled.div_ceil(4)) + .ok_or(ValidationError::SizeOverflow) +} + #[inline] fn extract_soft(code: MatterCode, ss: u8, xs: u8, soft: &str) -> Result<&str, ValidationError> { let expected_len = usize::from(ss - xs); @@ -546,7 +572,10 @@ fn validate_and_trim_raw( #[cfg(test)] #[allow(clippy::panic, reason = "tests use panic via unwrap/assert macros")] mod tests { - use super::{MatterBuildError, MatterBuilder, Start, ValidationError, validate_and_trim_raw}; + use super::{ + MatterBuildError, MatterBuilder, Start, ValidationError, compute_full_size, + compute_qb2_byte_size, validate_and_trim_raw, + }; use crate::core::matter::code::MatterCode; use std::{format, string::String, vec, vec::Vec}; @@ -569,6 +598,56 @@ mod tests { ); } + // ── Size-arithmetic overflow tests (#76) ──────────────────────────── + // `size` is decoded from the attacker-controlled soft field. Computing the + // frame size with bare arithmetic panics on overflow (debug) or wraps to a + // small bogus size (release) that then slices a truncated frame as valid. + // These probe the checked helpers directly because the parse API caps the + // soft field at ss=4 (size <= 2^24-1), so overflow is unreachable through + // `from_qualified_base64`/`from_qualified_base2` today — it is latent, and + // must still be rejected as a typed error, never panic or wrap. + + #[test] + fn compute_full_size_rejects_overflow() { + // size * 4 overflows usize. + let err = compute_full_size(usize::MAX / 2, 4) + .expect_err("size * 4 overflow must be a typed Err, not a panic or wrap"); + assert_eq!(err, ValidationError::SizeOverflow); + } + + #[test] + fn compute_full_size_rejects_add_overflow() { + // size * 4 fits but + cs overflows. + let err = compute_full_size(usize::MAX / 4, 8) + .expect_err("+ cs overflow must be a typed Err, not a panic or wrap"); + assert_eq!(err, ValidationError::SizeOverflow); + } + + #[test] + fn compute_full_size_in_range_is_exact() { + assert_eq!(compute_full_size(10, 4), Ok(44)); + assert_eq!(compute_full_size(0, 2), Ok(2)); + // Widest real variable code: ss=4 => size <= 2^24-1, must not overflow. + let max_real = 64_usize.pow(4) - 1; + assert_eq!(compute_full_size(max_real, 4), Ok((max_real * 4) + 4)); + } + + #[test] + fn compute_qb2_byte_size_rejects_overflow() { + // fs * 3 overflows usize. + let err = compute_qb2_byte_size(usize::MAX / 2) + .expect_err("fs * 3 overflow must be a typed Err, not a panic or wrap"); + assert_eq!(err, ValidationError::SizeOverflow); + } + + #[test] + fn compute_qb2_byte_size_in_range_is_exact() { + // ceil(44 * 3 / 4) = 33. + assert_eq!(compute_qb2_byte_size(44), Ok(33)); + // ceil(2 * 3 / 4) = 2. + assert_eq!(compute_qb2_byte_size(2), Ok(2)); + } + #[test] fn should_extract_raw_size_based() { use alloc::borrow::Cow; diff --git a/src/core/matter/error.rs b/src/core/matter/error.rs index e936835..94fefe8 100644 --- a/src/core/matter/error.rs +++ b/src/core/matter/error.rs @@ -168,6 +168,14 @@ pub enum ValidationError { "Structural integrity error: the parsed components do not match the expected total length of the primitive." )] StructuralIntegrityError, + + /// Computing the primitive's full size from the decoded soft field overflowed + /// `usize`. The soft field is attacker-controlled; a declared size this large + /// cannot describe a real frame, so it is rejected rather than wrapped. + #[error( + "Declared variable size is too large: computing the primitive's full size overflowed the address space." + )] + SizeOverflow, } /// Error returned by [`MatterBuilder`](super::builder::MatterBuilder) parse and