diff --git a/src/Makefile.test.include b/src/Makefile.test.include index faaee5aa1913..6d42c885e04a 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -137,6 +137,7 @@ BITCOIN_TESTS =\ test/limitedmap_tests.cpp \ test/llmq_blockprocessor_tests.cpp \ test/llmq_dkg_tests.cpp \ + test/llmq_dkg_intake_tests.cpp \ test/llmq_chainlock_tests.cpp \ test/llmq_commitment_tests.cpp \ test/llmq_hash_tests.cpp \ diff --git a/src/llmq/commitment.cpp b/src/llmq/commitment.cpp index 596c9f8599c8..d22a274af1b9 100644 --- a/src/llmq/commitment.cpp +++ b/src/llmq/commitment.cpp @@ -235,7 +235,7 @@ bool CheckLLMQCommitment(const llmq::UtilParameters& util_params, const CTransac } if (LogAcceptDebug(BCLog::LLMQ)) { - // Clamp to validMembers.size() because the wire-format DYNBITSET may be smaller than + // Clamp to validMembers.size() because the wire-format bitset may be smaller than // llmq_params.size for malformed payloads; VerifySizes() below catches the mismatch. std::stringstream ss; const auto log_size = std::min(llmq_params_opt->size, qcTx.commitment.validMembers.size()); @@ -296,7 +296,7 @@ uint256 BuildCommitmentHash(Consensus::LLMQType llmqType, const uint256& blockHa CHashWriter hw(SER_GETHASH, 0); hw << llmqType; hw << blockHash; - hw << DYNBITSET(validMembers); + hw << LIMITED_BITSET(validMembers, Consensus::MAX_LLMQ_SIZE); hw << pubKey; hw << vvecHash; return hw.GetHash(); diff --git a/src/llmq/commitment.h b/src/llmq/commitment.h index 292db406c71f..0bf0d92755ba 100644 --- a/src/llmq/commitment.h +++ b/src/llmq/commitment.h @@ -105,8 +105,8 @@ class CFinalCommitment ); } READWRITE( - DYNBITSET(obj.signers), - DYNBITSET(obj.validMembers), + LIMITED_BITSET(obj.signers, Consensus::MAX_LLMQ_SIZE), + LIMITED_BITSET(obj.validMembers, Consensus::MAX_LLMQ_SIZE), CBLSPublicKeyVersionWrapper(const_cast(obj.quorumPublicKey), (obj.nVersion == LEGACY_BLS_NON_INDEXED_QUORUM_VERSION || obj.nVersion == LEGACY_BLS_INDEXED_QUORUM_VERSION)), obj.quorumVvecHash, CBLSSignatureVersionWrapper(const_cast(obj.quorumSig), (obj.nVersion == LEGACY_BLS_NON_INDEXED_QUORUM_VERSION || obj.nVersion == LEGACY_BLS_INDEXED_QUORUM_VERSION)), diff --git a/src/llmq/dkgmessages.h b/src/llmq/dkgmessages.h index 177b68e5d4d4..090ed4cff2f3 100644 --- a/src/llmq/dkgmessages.h +++ b/src/llmq/dkgmessages.h @@ -6,6 +6,7 @@ #define BITCOIN_LLMQ_DKGMESSAGES_H #include +#include #include #include @@ -52,7 +53,7 @@ class CDKGContribution s >> llmqType; s >> quorumHash; s >> proTxHash; - s >> tmp1; + s >> LIMITED_VECTOR(tmp1, Consensus::MAX_LLMQ_SIZE); s >> tmp2; s >> sig; @@ -90,8 +91,8 @@ class CDKGComplaint obj.llmqType, obj.quorumHash, obj.proTxHash, - DYNBITSET(obj.badMembers), - DYNBITSET(obj.complainForMembers), + LIMITED_BITSET(obj.badMembers, Consensus::MAX_LLMQ_SIZE), + LIMITED_BITSET(obj.complainForMembers, Consensus::MAX_LLMQ_SIZE), obj.sig ); } @@ -124,7 +125,8 @@ class CDKGJustification public: SERIALIZE_METHODS(CDKGJustification, obj) { - READWRITE(obj.llmqType, obj.quorumHash, obj.proTxHash, obj.contributions, obj.sig); + READWRITE(obj.llmqType, obj.quorumHash, obj.proTxHash, + LIMITED_VECTOR(obj.contributions, Consensus::MAX_LLMQ_SIZE), obj.sig); } [[nodiscard]] uint256 GetSignHash() const @@ -170,7 +172,7 @@ class CDKGPrematureCommitment obj.llmqType, obj.quorumHash, obj.proTxHash, - DYNBITSET(obj.validMembers), + LIMITED_BITSET(obj.validMembers, Consensus::MAX_LLMQ_SIZE), obj.quorumPublicKey, obj.quorumVvecHash, obj.quorumSig, diff --git a/src/llmq/net_dkg.cpp b/src/llmq/net_dkg.cpp index e533aadf8777..c212120a6ab8 100644 --- a/src/llmq/net_dkg.cpp +++ b/src/llmq/net_dkg.cpp @@ -72,48 +72,6 @@ size_t MaxDKGMessageSize(std::string_view msg_type, const Consensus::LLMQParams& return cap < HARD_CEILING ? cap : HARD_CEILING; } -// Cheap, param-only structural validation of a pushed DKG message, run at intake -// before retention. Deserializes a COPY of the payload (leaving the caller's bytes -// intact for the pending queue and its inventory hash) and checks only safe upper -// bounds derived from quorum params: no member-list lookup and no signature -// verification, which remain on the DKG worker thread. Deserializing the copy does -// decompress the BLS points carried in the payload, but that work is bounded by -// the size cap applied just before this check. Rejects malformed or clearly -// oversized payloads before retention. -bool CheckDKGMessageStructure(std::string_view msg_type, const CDataStream& vRecv, const Consensus::LLMQParams& params) -{ - const size_t size = params.size > 0 ? static_cast(params.size) : 0; - const size_t min_size = params.minSize > 0 ? static_cast(params.minSize) : 0; - const size_t threshold = params.threshold > 0 ? static_cast(params.threshold) : 0; - try { - CDataStream s(vRecv); // copy; deserialization does not advance the caller's stream - if (msg_type == NetMsgType::QCONTRIB) { - CDKGContribution qc; - s >> qc; - return qc.vvec != nullptr && qc.vvec->size() == threshold && - qc.contributions != nullptr && - qc.contributions->blobs.size() >= min_size && - qc.contributions->blobs.size() <= size; - } else if (msg_type == NetMsgType::QCOMPLAINT) { - CDKGComplaint qc; - s >> qc; - return qc.badMembers.size() == qc.complainForMembers.size() && - qc.badMembers.size() <= size; - } else if (msg_type == NetMsgType::QJUSTIFICATION) { - CDKGJustification qj; - s >> qj; - return qj.contributions.size() <= size; - } else if (msg_type == NetMsgType::QPCOMMITMENT) { - CDKGPrematureCommitment qc; - s >> qc; - return qc.validMembers.size() <= size; - } - return false; - } catch (const std::exception&) { - return false; - } -} - // returns a set of NodeIds which sent invalid messages template std::unordered_set BatchVerifyMessageSigs(CDKGSession& session, @@ -459,13 +417,6 @@ void NetDKG::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStre return; } - // Cheap structural pre-validation before retention. Validates a copy so the - // original bytes (and their inventory hash) are preserved for the worker. - if (!CheckDKGMessageStructure(msg_type, vRecv, llmq_params)) { - m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100, "malformed DKG message"); - return; - } - int inv_type = 0; if (msg_type == NetMsgType::QCONTRIB) inv_type = MSG_QUORUM_CONTRIB; @@ -492,7 +443,7 @@ void NetDKG::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStre // // This check runs last so that every pre-existing rejection above -- and the heavier penalty it // carries -- is unchanged. It therefore bounds retention and signature verification, not the - // parsing and structural validation above, which an unsolicited sender still gets to trigger. + // header validation above, which an unsolicited sender still gets to trigger. const CInv inv{static_cast(inv_type), hash}; if (WITH_LOCK(::cs_main, return m_peer_manager->PeerConsumeGetDataResponse(from, inv)) == GetDataResponse::UNREQUESTED) { diff --git a/src/serialize.h b/src/serialize.h index cd644bd79822..cf2fda2f6672 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -585,7 +585,7 @@ class Wrapper template static inline Wrapper Using(T&& t) { return Wrapper(t); } -#define DYNBITSET(obj) Using(obj) +#define LIMITED_BITSET(obj,n) Using>(obj) #define AUTOBITSET(obj) Using(obj) #define VARINT_MODE(obj, mode) Using>(obj) #define VARINT(obj) Using>(obj) @@ -593,8 +593,16 @@ static inline Wrapper Using(T&& t) { return Wrapper>(obj) #define LIMITED_VECTOR(obj,n) Using>(obj) -/** TODO: describe DynamicBitSet */ -struct DynamicBitSetFormatter +/** + * Stores a bitset whose length is written on the wire as a CompactSize, followed by the packed bits. + * + * The declared length is bounded by Limit before the bitset is allocated. Without that bound a + * five-byte CompactSize can declare MAX_SIZE bits and make ReadFixedBitSet allocate megabytes for a + * payload that never arrives, so callers must pass the largest length the field can legitimately + * carry. + */ +template +struct LimitedBitSetFormatter { template void Ser(Stream& s, const std::vector& vec) const @@ -606,7 +614,11 @@ struct DynamicBitSetFormatter template void Unser(Stream& s, std::vector& vec) { - ReadFixedBitSet(s, vec, ReadCompactSize(s)); + const size_t size = ReadCompactSize(s); + if (size > Limit) { + throw std::ios_base::failure("Bitset length limit exceeded"); + } + ReadFixedBitSet(s, vec, size); } }; diff --git a/src/test/llmq_commitment_tests.cpp b/src/test/llmq_commitment_tests.cpp index d9bebee9cb87..f6a44482f8cc 100644 --- a/src/test/llmq_commitment_tests.cpp +++ b/src/test/llmq_commitment_tests.cpp @@ -102,7 +102,7 @@ BOOST_FIXTURE_TEST_CASE(commitment_check_undersized_bitset_debug_log_test, RegTe { // Catches the OOB-read regression in CheckLLMQCommitment's debug-log loop // by capturing log output rather than relying on undefined behaviour to - // trip a sanitizer. The wire-format validMembers DYNBITSET can deserialize + // trip a sanitizer. The wire-format validMembers bitset can deserialize // smaller than llmq_params.size; before the clamp the loop iterated up to // llmq_params.size and emitted v[0], v[1], ... reading past the bitset. // With the clamp an empty bitset must produce "validMembers[]". diff --git a/src/test/llmq_dkg_intake_tests.cpp b/src/test/llmq_dkg_intake_tests.cpp new file mode 100644 index 000000000000..e83abaf4366d --- /dev/null +++ b/src/test/llmq_dkg_intake_tests.cpp @@ -0,0 +1,239 @@ +// 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 + +#include +#include + +using namespace llmq; +using namespace llmq::testutils; + +BOOST_FIXTURE_TEST_SUITE(llmq_dkg_intake_tests, BasicTestingSetup) + +// A short read throws ios_base::failure too, so asserting the exception type alone would pass even +// with the limits removed. These match the limit's own message instead. +static auto BitsetLimitExceeded() +{ + return [](const std::ios_base::failure& e) { + return std::string_view{e.what()}.find("Bitset length limit exceeded") != std::string_view::npos; + }; +} + +static auto VectorLimitExceeded() +{ + return [](const std::ios_base::failure& e) { + return std::string_view{e.what()}.find("Vector length limit exceeded") != std::string_view::npos; + }; +} + +// A bitset's length is declared on the wire as a CompactSize, so five bytes can claim MAX_SIZE +// (33.5M) bits. Before the length was bounded, ReadFixedBitSet resized the bit vector and allocated +// a packed byte buffer for the full declared count -- roughly 8 MiB from a ~70-byte payload -- and +// only then threw on the short read. The limit must be applied before any allocation. +BOOST_AUTO_TEST_CASE(bitset_length_bounded_before_allocation) +{ + CDataStream s(SER_NETWORK, PROTOCOL_VERSION); + WriteCompactSize(s, MAX_SIZE); + + std::vector bits; + BOOST_CHECK_EXCEPTION(LimitedBitSetFormatter().Unser(s, bits), + std::ios_base::failure, BitsetLimitExceeded()); + BOOST_CHECK(bits.empty()); +} + +// One bit past the limit is rejected; exactly the limit is accepted. A quorum can never exceed +// MAX_LLMQ_SIZE members (chainparams rejects such a params set at startup), so the boundary is the +// largest bitset any honest LLMQ message can carry. +BOOST_AUTO_TEST_CASE(bitset_limit_boundary) +{ + const auto roundtrip = [](size_t bit_count) { + CDataStream s(SER_NETWORK, PROTOCOL_VERSION); + LimitedBitSetFormatter().Ser(s, std::vector(bit_count, true)); + std::vector out; + LimitedBitSetFormatter().Unser(s, out); + return out; + }; + + const std::vector at_limit = roundtrip(Consensus::MAX_LLMQ_SIZE); + BOOST_CHECK_EQUAL(at_limit.size(), size_t(Consensus::MAX_LLMQ_SIZE)); + BOOST_CHECK(std::all_of(at_limit.begin(), at_limit.end(), [](bool b) { return b; })); + + BOOST_CHECK_THROW(roundtrip(Consensus::MAX_LLMQ_SIZE + 1), std::ios_base::failure); +} + +// The wire format must be unchanged by the bound: the limit is a deserialization-side check only, +// so a bitset still round-trips bit-for-bit, including the partial trailing byte. +BOOST_AUTO_TEST_CASE(bitset_roundtrip_preserves_bits) +{ + std::vector src(77); + src[0] = src[13] = src[76] = true; + + CDataStream s(SER_NETWORK, PROTOCOL_VERSION); + LimitedBitSetFormatter().Ser(s, src); + // CompactSize(77) + ceil(77/8) packed bytes. + BOOST_CHECK_EQUAL(s.size(), size_t{1} + 10); + + std::vector dst; + LimitedBitSetFormatter().Unser(s, dst); + BOOST_CHECK(src == dst); +} + +// The same bound has to hold through the real message serializers, which is where an attacker +// actually reaches it: QCOMPLAINT and QPCOMMITMENT carry member bitsets, and CFinalCommitment +// carries signers/validMembers reachable from any peer via QFCOMMITMENT. +BOOST_AUTO_TEST_CASE(oversized_bitset_rejected_through_message_serializers) +{ + const auto oversized_prefix = [](Consensus::LLMQType type) { + CDataStream s(SER_NETWORK, PROTOCOL_VERSION); + s << type << uint256::ONE << uint256::TWO; + WriteCompactSize(s, MAX_SIZE); // first bitset, with no payload behind it + return s; + }; + + const Consensus::LLMQType type = Consensus::LLMQType::LLMQ_400_85; + + CDataStream complaint = oversized_prefix(type); + CDKGComplaint qc; + BOOST_CHECK_EXCEPTION(complaint >> qc, std::ios_base::failure, BitsetLimitExceeded()); + + CDataStream commitment = oversized_prefix(type); + CDKGPrematureCommitment qpc; + BOOST_CHECK_EXCEPTION(commitment >> qpc, std::ios_base::failure, BitsetLimitExceeded()); + + CDataStream final_commitment(SER_NETWORK, PROTOCOL_VERSION); + final_commitment << uint16_t{CFinalCommitment::LEGACY_BLS_NON_INDEXED_QUORUM_VERSION} << type << uint256::ONE; + WriteCompactSize(final_commitment, MAX_SIZE); // signers + CFinalCommitment fqc; + BOOST_CHECK_EXCEPTION(final_commitment >> fqc, std::ios_base::failure, BitsetLimitExceeded()); +} + +// Vector-valued DKG fields are bounded the same way, so a declared element count cannot outrun the +// bytes behind it before the worker gets a chance to reject the message. +BOOST_AUTO_TEST_CASE(oversized_vectors_rejected_through_message_serializers) +{ + const Consensus::LLMQType type = Consensus::LLMQType::LLMQ_400_85; + + CDataStream justification(SER_NETWORK, PROTOCOL_VERSION); + justification << type << uint256::ONE << uint256::TWO; + WriteCompactSize(justification, Consensus::MAX_LLMQ_SIZE + 1); + CDKGJustification qj; + BOOST_CHECK_EXCEPTION(justification >> qj, std::ios_base::failure, VectorLimitExceeded()); + + CDataStream contribution(SER_NETWORK, PROTOCOL_VERSION); + contribution << type << uint256::ONE << uint256::TWO; + WriteCompactSize(contribution, Consensus::MAX_LLMQ_SIZE + 1); // vvec + CDKGContribution qc; + BOOST_CHECK_EXCEPTION(contribution >> qc, std::ios_base::failure, VectorLimitExceeded()); +} + +// Every DKG message a real quorum can produce must still round-trip, for the smallest and the +// largest quorum type -- a bound that rejected honest traffic would partition the node. +BOOST_AUTO_TEST_CASE(well_formed_messages_roundtrip) +{ + for (const auto type : {Consensus::LLMQType::LLMQ_TEST, Consensus::LLMQType::LLMQ_400_85}) { + const Consensus::LLMQParams& params = GetLLMQParams(type); + + CDKGComplaint complaint(params); + complaint.llmqType = params.type; + complaint.quorumHash = uint256::ONE; + complaint.proTxHash = uint256::TWO; + for (int i = 0; i < params.size; i += 2) { + complaint.badMembers[i] = true; + } + complaint.complainForMembers[0] = true; + complaint.sig = CreateRandomBLSSignature(); + + CDataStream s(SER_NETWORK, PROTOCOL_VERSION); + s << complaint; + CDKGComplaint decoded; + s >> decoded; + BOOST_CHECK(decoded.badMembers == complaint.badMembers); + BOOST_CHECK(decoded.complainForMembers == complaint.complainForMembers); + + CDKGPrematureCommitment commitment(params); + commitment.llmqType = params.type; + commitment.quorumHash = uint256::ONE; + commitment.proTxHash = uint256::TWO; + for (int i = 0; i < params.size; ++i) { + commitment.validMembers[i] = true; + } + commitment.quorumPublicKey = CreateRandomBLSPublicKey(); + commitment.quorumVvecHash = uint256::ONE; + commitment.quorumSig = CreateRandomBLSSignature(); + commitment.sig = CreateRandomBLSSignature(); + + CDataStream s2(SER_NETWORK, PROTOCOL_VERSION); + s2 << commitment; + CDKGPrematureCommitment decoded2; + s2 >> decoded2; + BOOST_CHECK(decoded2.validMembers == commitment.validMembers); + BOOST_CHECK_EQUAL(decoded2.CountValidMembers(), params.size); + + // Worst legitimate justification: one contribution per quorum member, at the bound. + CDKGJustification justification; + justification.llmqType = params.type; + justification.quorumHash = uint256::ONE; + justification.proTxHash = uint256::TWO; + for (int i = 0; i < params.size; ++i) { + CBLSSecretKey sk; + sk.MakeNewKey(); + justification.contributions.push_back({static_cast(i), sk}); + } + justification.sig = CreateRandomBLSSignature(); + + CDataStream s3(SER_NETWORK, PROTOCOL_VERSION); + s3 << justification; + CDKGJustification decoded3; + s3 >> decoded3; + BOOST_CHECK_EQUAL(decoded3.contributions.size(), size_t(params.size)); + + // A member with nothing to justify sends an empty vector; the bound must admit that too. + justification.contributions.clear(); + CDataStream s4(SER_NETWORK, PROTOCOL_VERSION); + s4 << justification; + CDKGJustification decoded4; + s4 >> decoded4; + BOOST_CHECK(decoded4.contributions.empty()); + + // CDKGContribution hand-rolls Serialize/Unserialize rather than using SERIALIZE_METHODS, so + // its vvec bound is wired up separately and needs its own round-trip. threshold is the + // largest vvec an honest member sends (340 for llmq_400_85). + CDKGContribution contribution; + contribution.llmqType = params.type; + contribution.quorumHash = uint256::ONE; + contribution.proTxHash = uint256::TWO; + auto vvec = std::make_shared>(); + for (int i = 0; i < params.threshold; ++i) { + vvec->push_back(CreateRandomBLSPublicKey()); + } + contribution.vvec = std::move(vvec); + auto contributions = std::make_shared>(); + contributions->ephemeralPubKey = CreateRandomBLSPublicKey(); + contributions->ivSeed = uint256::ONE; + contributions->blobs.assign(params.size, std::vector(48, 0xab)); + contribution.contributions = std::move(contributions); + contribution.sig = CreateRandomBLSSignature(); + + CDataStream s5(SER_NETWORK, PROTOCOL_VERSION); + s5 << contribution; + CDKGContribution decoded5; + s5 >> decoded5; + BOOST_CHECK_EQUAL(decoded5.vvec->size(), size_t(params.threshold)); + BOOST_CHECK_EQUAL(decoded5.contributions->blobs.size(), size_t(params.size)); + BOOST_CHECK(decoded5.vvec->front() == contribution.vvec->front()); + } +} + +BOOST_AUTO_TEST_SUITE_END()