fix(cbor): reject overflowing negative integers

[thevilledev]

Motivation

CBOR encodes negative integers as "-1 - n" where n is an unsigned integer. Per RFC 8949 Section 5.5:

CBOR keeps the sign bit for its integer representation in the major type, it has one bit more for signed numbers of a certain length (e.g., -2^64..2^64-1 for 1+8-byte integers) than the typical platform signed integer representation of the same length (-2^63..2^63-1 for 8-byte int64_t).

and:

a protocol that uses numbers should define its expectations on the handling of nontrivial numbers

When n exceeds the representable range of number_integer_t, the computation causes undefined behaviour and silent data corruption.

For the default int64_t type:

• -9223372036854775809 (n = 0x8000000000000000) was incorrectly parsed as 9223372036854775807

For custom 32-bit configurations (int32_t):

• -2147483649 (n = 0x80000000) would similarly overflow

Changes

Add bounds checks for all CBOR negative integer types (0x38, 0x39, 0x3A, 0x3B) to reject values exceeding number_integer_t range, returning parse_error.112 instead of silently corrupting data.

Add regression tests for:

• Overflow detection at boundary values

• Truncated input handling

• Custom int32_t integer type configurations

• 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.

[github-actions]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @thevilledev
Please read and follow the Contribution Guidelines.

[coveralls]

coverage: 99.192% (+0.001%) from 99.191%
when pulling 482de87 on thevilledev:fix/cbor-int-overflow
into 8f75700 on nlohmann:develop.

[nlohmann]

Looks good to me.

[nlohmann]

It would be great to have a second opinion here. @gregmarr

[gregmarr]

I'm unsure that the tests are testing what they should be. It should be testing getting a result of int64 max, and getting a result of int64 max + 1 failing, but it looks like it's testing putting the bit patterns for those values in the CBOR data and then checking for the results from those bit patterns.

I should be sleeping now, maybe it will make more sense tomorrow.

[thevilledev]

It should be testing getting a result of int64 max, and getting a result of int64 max + 1 failing

So the tests are specifically for CBOR type 0x3B (negative integers). The formula for result is result = -1 - n and it can (or should) only produce negative results. Therefore result of INT64_MAX is not applicable here. The boundaries being tested are:

• n=INT64_MAX => result = INT64_MIN implying the most negative representable value
• n=INT64_MAX + 1 => result = INT64_MIN - 1 overflows & rejected

I ran the tests against develop branch with ./build/tests/test-cbor_cpp11 "Tagged values" and got this:

TEST CASE:  Tagged values
  negative integer overflow
  n = INT64_MAX + 1 causes overflow

/git/json/tests/src/unit-cbor.cpp:2712: ERROR: CHECK_THROWS_WITH_AS( _ = json::from_cbor(input), "[json.exception.parse_error.112] parse error at byte 9: syntax error while parsing CBOR value: negative integer overflow", json::parse_error ) did NOT throw at all!

---

TEST CASE:  Tagged values
  negative integer overflow
  n = UINT64_MAX causes overflow

/git/json/tests/src/unit-cbor.cpp:2722: ERROR: CHECK_THROWS_WITH_AS( _ = json::from_cbor(input), "[json.exception.parse_error.112] parse error at byte 9: syntax error while parsing CBOR value: negative integer overflow", json::parse_error ) did NOT throw at all!

---

TEST CASE:  Tagged values
  negative integer overflow
  overflow with allow_exceptions=false returns discarded

/git/json/tests/src/unit-cbor.cpp:2729: ERROR: CHECK( result.is_discarded() ) is NOT correct!
  values: CHECK( false )

For positive integers (type 0x1B) we use number_unsigned so it doesn't have an equivalent overflow issue.

But of course welcome to other ideas how to test this. I guess we could check that there's no sign flip, i.e. negative results are never positive?

[gregmarr]

Like I said, don't review code when you should be sleeping. :)

I mostly deal with floating point, where "min" refers to the smallest possible positive number, rather than integers, where it refers to the negative number with the largest magnitude, so I often mix up the integer min unless I really stop and think about it, and I was definitely not doing that properly.

I agree that we should be testing -1 which should work as a round trip, 0 which should fail, int64_min which should work as a round trip, and int64_min - 1 which should fail.

[thevilledev]

np, no worries! I clarified the tests a bit, hopefully covering the boundaries fully now. Let me know what you think 👍

[nlohmann]

Quick question: don't we have the same situation with int32 overflow?

#include <nlohmann/json.hpp>
#include <iostream>

using json = nlohmann::json;

int main() {
    const std::vector<uint8_t> input = {0x3A, 0x80, 0x00, 0x00, 0x00};
    auto j = json::from_cbor(input);
    std::cout << j << '\n';
}

Instead of printing -2147483649, I would expect the same overflow exception here.

[thevilledev]

I gotta say I totally missed the json32 type aliasing from the tests. I'll add it to 0x3A. Technically similar boundary checks could be added for 0x38 (1-byte) and 0x39 (2-byte) but sounds overreaching to take int8/int16 into account :-)

Thanks for pointing this out!

[nlohmann]

Come to think of it, I think adding a check also for the other types would be consequent.

[thevilledev]

Sure, I'll add those while at it

[thevilledev]

Hmm, tests failing only on Windows clang-cl-12 (x64) failing due to:

SIGSEGV - Stack overflow

The new boundary checks introduce switch-case-local variables (number, max_val). Since parse_cbor_internal is recursive (for nested arrays/objects), this change increases stack pressure per recursion layer.

Is this test flaky in general? Or should we try breaking the logic down to a helper function? I'm not sure if this would get inlined though:

case 0x38:
    return get_cbor_negative_integer<std::uint8_t>();
case 0x39:
    return get_cbor_negative_integer<std::uint16_t>();
case 0x3A:
    return get_cbor_negative_integer<std::uint32_t>();
case 0x3B:
    return get_cbor_negative_integer<std::uint64_t>();

[nlohmann]

Hmm, tests failing only on Windows clang-cl-12 (x64) failing due to:

SIGSEGV - Stack overflow

Oh no!

The new boundary checks introduce switch-case-local variables (number, max_val). Since parse_cbor_internal is recursive (for nested arrays/objects), this change increases stack pressure per recursion layer.

That's not good...

Is this test flaky in general? Or should we try breaking the logic down to a helper function? I'm not sure if this would get inlined though:

No, the test was not flaky so far.

[thevilledev]

Looks like it's fixed now 👍

[nlohmann]

Looks good to me.

[nlohmann]

Looks like it's fixed now 👍

Thanks! This is much cleaner now!