Skip to content

reject CBOR array/map length equal to the indefinite-length marker#5274

Open
Angadi56 wants to merge 2 commits into
nlohmann:developfrom
Angadi56:cbor-length-sentinel
Open

reject CBOR array/map length equal to the indefinite-length marker#5274
Angadi56 wants to merge 2 commits into
nlohmann:developfrom
Angadi56:cbor-length-sentinel

Conversation

@Angadi56

Copy link
Copy Markdown
Contributor

CBOR arrays and maps can prefix their element count with a full eight-byte integer, and the reader tells a definite-length container from an indefinite one by comparing that count against detail::unknown_size(), which is SIZE_MAX. Nothing stops a definite count from being exactly SIZE_MAX, so on a 64-bit build a stream that declares 0xFFFFFFFFFFFFFFFF elements narrows straight onto the sentinel and get_cbor_array/get_cbor_object read the container as indefinite rather than rejecting it. The effect is that bytes like 9B FF FF FF FF FF FF FF FF 01 02 FF come back as [1, 2], read up to the next 0xFF break, even though the neighboring count 0xFFFFFFFFFFFFFFFE is correctly turned away as excessive. I noticed the gap while checking how the eight-byte length path lines up with the indefinite marker. The fix adds a small get_cbor_container_size helper that narrows the length and rejects the one value that aliases the sentinel; such a count is larger than any container's max_size() anyway, so no representable input changes behavior. The four-byte and eight-byte array and map cases route through it, and the indefinite forms stay as they were.

  • The changes are described in detail, both the what and why.
  • If applicable, an existing issue is referenced.
  • The Code coverage remained at 100%. A test case for every new line of code.
  • If applicable, the documentation is updated.
  • The source code is amalgamated by running make amalgamate.

Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>

Copy link
Copy Markdown
Owner

Nice, narrowly-scoped fix — the mechanics check out (conditional_static_cast is a bare static_cast, unknown_size() is SIZE_MAX, so the aliasing is real, and the test's expected message matches exception_message()'s output exactly). Two non-blocking suggestions:

Consider reusing value_in_range_of<std::size_t> instead of the equality check. The file already has an established, more general solution for this exact class of problem: the BJData 'M' case (binary_reader.hpp around line 2155) narrows a uint64_t length to std::size_t and guards it with value_in_range_of<std::size_t>(number) before casting, rejecting any value that doesn't fit — not just the one that happens to alias the sentinel.

get_cbor_container_size only catches the exact SIZE_MAX-aliasing value. On 32-bit builds, a CBOR 8-byte length like 0x100000005 truncates to 5 via conditional_static_cast, silently passes this check (since 5 != unknown_size()), and the parser proceeds with a wrong length instead of rejecting an unrepresentable input. That's a pre-existing gap this PR doesn't introduce, but since the PR is already reworking this exact narrowing step, it'd be low-cost to close it completely by swapping in value_in_range_of<std::size_t>(len) — more correct, and consistent with the codebase's own precedent for this narrowing pattern.

Minor: exception_message context string. Other length/count-related 408 errors in this file consistently pass "size" as the context argument (e.g. the BJData overflow cases, and the ndarray "excessive ndarray size caused overflow" case, which uses the identical result == npos/unknown_size() idiom this PR introduces for CBOR). This PR's get_cbor_container_size passes "value" instead, which elsewhere in the file is reserved for generic value-parsing errors (EOF, invalid byte, negative-integer overflow), not size/length errors. Using "size" here would be more consistent — cosmetic, but it affects the exact wording of the user-facing exception string, which the new test currently hardcodes.

Neither point blocks this fix; both would just make it more thorough/consistent with existing conventions in the same file.


Generated by Claude Code

Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>
@Angadi56

Copy link
Copy Markdown
Contributor Author

Both done. I swapped in value_in_range_of<std::size_t> and kept the sentinel equality check next to it, since the range check alone doesn't cover either width: on 64-bit SIZE_MAX fits in size_t, and on 32-bit a four-byte length of 0xFFFFFFFF is in range but still aliases unknown_size(). Both paths now raise the same 408.

The context string is now "size" and the expected messages in the tests are updated to match. Since the truncation half only manifests on 32-bit builds, I put the 0x100000005 case (plus the four-byte sentinel case) in unit-32bit.cpp next to the existing BJData ones. CBOR suite is green and the amalgamation is regenerated.

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann added this to the Release 3.13.0 milestone Jul 18, 2026
@nlohmann nlohmann added the 🚀 ready to merge Ready to merge - just waiting for CI to complete. label Jul 18, 2026
@nlohmann

Copy link
Copy Markdown
Owner

Thanks a lot!

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

Labels

L 🚀 ready to merge Ready to merge - just waiting for CI to complete. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants