From c0af1562af5a17528cc20c1f704cdc086ef50e2b Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 9 Jul 2026 21:17:25 -0500 Subject: [PATCH 1/3] llmq: bound batched sig-share intake CBatchedSigShares::sigShares is a std::vector whose upper bound is the compile-time constant MAX_MSGS_TOTAL_BATCHED_SIGS, so express the cap inside SERIALIZE_METHODS with LIMITED_VECTOR. Wire counts above the cap now throw before any element is decoded or memory allocated, and the write side stays interoperable with the unbounded reader. At the QBSIGSHARES callsite the inner cap alone is not enough: many individually-valid batches can still exceed the total sig-share cap. Bound the outer batch count via ReadCompactSize with range_check=false (so this callsite's own limit, not the generic MAX_SIZE cap, decides) and check the running sig-share total as each batch is decoded so we stop before an attacker forces us through the full cross product of the per-vector limits. Any failure is logged, bans the peer, and rethrows. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/llmq/net_signing.cpp | 33 ++++++++++++++++++++++++--------- src/llmq/signing_shares.h | 2 +- src/test/llmq_utils_tests.cpp | 26 ++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 10 deletions(-) diff --git a/src/llmq/net_signing.cpp b/src/llmq/net_signing.cpp index 3b622e5b05aa..5376d1f328aa 100644 --- a/src/llmq/net_signing.cpp +++ b/src/llmq/net_signing.cpp @@ -10,7 +10,6 @@ #include #include #include -#include #include #include @@ -98,16 +97,32 @@ void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CData return; } } else if (msg_type == NetMsgType::QBSIGSHARES) { + // The inner sigShares vector is bounded by CBatchedSigShares's SERIALIZE_METHODS + // via LIMITED_VECTOR, but many individually-valid batches could still exceed the + // total sig-share cap, so bound the outer count and check the running total as we + // decode so we stop reading before an attacker forces us through the full cross + // product of the per-vector limits. 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 { + 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"); + } + } + } 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/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..37e812d2df05 100644 --- a/src/test/llmq_utils_tests.cpp +++ b/src/test/llmq_utils_tests.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include @@ -171,6 +172,31 @@ 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); +} + BOOST_AUTO_TEST_CASE(deterministic_outbound_connection_test) { // Test deterministic behavior From 89c0933aa1cbf7a512cd57a8d4f7f0b42fe01b27 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 9 Jul 2026 21:18:34 -0500 Subject: [PATCH 2/3] llmq: bound LLMQ signing vector intake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The QSIGSHARE, QSIGSESANN and QSIGSHARESINV/QGETSIGSHARES handlers each decoded a std::vector unbounded before comparing the resulting count against a callsite-specific cap. That lets a malicious peer force us to allocate and decode arbitrarily large batches before we reject them. Replace the post-decode size checks with the shared UnserializeVectorWithMaxSize primitive, which reads the CompactSize prefix with the default range-checked ReadCompactSize (rejecting any count above the generic MAX_SIZE cap before this tighter callsite max_size is even applied), compares the resulting size_t directly against max_size with no separate uint64_t staging, and returns false without consuming any element on rejection. Convert the false return into an ios_base::failure so the surrounding catch uniformly logs, bans the peer, and rethrows for any decode failure — matching the existing QSIGSHARESINV/QGETSIGSHARES behavior on a malformed stream. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/llmq/net_signing.cpp | 42 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/llmq/net_signing.cpp b/src/llmq/net_signing.cpp index 5376d1f328aa..8983ed68b49e 100644 --- a/src/llmq/net_signing.cpp +++ b/src/llmq/net_signing.cpp @@ -44,13 +44,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) { @@ -62,13 +64,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); @@ -79,17 +83,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); })) { From 4f129b609e3aed8c55eb9006b06982fa1c88d878 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Fri, 10 Jul 2026 06:21:23 -0500 Subject: [PATCH 3/3] llmq: cover QBSIGSHARES aggregate sig-share bound with a testable decoder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The self-review of the batched sig-share intake bounds flagged one gap: the running-total abort in the QBSIGSHARES handler — where many individually-valid batches together exceed MAX_MSGS_TOTAL_BATCHED_SIGS — had no direct unit coverage. The check lived inline in NetSigning::ProcessMessage, so the only way to reach it was to drive a full P2P message through the handler. Extract the QBSIGSHARES decode loop into a free function, llmq::UnserializeBatchedSigShares(CDataStream&), that production calls inside the existing try/catch. Wire format, peer banning, and logging semantics are unchanged — the callsite still logs, bans, and rethrows on any ios_base::failure. The extracted decoder is the exact code that runs in production, so the new tests exercise the real path rather than a mirror. Add three targeted cases in llmq_utils_tests: - oversized outer batch count (built from empty batches so only the outer-count guard, not an end-of-stream artifact, can reject it), - oversized running aggregate across batches each within the per-batch cap, - an aggregate exactly at the cap, which must decode intact. Both count guards were mutation-verified: neutralizing either guard makes its corresponding test fail and no other. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/llmq/net_signing.cpp | 40 +++++++++++++------------ src/llmq/net_signing.h | 12 ++++++++ src/test/llmq_utils_tests.cpp | 56 +++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 19 deletions(-) diff --git a/src/llmq/net_signing.cpp b/src/llmq/net_signing.cpp index 8983ed68b49e..0883358b14c8 100644 --- a/src/llmq/net_signing.cpp +++ b/src/llmq/net_signing.cpp @@ -23,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) { @@ -99,27 +119,9 @@ void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CData return; } } else if (msg_type == NetMsgType::QBSIGSHARES) { - // The inner sigShares vector is bounded by CBatchedSigShares's SERIALIZE_METHODS - // via LIMITED_VECTOR, but many individually-valid batches could still exceed the - // total sig-share cap, so bound the outer count and check the running total as we - // decode so we stop reading before an attacker forces us through the full cross - // product of the per-vector limits. std::vector msgs; try { - 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"); - } - } + 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()); 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/test/llmq_utils_tests.cpp b/src/test/llmq_utils_tests.cpp index 37e812d2df05..a3f80e5aa1f9 100644 --- a/src/test/llmq_utils_tests.cpp +++ b/src/test/llmq_utils_tests.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -197,6 +198,61 @@ BOOST_AUTO_TEST_CASE(batched_sig_shares_accepts_max_inner_vector) 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