[kakkar87d]

32-bit integer overflow in the AMQP consumer length checks

pni_consumer_readv32 and the 4-octet variable-width case in pni_consumer_read_value_not_described validate a wire-supplied 32-bit length with position + size > size. On an ILP32 target (size_t is 32 bits) that addition wraps, so a length close to UINT32_MAX passes the check and the consumer returns a pn_bytes_t that points at a few bytes but claims up to 4 GiB, which the caller then reads past the buffer. These consumers parse SASL mechanism/init frames and transport performatives, so the length is attacker controlled before authentication.

Rewritten to the non-overflowing form already used by pn_decoder_remaining, comparing the length against the bytes actually remaining (consumer->position <= consumer->size always holds at these points). The nearby uint8 and fixed-width checks cannot overflow for any real buffer and are left unchanged.

Verified on a 32-bit armhf build: a crafted length of 0xFFFFFFFF against a 16-byte buffer was accepted before the change and rejected after. The full build, c-core-test, the codec/message/value/interop tests and the fuzz-connection-driver/fuzz-message-decode regression corpora all pass.

[astitcher]

@kakkar87d Thank you so much for working on Qpid Proton - a few comments on this PR:

1. This is a functional change so requires a JIRA issue.
2. You have provided no way to verify your claim - no test that fails before the change and succeeds afterwards and no "crafted" sequence that would otherwise be accepted but cause a failure.

[kakkar87d]

Fair on both. I'll raise a JIRA for it and retitle the commits once I have the number.

I've pushed a test, driver_open_oversized_str32 in connection_driver_test.cpp. It feeds a server driver an open frame whose container-id is a str32 claiming 0xFFFFFFFF bytes inside a 10 byte list, then checks the container is rejected. The wrap only happens with a 32-bit size_t, so it only fails before the change on a 32-bit build. On linux/arm/v7 (bookworm, gcc 12) without the fix it fails with the oversized string accepted as the remote container:

CHECK( pn_connection_remote_container(d.connection) == __null )
with expansion:
  "AAAA" == 0

With the fix it passes, and so does the rest of c-core-test. On 64-bit it passes either way, since position+size can't wrap there.