Skip to content

Conversation

abhip2565
Copy link

@abhip2565 abhip2565 commented Jul 16, 2025

Motivation

This PR prevents potential crashes and unsafe decoding results when encountering invalid or malicious CBOR inputs. Specifically:

  • Arrays and maps that declare absurdly large lengths (e.g., uint64.max) would previously cause memory allocation crashes.
  • Some invalid or truncated CBOR inputs could return nil silently, instead of throwing a meaningful error.

These issues can lead to application crashes or unexpected behavior in production.

Changes

  • Adds an application-safe maximum length check in readLength(), defaulting to 200,000 elements.
    • Throws CBORError.tooLongSequence if the declared array or map length exceeds this limit.
  • Tightens the default case in decodeItem() to always throw CBORError.unfinishedSequence for unknown or malformed input.
    • Prevents any nil return from decoding, ensuring consistent error behavior on corrupt data.
  • Ensures the decoder fails early and safely on corrupted, truncated, or malicious CBOR input.

Rationale

  • Enforcing a maximum size limit is a best practice for parsers to avoid denial-of-service vulnerabilities from maliciously large CBOR structures.
  • Throwing explicit errors for unrecognized CBOR types or invalid input improves safety and developer ergonomics by making failures predictable and testable.

Testing

  • Added new unit tests in CBORDecoderTests that verify decoding fails with CBORError.tooLongSequence when encountering absurd array or map sizes.
  • Added unit tests to ensure decoding empty input or truncated/malformed headers throws CBORError.unfinishedSequence.
  • All existing unit tests continue to pass.(that were passing)

Notes

  • The maximum length limit (200,000) is intentionally conservative and can be adjusted by library users or maintainers as needed.
  • These changes are fully backward-compatible for all valid CBOR inputs within reasonable sizes.

Linked Issues

Signed-off-by: Abhishek Paul <[email protected]>
@abhip2565
Copy link
Author

@hamchapman please look into this PR, to unblock crashes in ios apps consuming this library.

@abhip2565
Copy link
Author

@hamchapman, @valpackett Any update on this? Would be really helpful if any one of you can look into this.Thanks

@valpackett
Copy link
Owner

defaulting to 200,000

It's not "defaulting", currently it's just entirely hardcoded, which I'm rather iffy about. What if I do want to handle 10 million entries?

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