diff --git a/const-oid/src/arcs.rs b/const-oid/src/arcs.rs index bbfa9f763..68294ec5f 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,28 @@ 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 { + // 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)) + { + 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();