diff --git a/src/llmq/net_signing.cpp b/src/llmq/net_signing.cpp index 3b622e5b05aa..0883358b14c8 100644 --- a/src/llmq/net_signing.cpp +++ b/src/llmq/net_signing.cpp @@ -10,7 +10,6 @@ #include #include #include -#include #include #include @@ -24,6 +23,26 @@ #include namespace llmq { +std::vector UnserializeBatchedSigShares(CDataStream& vRecv) +{ + std::vector msgs; + const uint64_t msgs_size{ReadCompactSize(vRecv, /*range_check=*/false)}; + if (msgs_size > MAX_MSGS_TOTAL_BATCHED_SIGS) { + throw std::ios_base::failure("QBSIGSHARES batch count too large"); + } + msgs.reserve(msgs_size); + size_t total_sigs_count{0}; + while (msgs.size() < msgs_size) { + msgs.emplace_back(); + vRecv >> msgs.back(); + total_sigs_count += msgs.back().sigShares.size(); + if (total_sigs_count > MAX_MSGS_TOTAL_BATCHED_SIGS) { + throw std::ios_base::failure("QBSIGSHARES sig share count too large"); + } + } + return msgs; +} + void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv) { if (msg_type == NetMsgType::QSIGREC) { @@ -45,13 +64,15 @@ void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CData if (m_sporkman.IsSporkActive(SPORK_21_QUORUM_ALL_CONNECTED) && msg_type == NetMsgType::QSIGSHARE) { std::vector receivedSigShares; - vRecv >> receivedSigShares; - - if (receivedSigShares.size() > CSigSharesManager::MAX_MSGS_SIG_SHARES) { - LogPrint(BCLog::LLMQ_SIGS, "NetSigning::%s -- too many sigs in QSIGSHARE message. cnt=%d, max=%d, node=%d\n", - __func__, receivedSigShares.size(), CSigSharesManager::MAX_MSGS_SIG_SHARES, pfrom.GetId()); + try { + if (!UnserializeVectorWithMaxSize(vRecv, receivedSigShares, CSigSharesManager::MAX_MSGS_SIG_SHARES)) { + throw std::ios_base::failure("QSIGSHARE vector size too large"); + } + } catch (const std::ios_base::failure& e) { + LogPrint(BCLog::LLMQ_SIGS, "NetSigning::%s -- rejected %s from peer=%d: %s\n", + __func__, msg_type, pfrom.GetId(), e.what()); BanNode(pfrom.GetId()); - return; + throw; } for (const auto& sigShare : receivedSigShares) { @@ -63,13 +84,15 @@ void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CData if (msg_type == NetMsgType::QSIGSESANN) { std::vector msgs; - vRecv >> msgs; - if (msgs.size() > CSigSharesManager::MAX_MSGS_CNT_QSIGSESANN) { - LogPrint(BCLog::LLMQ_SIGS, /* Continued */ - "NetSigning::%s -- too many announcements in QSIGSESANN message. cnt=%d, max=%d, node=%d\n", - __func__, msgs.size(), CSigSharesManager::MAX_MSGS_CNT_QSIGSESANN, pfrom.GetId()); + try { + if (!UnserializeVectorWithMaxSize(vRecv, msgs, CSigSharesManager::MAX_MSGS_CNT_QSIGSESANN)) { + throw std::ios_base::failure("QSIGSESANN vector size too large"); + } + } catch (const std::ios_base::failure& e) { + LogPrint(BCLog::LLMQ_SIGS, "NetSigning::%s -- rejected %s from peer=%d: %s\n", + __func__, msg_type, pfrom.GetId(), e.what()); BanNode(pfrom.GetId()); - return; + throw; } if (!std::ranges::all_of(msgs, [this, &pfrom](const auto& ann) { return m_shares_manager->ProcessMessageSigSesAnn(pfrom, ann); @@ -80,17 +103,15 @@ void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CData } else if (msg_type == NetMsgType::QSIGSHARESINV || msg_type == NetMsgType::QGETSIGSHARES) { std::vector msgs; try { - vRecv >> msgs; - } catch (const std::ios_base::failure&) { + if (!UnserializeVectorWithMaxSize(vRecv, msgs, CSigSharesManager::MAX_MSGS_CNT_QSIGSHARES)) { + throw std::ios_base::failure("QSIGSHARESINV vector size too large"); + } + } catch (const std::ios_base::failure& e) { + LogPrint(BCLog::LLMQ_SIGS, "NetSigning::%s -- rejected %s from peer=%d: %s\n", + __func__, msg_type, pfrom.GetId(), e.what()); BanNode(pfrom.GetId()); throw; } - if (msgs.size() > CSigSharesManager::MAX_MSGS_CNT_QSIGSHARES) { - LogPrint(BCLog::LLMQ_SIGS, "NetSigning::%s -- too many invs in %s message. cnt=%d, max=%d, node=%d\n", - __func__, msg_type, msgs.size(), CSigSharesManager::MAX_MSGS_CNT_QSIGSHARES, pfrom.GetId()); - BanNode(pfrom.GetId()); - return; - } if (!std::ranges::all_of(msgs, [this, &pfrom, &msg_type](const auto& inv) { return m_shares_manager->ProcessMessageSigShares(pfrom, inv, msg_type); })) { @@ -99,15 +120,13 @@ void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CData } } else if (msg_type == NetMsgType::QBSIGSHARES) { std::vector msgs; - vRecv >> msgs; - const size_t totalSigsCount = std23::ranges::fold_left(msgs, size_t{0}, [](size_t s, const auto& bs) { - return s + bs.sigShares.size(); - }); - if (totalSigsCount > MAX_MSGS_TOTAL_BATCHED_SIGS) { - LogPrint(BCLog::LLMQ_SIGS, "NetSigning::%s -- too many sigs in QBSIGSHARES message. cnt=%d, max=%d, node=%d\n", - __func__, msgs.size(), MAX_MSGS_TOTAL_BATCHED_SIGS, pfrom.GetId()); + try { + msgs = UnserializeBatchedSigShares(vRecv); + } catch (const std::ios_base::failure& e) { + LogPrint(BCLog::LLMQ_SIGS, "NetSigning::%s -- rejected %s from peer=%d: %s\n", + __func__, msg_type, pfrom.GetId(), e.what()); BanNode(pfrom.GetId()); - return; + throw; } if (!std::ranges::all_of(msgs, [this, &pfrom](const auto& bs) { return m_shares_manager->ProcessMessageBatchedSigShares(pfrom, bs); diff --git a/src/llmq/net_signing.h b/src/llmq/net_signing.h index 431e6efb1537..2dcbaf45a7b3 100644 --- a/src/llmq/net_signing.h +++ b/src/llmq/net_signing.h @@ -24,6 +24,18 @@ namespace llmq { class CSigSharesManager; class CSigningManager; +//! Decode a QBSIGSHARES payload into its vector of CBatchedSigShares. +//! +//! The inner sigShares vector of each batch is bounded by CBatchedSigShares's +//! SERIALIZE_METHODS via LIMITED_VECTOR, but many individually-valid batches +//! could still exceed the total sig-share cap, so this bounds the outer batch +//! count and checks the running total of inner sig shares as it decodes, +//! stopping before an attacker forces us through the full cross product of the +//! per-vector limits. A wire count above the cap (outer batch count or running +//! inner total) throws std::ios_base::failure once detected, leaving the caller +//! to log, ban, and rethrow uniformly. +std::vector UnserializeBatchedSigShares(CDataStream& vRecv); + class NetSigning final : public NetHandler, public CValidationInterface { public: diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index 332f553fc895..17b825c4b094 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -153,7 +153,7 @@ class CBatchedSigShares public: SERIALIZE_METHODS(CBatchedSigShares, obj) { - READWRITE(VARINT(obj.sessionId), obj.sigShares); + READWRITE(VARINT(obj.sessionId), LIMITED_VECTOR(obj.sigShares, MAX_MSGS_TOTAL_BATCHED_SIGS)); } [[nodiscard]] std::string ToInvString() const; diff --git a/src/test/llmq_utils_tests.cpp b/src/test/llmq_utils_tests.cpp index 839652b73f8e..a3f80e5aa1f9 100644 --- a/src/test/llmq_utils_tests.cpp +++ b/src/test/llmq_utils_tests.cpp @@ -6,11 +6,13 @@ #include #include +#include #include #include #include #include #include +#include #include @@ -171,6 +173,86 @@ BOOST_AUTO_TEST_CASE(pending_sig_shares_session_removal_updates_count) BOOST_CHECK_EQUAL(node_state.pendingIncomingSigShares.Size(), 1U); } +BOOST_AUTO_TEST_CASE(batched_sig_shares_rejects_oversized_inner_vector) +{ + CDataStream stream{SER_NETWORK, PROTOCOL_VERSION}; + stream << VARINT(uint32_t{1}); + WriteCompactSize(stream, MAX_MSGS_TOTAL_BATCHED_SIGS + 1); + + CBatchedSigShares batched_sig_shares; + BOOST_CHECK_THROW(stream >> batched_sig_shares, std::ios_base::failure); + BOOST_CHECK(batched_sig_shares.sigShares.empty()); +} + +BOOST_AUTO_TEST_CASE(batched_sig_shares_accepts_max_inner_vector) +{ + CBatchedSigShares batched; + batched.sessionId = 1; + batched.sigShares.resize(MAX_MSGS_TOTAL_BATCHED_SIGS); // exactly the cap must be accepted + + CDataStream stream{SER_NETWORK, PROTOCOL_VERSION}; + stream << batched; + + CBatchedSigShares roundtripped; + BOOST_CHECK_NO_THROW(stream >> roundtripped); + BOOST_CHECK_EQUAL(roundtripped.sigShares.size(), MAX_MSGS_TOTAL_BATCHED_SIGS); +} + +static CBatchedSigShares MakeBatch(uint32_t session_id, size_t share_count) +{ + CBatchedSigShares batch; + batch.sessionId = session_id; + batch.sigShares.resize(share_count); // default pairs are enough to exercise the count invariant + return batch; +} + +// Exercise the production QBSIGSHARES decoder directly: the outer batch count is +// bounded regardless of the inner contents. Every batch here is empty, so the +// aggregate running-total guard never fires; only the outer-count guard can +// reject the stream, which keeps this case a genuine test of that guard rather +// than an end-of-stream artifact. +BOOST_AUTO_TEST_CASE(qbsigshares_rejects_oversized_batch_count) +{ + std::vector msgs(MAX_MSGS_TOTAL_BATCHED_SIGS + 1); // count over cap, 0 inner shares each + + CDataStream stream{SER_NETWORK, PROTOCOL_VERSION}; + stream << msgs; + + BOOST_CHECK_THROW(UnserializeBatchedSigShares(stream), std::ios_base::failure); +} + +// The regression this targets: each batch is individually within the per-batch +// LIMITED_VECTOR cap, but their running aggregate exceeds MAX_MSGS_TOTAL_BATCHED_SIGS, +// so the decoder must abort mid-stream rather than accept the cross product. +BOOST_AUTO_TEST_CASE(qbsigshares_rejects_oversized_aggregate_total) +{ + std::vector msgs; + msgs.push_back(MakeBatch(1, 300)); + msgs.push_back(MakeBatch(2, 200)); // 300 + 200 = 500 > 400, though each batch is <= the cap + + CDataStream stream{SER_NETWORK, PROTOCOL_VERSION}; + stream << msgs; + + BOOST_CHECK_THROW(UnserializeBatchedSigShares(stream), std::ios_base::failure); +} + +// Batches whose aggregate is exactly at the cap must be accepted and decoded intact. +BOOST_AUTO_TEST_CASE(qbsigshares_accepts_aggregate_at_cap) +{ + std::vector msgs; + msgs.push_back(MakeBatch(1, 200)); + msgs.push_back(MakeBatch(2, MAX_MSGS_TOTAL_BATCHED_SIGS - 200)); // aggregate == cap + + CDataStream stream{SER_NETWORK, PROTOCOL_VERSION}; + stream << msgs; + + std::vector decoded; + BOOST_CHECK_NO_THROW(decoded = UnserializeBatchedSigShares(stream)); + BOOST_CHECK_EQUAL(decoded.size(), 2U); + BOOST_CHECK_EQUAL(decoded[0].sigShares.size(), 200U); + BOOST_CHECK_EQUAL(decoded[1].sigShares.size(), MAX_MSGS_TOTAL_BATCHED_SIGS - 200); +} + BOOST_AUTO_TEST_CASE(deterministic_outbound_connection_test) { // Test deterministic behavior