diff --git a/src/serialize.h b/src/serialize.h index cd644bd79822..6f266311fa87 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -435,12 +435,31 @@ 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; + // 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 (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