From 9f026a92b7a8e8dae0fedbadc4204d45ef031c24 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 3 Aug 2026 09:43:09 -0500 Subject: [PATCH 1/2] fix: bound DYNBITSET allocation against remaining stream size ReadFixedBitSet allocated from a wire-declared CompactSize with no bound beyond ReadCompactSize's 33,554,432 cap. Roughly five bytes on the wire (a CompactSize claiming millions of bits and no payload) therefore forced a std::vector resize plus a byte buffer totalling several MiB, all of which was only abandoned when the subsequent short read threw. MAX_PROTOCOL_MESSAGE_LENGTH does not help, because the attack uses an undersized message. Bound the declared length against the bytes actually remaining in the stream before allocating. A well-formed message always carries exactly the required bytes, so this rejects only claims that could never have been satisfied. This covers every DYNBITSET caller, including CFinalCommitment::signers and validMembers, which are reachable from an unauthenticated QFCOMMITMENT. --- src/serialize.h | 14 +++++- src/test/serialize_tests.cpp | 88 ++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/src/serialize.h b/src/serialize.h index cd644bd79822..3e00306d1014 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -438,9 +438,21 @@ void WriteFixedBitSet(Stream& s, const std::vector& vec, size_t size) template void ReadFixedBitSet(Stream& s, std::vector& vec, size_t size) { + const size_t nbytes = (size + 7) / 8; + // Bound the wire-declared length against the bytes actually left in the stream before + // allocating anything. Otherwise a handful of bytes declaring millions of bits forces a + // multi-megabyte resize and zero-fill that is only abandoned when the short read throws. + // A well-formed message always carries exactly the required bytes, so this rejects only + // claims that could never have been satisfied. + if constexpr (requires(const Stream& cs) { cs.size(); }) { + if (nbytes > s.size()) { + throw std::ios_base::failure("ReadFixedBitSet(): declared size exceeds remaining bytes"); + } + } + vec.resize(size); - std::vector vBytes((size + 7) / 8); + std::vector vBytes(nbytes); s.read(AsWritableBytes(Span{vBytes})); for (size_t p = 0; p < size; p++) vec[p] = (vBytes[p / 8] & (1 << (p % 8))) != 0; diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp index a51b98a1ba80..1cfc74f3ab69 100644 --- a/src/test/serialize_tests.cpp +++ b/src/test/serialize_tests.cpp @@ -3,15 +3,20 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include +#include #include #include #include +#include #include #include +#include #include #include +#include #include #include @@ -183,6 +188,89 @@ BOOST_AUTO_TEST_CASE(vector_bool) BOOST_CHECK(SerializeHash(vec1) == SerializeHash(vec2)); } + +//! The message the bound throws. Matching it exactly keeps these tests from passing on an +//! unrelated short read, which is the very failure mode the bound replaces. +static constexpr std::string_view BOUND_REJECTION{"declared size exceeds remaining bytes"}; + +/** + * DYNBITSET must not allocate from an attacker-declared CompactSize when the remaining stream + * is far too small to hold the claimed bit payload. A handful of bytes claiming ~1e6 bits is + * the amplification primitive: ReadCompactSize permits up to 33,554,432, which would resize a + * std::vector to ~4 MiB and allocate another ~4 MiB byte buffer before the short read + * throws. The claim below is deliberately modest so the pre-fix path also stays safe on CI. + */ +BOOST_AUTO_TEST_CASE(dynbitset_rejects_oversized_declared_length) +{ + constexpr uint64_t kClaimedBits = 1'000'000; + + CDataStream s(SER_NETWORK, PROTOCOL_VERSION); + WriteCompactSize(s, kClaimedBits); + // No bit payload follows, so the remaining size is zero. + + std::vector bits; + std::string what; + bool threw = false; + try { + s >> DYNBITSET(bits); + } catch (const std::ios_base::failure& e) { + threw = true; + what = e.what(); + } + BOOST_CHECK_MESSAGE(threw, "DYNBITSET must reject a declared length that exceeds remaining bytes"); + // Rejection has to precede the resize, so the destination must still hold its exact + // pre-deserialization state. Merely falling short of the declared size would also be + // satisfied by an allocation that happened and was then abandoned. + BOOST_CHECK(bits.empty()); + BOOST_CHECK_MESSAGE(what.find(BOUND_REJECTION) != std::string::npos, + "Expected a pre-allocation rejection, got: " + what); +} + +/** A legitimately sized DYNBITSET (LLMQ max 400) must still round-trip unchanged. */ +BOOST_AUTO_TEST_CASE(dynbitset_accepts_legitimate_llmq_size) +{ + constexpr size_t kSize = Consensus::MAX_LLMQ_SIZE; + std::vector original(kSize, false); + for (size_t i = 0; i < kSize; i += 3) { + original[i] = true; + } + + CDataStream s(SER_NETWORK, PROTOCOL_VERSION); + s << DYNBITSET(original); + + std::vector decoded; + s >> DYNBITSET(decoded); + BOOST_CHECK(decoded == original); +} + +/** + * The same primitive is reachable from an unauthenticated QFCOMMITMENT via CFinalCommitment's + * signers bitset, so cover the real message type too. + */ +BOOST_AUTO_TEST_CASE(qfinalcommitment_rejects_oversized_signers_bitset) +{ + CDataStream s(SER_NETWORK, PROTOCOL_VERSION); + // nVersion (u16) | llmqType (u8) | quorumHash (32) | signers DYNBITSET | ... + s << static_cast(llmq::CFinalCommitment::BASIC_BLS_NON_INDEXED_QUORUM_VERSION); + s << Consensus::LLMQType::LLMQ_400_85; + s << uint256::ONE; + WriteCompactSize(s, 1'000'000); + + llmq::CFinalCommitment qc; + std::string what; + bool threw = false; + try { + s >> qc; + } catch (const std::ios_base::failure& e) { + threw = true; + what = e.what(); + } + BOOST_CHECK(threw); + BOOST_CHECK(qc.signers.empty()); + BOOST_CHECK_MESSAGE(what.find(BOUND_REJECTION) != std::string::npos, + "Expected a pre-allocation rejection for CFinalCommitment, got: " + what); +} + BOOST_AUTO_TEST_CASE(noncanonical) { // Write some non-canonical CompactSize encodings, and From aa1447f7e7b76bde0dac841f920ecc20476a0aea Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 3 Aug 2026 13:36:39 -0500 Subject: [PATCH 2/2] serialize: require SizedStream for ReadFixedBitSet instead of duck-typing it The if constexpr guard failed open silently: a stream without size() compiled the bound away with no diagnostic, leaving the unbounded allocation in place. Constraining the template turns that into a build error, so the invariant is checked on every build rather than by inspection. --- src/serialize.h | 17 ++++++++---- src/test/serialize_tests.cpp | 50 ++++-------------------------------- 2 files changed, 17 insertions(+), 50 deletions(-) diff --git a/src/serialize.h b/src/serialize.h index 3e00306d1014..6f266311fa87 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -435,7 +435,16 @@ void WriteFixedBitSet(Stream& s, const std::vector& vec, size_t size) s.write(AsBytes(Span{vBytes})); } -template +/** A stream that can report how many bytes are still available to read. + * + * size() must mean bytes *remaining*, not the total the stream ever held. ReadFixedBitSet + * relies on that to bound a wire-declared bit count before allocating, so a stream whose + * size() means anything else would silently weaken the bound rather than fail to compile. + */ +template +concept SizedStream = requires(const S& s) { { s.size() } -> std::convertible_to; }; + +template void ReadFixedBitSet(Stream& s, std::vector& vec, size_t size) { const size_t nbytes = (size + 7) / 8; @@ -444,10 +453,8 @@ void ReadFixedBitSet(Stream& s, std::vector& vec, size_t size) // multi-megabyte resize and zero-fill that is only abandoned when the short read throws. // A well-formed message always carries exactly the required bytes, so this rejects only // claims that could never have been satisfied. - if constexpr (requires(const Stream& cs) { cs.size(); }) { - if (nbytes > s.size()) { - throw std::ios_base::failure("ReadFixedBitSet(): declared size exceeds remaining bytes"); - } + if (nbytes > s.size()) { + throw std::ios_base::failure("ReadFixedBitSet(): declared size exceeds remaining bytes"); } vec.resize(size); diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp index 1cfc74f3ab69..0c89b1eddd86 100644 --- a/src/test/serialize_tests.cpp +++ b/src/test/serialize_tests.cpp @@ -3,12 +3,9 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include -#include -#include #include #include #include -#include #include #include @@ -209,27 +206,18 @@ BOOST_AUTO_TEST_CASE(dynbitset_rejects_oversized_declared_length) // No bit payload follows, so the remaining size is zero. std::vector bits; - std::string what; - bool threw = false; - try { - s >> DYNBITSET(bits); - } catch (const std::ios_base::failure& e) { - threw = true; - what = e.what(); - } - BOOST_CHECK_MESSAGE(threw, "DYNBITSET must reject a declared length that exceeds remaining bytes"); + BOOST_CHECK_EXCEPTION(s >> DYNBITSET(bits), std::ios_base::failure, + HasReason(std::string{BOUND_REJECTION})); // Rejection has to precede the resize, so the destination must still hold its exact // pre-deserialization state. Merely falling short of the declared size would also be // satisfied by an allocation that happened and was then abandoned. BOOST_CHECK(bits.empty()); - BOOST_CHECK_MESSAGE(what.find(BOUND_REJECTION) != std::string::npos, - "Expected a pre-allocation rejection, got: " + what); } -/** A legitimately sized DYNBITSET (LLMQ max 400) must still round-trip unchanged. */ -BOOST_AUTO_TEST_CASE(dynbitset_accepts_legitimate_llmq_size) +/** The largest bit count accepted by ReadCompactSize must still round-trip unchanged. */ +BOOST_AUTO_TEST_CASE(dynbitset_accepts_maximum_size) { - constexpr size_t kSize = Consensus::MAX_LLMQ_SIZE; + constexpr size_t kSize = MAX_SIZE; std::vector original(kSize, false); for (size_t i = 0; i < kSize; i += 3) { original[i] = true; @@ -243,34 +231,6 @@ BOOST_AUTO_TEST_CASE(dynbitset_accepts_legitimate_llmq_size) BOOST_CHECK(decoded == original); } -/** - * The same primitive is reachable from an unauthenticated QFCOMMITMENT via CFinalCommitment's - * signers bitset, so cover the real message type too. - */ -BOOST_AUTO_TEST_CASE(qfinalcommitment_rejects_oversized_signers_bitset) -{ - CDataStream s(SER_NETWORK, PROTOCOL_VERSION); - // nVersion (u16) | llmqType (u8) | quorumHash (32) | signers DYNBITSET | ... - s << static_cast(llmq::CFinalCommitment::BASIC_BLS_NON_INDEXED_QUORUM_VERSION); - s << Consensus::LLMQType::LLMQ_400_85; - s << uint256::ONE; - WriteCompactSize(s, 1'000'000); - - llmq::CFinalCommitment qc; - std::string what; - bool threw = false; - try { - s >> qc; - } catch (const std::ios_base::failure& e) { - threw = true; - what = e.what(); - } - BOOST_CHECK(threw); - BOOST_CHECK(qc.signers.empty()); - BOOST_CHECK_MESSAGE(what.find(BOUND_REJECTION) != std::string::npos, - "Expected a pre-allocation rejection for CFinalCommitment, got: " + what); -} - BOOST_AUTO_TEST_CASE(noncanonical) { // Write some non-canonical CompactSize encodings, and