Skip to content

Bound CBOR container lengths and nesting depth when decoding - #15

Open
zshenker wants to merge 1 commit into
mainfrom
cbor-decode-limits
Open

zshenker wants to merge 1 commit into
mainfrom
cbor-decode-limits

Conversation

@zshenker

Copy link
Copy Markdown
Collaborator

Problem

Token::from_bytes decodes untrusted input, but its recursive CBOR map/array decoder trusted two things from that input:

  • Allocation from a declared length. decode_array pre-allocated Vec::with_capacity(declared_len) straight from the array header, so a declared length near 2^64 panicked ("capacity overflow") before any element was read.
  • Unbounded recursion. Decoding recursed once per nesting level with no bound, so deeply nested arrays or maps overflowed the stack and aborted the process.

Indefinite-length arrays and maps were also read as empty, which misparsed whatever followed.

Change

  • A declared array or map length that the remaining input can't hold is rejected before allocating or iterating. Each item needs at least one byte, and each map entry two.
  • Container nesting is capped at 32 levels (MAX_NESTING_DEPTH). CWT, COSE and CAT structures nest only a few.
  • Indefinite-length arrays and maps are rejected explicitly.
  • All three return Error::InvalidFormat. The limits apply to the protected header, unprotected header and payload, and to every nested claim value. Valid tokens are unaffected, and the public API is unchanged.
  • A cargo-fuzz target (fuzz/) exercises Token::from_bytes, to_bytes and verify_claims on arbitrary input. It is excluded from the published crate.

Tests

  • Seven regression tests run each case in each position (protected header, unprotected header, payload): a 2^64-1 array, an oversized map, nesting 200,000 deep, the depth boundary (31 levels pass, 32 fail), indefinite-length containers, oversized strings, and a nested catu claim that must still decode.
  • cargo clippy --all-targets -- -D warnings and cargo test pass, with 62 unit tests and 73 doctests.
  • A 60-second fuzz run of about 2M inputs found no crashes.

🤖 Generated with Claude Code

Token::from_bytes decodes attacker-supplied bytes, but the recursive
map/array decoder trusted the input in two ways:

- decode_array pre-allocated Vec::with_capacity(declared_len) straight
  from the array header, so a declared length near 2^64 panicked with a
  capacity overflow before any element was read.
- Decoding recursed once per nesting level with no bound, so a header
  or payload nesting arrays/maps a few hundred thousand levels deep
  overflowed the stack and aborted the process.

Reject a declared array or map length that the remaining input cannot
hold (each array item needs at least one byte, each map entry two)
before allocating or iterating, and cap container nesting at 32 levels
(CWT/COSE/CAT structures nest only a few). Both return
Error::InvalidFormat. Indefinite-length arrays and maps, which were
misread as empty, are now rejected explicitly. The limits apply to the
protected header, unprotected header and payload alike, and to every
nested claim value.

Add regression tests for each case in each position, and a cargo-fuzz
target (fuzz/) for Token::from_bytes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/token.rs
}
let array_len = dec.array()?;
let array_len = container_len(dec, array_len, 1, "array")?;
let mut array = Vec::with_capacity(array_len);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that assigning capacity like this can end up eating quite a bit of memory in the worst MAX_NESTING_DEPTH nested array case - is it worth considering Vec::new() instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants