From df3bba6068bc76dca71bd82b34f0972ae67a4563 Mon Sep 17 00:00:00 2001 From: jaideeppyne Date: Wed, 26 Aug 2026 10:07:27 +0530 Subject: [PATCH 1/2] const-oid: reject over-u32 arcs in BER decoder instead of truncating The BER byte decoder (`Arcs::try_next`) accumulated base-128 arc digits with an unchecked `result << 7` shift, guarded only by `arc_bytes > ARC_MAX_BYTES`. Because a `u32` arc can occupy up to five base-128 bytes, that guard never fired for a five-byte arc, so the final shift could push the value past `u32::MAX` and silently truncate it. As a result `ObjectIdentifier::from_bytes` accepted encodings denoting arcs greater than `u32::MAX` and returned a wrong value rather than `ArcTooBig`. For example `2A 90 80 80 80 00` (arc 2^32) decoded to `1.2.0` and `2A 90 80 80 80 05` (arc 2^32+5) decoded to `1.2.5`. Accumulate the digits with `checked_mul`/`checked_add` and return `ArcTooBig` on overflow. The largest in-range arc (`u32::MAX`, `2A 8F FF FF FF 7F`) still decodes correctly. Add a regression test. --- const-oid/src/arcs.rs | 40 +++++++++++++++++++--------------------- const-oid/tests/oid.rs | 29 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 21 deletions(-) diff --git a/const-oid/src/arcs.rs b/const-oid/src/arcs.rs index bbfa9f763..a306325ad 100644 --- a/const-oid/src/arcs.rs +++ b/const-oid/src/arcs.rs @@ -24,15 +24,6 @@ pub(crate) const ARC_MAX_FIRST: Arc = 2; /// Maximum value of the second arc in an OID. pub(crate) const ARC_MAX_SECOND: Arc = 39; -/// Maximum number of bytes supported in an arc. -/// -/// Note that OIDs are base 128 encoded (with continuation bits), so we must consider how many bytes -/// are required when each byte can only represent 7-bits of the input. -const ARC_MAX_BYTES: usize = (Arc::BITS as usize).div_ceil(7); - -/// Maximum value of the last byte in an arc. -const ARC_MAX_LAST_OCTET: u8 = 0b11110000; // Max bytes of leading 1-bits - /// [`Iterator`] over [`Arc`] values (a.k.a. nodes) in an [`ObjectIdentifier`]. /// /// This iterates over all arcs in an OID, including the root. @@ -72,27 +63,34 @@ impl<'a> Arcs<'a> { Ok(Some(root.second_arc())) } Some(offset) => { - let mut result = 0; + let mut result: Arc = 0; let mut arc_bytes = 0; loop { let len = checked_add!(offset, arc_bytes); match self.bytes.get(len).cloned() { - // The arithmetic below includes advance checks - // against `ARC_MAX_BYTES` and `ARC_MAX_LAST_OCTET` - // which ensure the operations will not overflow. - #[allow(clippy::arithmetic_side_effects)] Some(byte) => { arc_bytes = checked_add!(arc_bytes, 1); - if (arc_bytes > ARC_MAX_BYTES) && (byte & ARC_MAX_LAST_OCTET != 0) { - return Err(Error::ArcTooBig); - } - - result = (result << 7) | (byte & 0b1111111) as Arc; - - if byte & 0b10000000 == 0 { + // Accumulate the base 128 digits with overflow checking. + // + // An arc whose value does not fit in `Arc` (`u32`) is + // rejected as `ArcTooBig` rather than being silently + // truncated by the `<< 7` shift. Note that a `u32` arc can + // be up to five base 128 bytes long, so checking the number + // of consumed bytes alone is not sufficient: the final byte + // of a five-byte arc can still push the value past + // `u32::MAX`. + result = match result + .checked_mul(0x80) + .and_then(|result| result.checked_add((byte & 0b0111_1111) as Arc)) + { + Some(result) => result, + None => return Err(Error::ArcTooBig), + }; + + if byte & 0b1000_0000 == 0 { self.cursor = Some(checked_add!(offset, arc_bytes)); return Ok(Some(result)); } diff --git a/const-oid/tests/oid.rs b/const-oid/tests/oid.rs index 92bfc49c4..5e6005376 100644 --- a/const-oid/tests/oid.rs +++ b/const-oid/tests/oid.rs @@ -113,6 +113,35 @@ fn from_bytes_oid_largearc_2() { assert_eq!(ObjectIdentifier::from_bytes(&[]), Err(Error::Empty)); } +/// An arc whose base 128 encoding denotes a value greater than `u32::MAX` (the +/// `Arc` bound) must be rejected rather than silently truncated by the decoder. +/// +/// `u32::MAX` (`2A 8F FF FF FF 7F`) is the largest valid arc; incrementing the +/// leading arc byte to `0x90` denotes `2^32` (and `... 05` denotes `2^32 + 5`), +/// both of which exceed `u32::MAX`. +#[test] +fn from_bytes_reject_arc_above_u32_max() { + // Sanity check: the largest in-range arc (`u32::MAX`) still decodes. + assert_eq!( + ObjectIdentifier::from_bytes(&[0x2A, 0x8F, 0xFF, 0xFF, 0xFF, 0x7F]) + .unwrap() + .arc(2), + Some(4294967295), + ); + + // `1.2.4294967296` (`2^32`) -- previously truncated to `1.2.0`. + assert_eq!( + ObjectIdentifier::from_bytes(&[0x2A, 0x90, 0x80, 0x80, 0x80, 0x00]), + Err(Error::ArcTooBig), + ); + + // `1.2.4294967301` (`2^32 + 5`) -- previously truncated to `1.2.5`. + assert_eq!( + ObjectIdentifier::from_bytes(&[0x2A, 0x90, 0x80, 0x80, 0x80, 0x05]), + Err(Error::ArcTooBig), + ); +} + #[test] fn from_str() { let oid0 = EXAMPLE_OID_0_STR.parse::().unwrap(); From d2ea21abf74ec2bf355ff36418ea760a65238a9b Mon Sep 17 00:00:00 2001 From: jaideeppyne Date: Fri, 28 Aug 2026 19:36:01 +0530 Subject: [PATCH 2/2] Shorten the overflow comment --- const-oid/src/arcs.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/const-oid/src/arcs.rs b/const-oid/src/arcs.rs index a306325ad..68294ec5f 100644 --- a/const-oid/src/arcs.rs +++ b/const-oid/src/arcs.rs @@ -73,15 +73,9 @@ impl<'a> Arcs<'a> { Some(byte) => { arc_bytes = checked_add!(arc_bytes, 1); - // Accumulate the base 128 digits with overflow checking. - // - // An arc whose value does not fit in `Arc` (`u32`) is - // rejected as `ArcTooBig` rather than being silently - // truncated by the `<< 7` shift. Note that a `u32` arc can - // be up to five base 128 bytes long, so checking the number - // of consumed bytes alone is not sufficient: the final byte - // of a five-byte arc can still push the value past - // `u32::MAX`. + // A five byte arc can still exceed `Arc`, so the digits are + // accumulated with overflow checking rather than by counting + // bytes. result = match result .checked_mul(0x80) .and_then(|result| result.checked_add((byte & 0b0111_1111) as Arc))