Bound CBOR container lengths and nesting depth when decoding

fuzz/.gitignore

@@ -0,0 +1,4 @@
+target
+corpus
+artifacts
+coverage

fuzz/Cargo.toml

@@ -0,0 +1,21 @@
+[package]
+name = "common-access-token-fuzz"
+version = "0.0.0"
+publish = false
+edition = "2021"
+
+[package.metadata]
+cargo-fuzz = true
+
+[dependencies]
+libfuzzer-sys = "0.4"
+
+[dependencies.common-access-token]
+path = ".."
+
+[[bin]]
+name = "decode_token"
+path = "fuzz_targets/decode_token.rs"
+test = false
+doc = false
+bench = false

fuzz/fuzz_targets/decode_token.rs

@@ -0,0 +1,16 @@
+//! Fuzz `Token::from_bytes` and what a verifier does with a decoded token.
+//!
+//! Run with `cargo +nightly fuzz run decode_token`. Any panic, abort or stack
+//! overflow on arbitrary input is a bug: decoding untrusted tokens must only
+//! ever return `Ok` or `Err`.
+#![no_main]
+
+use common_access_token::{Token, VerificationOptions};
+use libfuzzer_sys::fuzz_target;
+
+fuzz_target!(|data: &[u8]| {
+    if let Ok(token) = Token::from_bytes(data) {
+        let _ = token.to_bytes();
+        let _ = token.verify_claims(&VerificationOptions::new());
+    }
+});

src/token.rs

@@ -1604,10 +1604,52 @@ fn decode_map(bytes: &[u8]) -> Result<HeaderMap, Error> {
     decode_map_direct(&mut dec)
 }
 
+/// Maximum nesting depth of maps and arrays accepted when decoding.
+///
+/// Decoding recurses once per level, so without a bound a token nesting
+/// containers deeply enough overflows the stack and aborts the process. CWT,
+/// COSE and CAT structures nest only a few levels; 32 leaves ample headroom.
+const MAX_NESTING_DEPTH: usize = 32;
+
+/// Upper bound on the capacity reserved up front for a decoded array.
+const MAX_PREALLOCATED_ITEMS: usize = 16;
+
+/// Read a definite container length, rejecting lengths the remaining input
+/// can't possibly hold (each item takes at least `min_item_bytes`). This keeps
+/// a declared length from driving allocation or iteration beyond the input.
+fn container_len(
+    dec: &Decoder<'_>,
+    len: Option<u64>,
+    min_item_bytes: u64,
+    kind: &str,
+) -> Result<usize, Error> {
+    let len = len.ok_or_else(|| {
+        Error::InvalidFormat(format!("indefinite-length {kind}s are not supported"))
+    })?;
+    let remaining = (dec.input().len() - dec.position()) as u64;
+    if len > remaining / min_item_bytes {
+        return Err(Error::InvalidFormat(format!(
+            "{kind} declares {len} items but only {remaining} bytes remain"
+        )));
+    }
+    // Bounded by the input length, so this fits in usize
+    Ok(len as usize)
+}
+
 /// Decode a CBOR array
-fn decode_array(dec: &mut Decoder<'_>) -> Result<Vec<CborValue>, Error> {
-    let array_len = dec.array()?.unwrap_or(0);
-    let mut array = Vec::with_capacity(array_len as usize);
+fn decode_array(dec: &mut Decoder<'_>, depth: usize) -> Result<Vec<CborValue>, Error> {
+    if depth >= MAX_NESTING_DEPTH {
+        return Err(Error::InvalidFormat(format!(
+            "CBOR nesting exceeds the maximum depth of {MAX_NESTING_DEPTH}"
+        )));
+    }
+    let array_len = dec.array()?;
+    let array_len = container_len(dec, array_len, 1, "array")?;
+    // Don't size the Vec from the declared length: nested arrays would each
+    // reserve room for the whole remaining input at once, multiplying memory
+    // use far beyond the input size before the token is rejected. Grow as
+    // items are actually decoded instead.
+    let mut array = Vec::with_capacity(array_len.min(MAX_PREALLOCATED_ITEMS));
 
     for _ in 0..array_len {
         // Try to decode based on the datatype
@@ -1644,11 +1686,11 @@ fn decode_array(dec: &mut Decoder<'_>) -> Result<Vec<CborValue>, Error> {
             CborValue::Text(s.to_string())
         } else if datatype == minicbor::data::Type::Map {
             // Nested map
-            let nested_map = decode_map_direct(dec)?;
+            let nested_map = decode_map_nested(dec, depth + 1)?;
             CborValue::Map(nested_map)
         } else if datatype == minicbor::data::Type::Array {
             // Nested array
-            let nested_array = decode_array(dec)?;
+            let nested_array = decode_array(dec, depth + 1)?;
             CborValue::Array(nested_array)
         } else if datatype == minicbor::data::Type::Null {
             // Null value
@@ -1668,7 +1710,19 @@ fn decode_array(dec: &mut Decoder<'_>) -> Result<Vec<CborValue>, Error> {
 }
 
 fn decode_map_direct(dec: &mut Decoder<'_>) -> Result<HeaderMap, Error> {
-    let map_len = dec.map()?.unwrap_or(0);
+    decode_map_nested(dec, 0)
+}
+
+/// Decode a CBOR map at nesting `depth` (see [`MAX_NESTING_DEPTH`])
+fn decode_map_nested(dec: &mut Decoder<'_>, depth: usize) -> Result<HeaderMap, Error> {
+    if depth >= MAX_NESTING_DEPTH {
+        return Err(Error::InvalidFormat(format!(
+            "CBOR nesting exceeds the maximum depth of {MAX_NESTING_DEPTH}"
+        )));
+    }
+    let map_len = dec.map()?;
+    // Each entry is a key and a value, at least one byte each
+    let map_len = container_len(dec, map_len, 2, "map")?;
     let mut map = HeaderMap::new();
 
     for _ in 0..map_len {
@@ -1708,11 +1762,11 @@ fn decode_map_direct(dec: &mut Decoder<'_>) -> Result<HeaderMap, Error> {
             CborValue::Text(s.to_string())
         } else if datatype == minicbor::data::Type::Map {
             // Nested map
-            let nested_map = decode_map_direct(dec)?;
+            let nested_map = decode_map_nested(dec, depth + 1)?;
             CborValue::Map(nested_map)
         } else if datatype == minicbor::data::Type::Array {
             // Array
-            let array = decode_array(dec)?;
+            let array = decode_array(dec, depth + 1)?;
             CborValue::Array(array)
         } else if datatype == minicbor::data::Type::Null {
             // Null value
@@ -1730,3 +1784,131 @@ fn decode_map_direct(dec: &mut Decoder<'_>) -> Result<HeaderMap, Error> {
 
     Ok(map)
 }
+
+#[cfg(test)]
+mod decode_limit_tests {
+    use super::*;
+
+    /// A COSE_Mac0-shaped array whose protected header, unprotected header and
+    /// payload are supplied as raw encoded bytes (the protected header and
+    /// payload are wrapped in byte strings here).
+    fn cose(protected_map: &[u8], unprotected_map: &[u8], payload_map: &[u8]) -> Vec<u8> {
+        let mut out = Vec::new();
+        let mut enc = Encoder::new(&mut out);
+        enc.array(4).unwrap();
+        enc.bytes(protected_map).unwrap();
+        out.extend_from_slice(unprotected_map);
+        let mut enc = Encoder::new(&mut out);
+        enc.bytes(payload_map).unwrap();
+        enc.bytes(&[]).unwrap();
+        out
+    }
+
+    const HS256: &[u8] = &[0xa1, 0x01, 0x05];
+    const EMPTY: &[u8] = &[0xa0];
+
+    /// `{1: <value>}`
+    fn map_with(value: &[u8]) -> Vec<u8> {
+        let mut m = vec![0xa1, 0x01];
+        m.extend_from_slice(value);
+        m
+    }
+
+    /// `n` nested single-element arrays around an empty array (n + 1 arrays)
+    fn nested_arrays(n: usize) -> Vec<u8> {
+        let mut v = vec![0x81; n];
+        v.push(0x80);
+        v
+    }
+
+    /// Each malformed value in each position: protected header, unprotected
+    /// header, payload.
+    fn assert_rejected_everywhere(value: &[u8]) {
+        let inner = map_with(value);
+        for bytes in [
+            cose(&inner, EMPTY, EMPTY),
+            cose(HS256, &inner, EMPTY),
+            cose(HS256, EMPTY, &inner),
+        ] {
+            assert!(matches!(
+                Token::from_bytes(&bytes),
+                Err(Error::InvalidFormat(_) | Error::CborDecode(_))
+            ));
+        }
+    }
+
+    #[test]
+    fn test_huge_declared_array_length_is_rejected_without_allocating() {
+        // array(2^64 - 1): previously a capacity-overflow panic
+        assert_rejected_everywhere(&[0x9b, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff]);
+        // array(2^32) with a few bytes following
+        assert_rejected_everywhere(&[0x9b, 0, 0, 0, 1, 0, 0, 0, 0, 0x01, 0x02]);
+    }
+
+    #[test]
+    fn test_huge_declared_map_length_is_rejected() {
+        assert_rejected_everywhere(&[0xbb, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff]);
+        // map(3) with room for only one entry
+        assert_rejected_everywhere(&[0xa3, 0x01, 0x02]);
+    }
+
+    #[test]
+    fn test_deep_nesting_is_rejected_without_overflowing_the_stack() {
+        // Previously a stack overflow that aborted the process
+        assert_rejected_everywhere(&nested_arrays(200_000));
+        // Alternating maps and arrays
+        let mut v = Vec::new();
+        for _ in 0..100_000 {
+            v.extend_from_slice(&[0xa1, 0x01, 0x81]);
+        }
+        v.push(0x80);
+        assert_rejected_everywhere(&v);
+    }
+
+    #[test]
+    fn test_nesting_depth_boundary() {
+        // The header map is depth 0 and `nested_arrays(n)` is n + 1 arrays, so
+        // nested_arrays(30) reaches depth 31 (allowed) and 31 reaches depth 32
+        let ok = cose(HS256, &map_with(&nested_arrays(30)), EMPTY);
+        assert!(Token::from_bytes(&ok).is_ok());
+        let too_deep = cose(HS256, &map_with(&nested_arrays(31)), EMPTY);
+        assert!(matches!(
+            Token::from_bytes(&too_deep),
+            Err(Error::InvalidFormat(_))
+        ));
+    }
+
+    #[test]
+    fn test_indefinite_length_containers_are_rejected() {
+        // [_ 1, 2] and {_ 1: 2}
+        assert_rejected_everywhere(&[0x9f, 0x01, 0x02, 0xff]);
+        assert_rejected_everywhere(&[0xbf, 0x01, 0x02, 0xff]);
+    }
+
+    #[test]
+    fn test_truncated_strings_are_rejected() {
+        // bstr / tstr declaring 2^64 - 1 bytes
+        assert_rejected_everywhere(&[0x5b, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff]);
+        assert_rejected_everywhere(&[0x7b, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff]);
+    }
+
+    #[test]
+    fn test_nested_claims_still_decode() {
+        // A realistic catu-style nested claim well within the limit
+        let token = TokenBuilder::new()
+            .algorithm(Algorithm::HmacSha256)
+            .custom_cbor(
+                crate::constants::cat_keys::CATU,
+                CborValue::Map(BTreeMap::from([(
+                    crate::constants::uri_components::PATH,
+                    CborValue::Map(BTreeMap::from([(
+                        crate::constants::match_types::PREFIX,
+                        CborValue::Text("/videos/".into()),
+                    )])),
+                )])),
+            )
+            .sign(b"key")
+            .unwrap();
+        assert!(Token::from_bytes(&token.to_bytes().unwrap()).is_ok());
+    }
+}

tests/decode_allocation.rs

@@ -0,0 +1,67 @@
+//! Decoding must not reserve memory far beyond the size of its input.
+//!
+//! This is an integration test so its counting global allocator only applies
+//! to this test binary.
+
+use common_access_token::Token;
+use std::alloc::{GlobalAlloc, Layout, System};
+use std::sync::atomic::{AtomicUsize, Ordering};
+
+struct Counting;
+
+static CURRENT: AtomicUsize = AtomicUsize::new(0);
+static PEAK: AtomicUsize = AtomicUsize::new(0);
+
+unsafe impl GlobalAlloc for Counting {
+    unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
+        let ptr = System.alloc(layout);
+        if !ptr.is_null() {
+            let now = CURRENT.fetch_add(layout.size(), Ordering::SeqCst) + layout.size();
+            PEAK.fetch_max(now, Ordering::SeqCst);
+        }
+        ptr
+    }
+
+    unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
+        System.dealloc(ptr, layout);
+        CURRENT.fetch_sub(layout.size(), Ordering::SeqCst);
+    }
+}
+
+#[global_allocator]
+static ALLOCATOR: Counting = Counting;
+
+/// A COSE structure whose unprotected header is `{1: X}`, where X is a chain
+/// of 31 nested arrays (the maximum depth). Each array declares as many items
+/// as the remaining input could hold, with the next array as its first item,
+/// followed by `pad` one-byte items.
+fn nested_array_chain(pad: u32) -> Vec<u8> {
+    let mut bytes = vec![0x84, 0x43, 0xa1, 0x01, 0x05, 0xa1, 0x01];
+    for level in 0..31u32 {
+        bytes.push(0x9a);
+        bytes.extend_from_slice(&(pad + (30 - level) * 5).to_be_bytes());
+    }
+    bytes.resize(bytes.len() + pad as usize, 0x00);
+    bytes
+}
+
+#[test]
+fn test_nested_declared_lengths_do_not_multiply_allocation() {
+    let input = nested_array_chain(100_000);
+
+    let baseline = CURRENT.load(Ordering::SeqCst);
+    PEAK.store(baseline, Ordering::SeqCst);
+    let result = Token::from_bytes(&input);
+    let peak = PEAK.load(Ordering::SeqCst) - baseline;
+
+    assert!(result.is_err());
+    // Decoding the innermost array legitimately holds one value per input
+    // byte (tens of bytes each); preallocating every level's declared length
+    // reached roughly 1000x the input.
+    assert!(
+        peak < 64 * input.len(),
+        "decoding a {}-byte input reserved {peak} bytes ({}x)",
+        input.len(),
+        peak / input.len()
+    );
+}