Merge dashpay#7418: fix(net): bound signing message vector intake

8ff75e0 llmq: cover QBSIGSHARES aggregate sig-share bound with a testable decoder (PastaClaw)
e51e304 llmq: bound LLMQ signing vector intake (PastaClaw)
c0af156 llmq: bound batched sig-share intake (PastaClaw)

Pull request description:

  Depends on dashpay#7439. Please review only the two command-specific commits here.

  ## Issue being fixed or feature implemented

  LLMQ signing P2P messages previously deserialized peer-controlled vectors before enforcing the existing per-message count limits. This hardens the signing message intake path so oversized counts are rejected before vector materialization.

  This intentionally does not include the QFCOMMITMENT dynamic-bitset fix.

  ## What was done?

  - Use the shared runtime-bounded vector reader for signing message batches in `NetSigning::ProcessMessage`.
  - Bound `CBatchedSigShares::sigShares` declaratively with `LIMITED_VECTOR`.
  - Retain explicit running-total accounting for QBSIGSHARES, where many individually valid batches could exceed the aggregate cap.
  - Ban peers on malformed or oversized signing vectors.
  - Add unit coverage for the exact boundary and oversized batched sig-share vectors.

  The reusable bounded-vector serialization primitives are introduced separately in dashpay#7439.

  ## How Has This Been Tested?

  - `src/test/test_dash --run_test=llmq_utils_tests`
  - `src/test/test_dash --run_test=serialize_tests`
  - `test/lint/lint-whitespace.py`
  - `test/lint/lint-circular-dependencies.py`

  ## Breaking Changes

  None.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)

ACKs for top commit:
  UdjinM6:
    utACK 8ff75e0

Tree-SHA512: 873fb78ef55749449c0548828dc8709bf0709637732a000bd009d8bba9ed0270e1582eb0020f852c3a8e8bb6d448d1f7b8c0402e271ac42155081a9d13d25bc8

Author: PastaPastaPasta

src/llmq/net_signing.cpp

@@ -10,7 +10,6 @@
 #include <llmq/signing_shares.h>
 #include <llmq/signing.h>
 #include <spork.h>
-#include <util/std23.h>
 
 #include <logging.h>
 #include <streams.h>
@@ -24,6 +23,26 @@
 #include <unordered_map>
 
 namespace llmq {
+std::vector<CBatchedSigShares> UnserializeBatchedSigShares(CDataStream& vRecv)
+{
+    std::vector<CBatchedSigShares> 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<CSigShare> 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<CSigSesAnn> 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<CSigSharesInv> 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<CBatchedSigShares> 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);

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<CBatchedSigShares> UnserializeBatchedSigShares(CDataStream& vRecv);
+
 class NetSigning final : public NetHandler, public CValidationInterface
 {
 public:

src/llmq/signing_shares.h

@@ -154,7 +154,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;

src/test/llmq_utils_tests.cpp

@@ -6,11 +6,13 @@
 #include <test/util/setup_common.h>
 
 #include <consensus/params.h>
+#include <llmq/net_signing.h>
 #include <llmq/params.h>
 #include <llmq/signing_shares.h>
 #include <llmq/utils.h>
 #include <netaddress.h>
 #include <random.h>
+#include <streams.h>
 
 #include <boost/test/unit_test.hpp>
 
@@ -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<CBatchedSigShares> 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<CBatchedSigShares> 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<CBatchedSigShares> 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<CBatchedSigShares> 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