From 4b7ae620272242feb9f5cc1c8c13fa8b93bdaa7e 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/Makefile.test.include | 1 + src/serialize.h | 14 +++- src/test/serialize_bitset_tests.cpp | 103 ++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 src/test/serialize_bitset_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index faaee5aa1913..6b725739ecfe 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -181,6 +181,7 @@ BITCOIN_TESTS =\ test/script_tests.cpp \ test/scriptnum_tests.cpp \ test/serfloat_tests.cpp \ + test/serialize_bitset_tests.cpp \ test/serialize_tests.cpp \ test/settings_tests.cpp \ test/sighash_tests.cpp \ 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_bitset_tests.cpp b/src/test/serialize_bitset_tests.cpp new file mode 100644 index 000000000000..ae2c0fd8b401 --- /dev/null +++ b/src/test/serialize_bitset_tests.cpp @@ -0,0 +1,103 @@ +// Copyright (c) 2026 The Dash Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include + +BOOST_FIXTURE_TEST_SUITE(serialize_bitset_tests, BasicTestingSetup) + +namespace { +//! The bound must reject before allocating, so a short read after the fact is not good enough. +bool RejectedBeforeAllocating(const std::string& what) +{ + return what.find("exceeds remaining") != std::string::npos || + what.find("ReadFixedBitSet") != std::string::npos; +} +} // namespace + +/** + * 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"); + BOOST_CHECK_NE(bits.size(), kClaimedBits); + BOOST_CHECK_MESSAGE(RejectedBeforeAllocating(what), "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_NE(qc.signers.size(), 1'000'000u); + BOOST_CHECK_MESSAGE(RejectedBeforeAllocating(what), + "Expected a pre-allocation rejection for CFinalCommitment, got: " + what); +} + +BOOST_AUTO_TEST_SUITE_END() From 8825c6bdffb8fc22ecee8e6c85f173bae2f32877 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 ++++++++++++----- 1 file changed, 12 insertions(+), 5 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);