From d2e600f231e42d6d3a144cface7730fdac97c038 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 9 Jul 2026 20:42:51 -0500 Subject: [PATCH 1/3] fix: bound CSporkMessage signature vector allocation Add an idiomatic serialization wrapper LIMITED_VECTOR / LimitedVectorFormatter analogous to LIMITED_STRING and backed by the existing UnserializeVectorWithMaxSize primitive. Serialization preserves the ordinary std::vector wire format so it is a drop-in wrapper for fields declared as an unbounded vector but bounded in practice; deserialization rejects a CompactSize above the compile-time limit before any element allocation via std::ios_base::failure. Apply it to CSporkMessage::vchSig with CPubKey::COMPACT_SIGNATURE_SIZE (65) as the cap. Prior to this change a peer could hand us a bogus wire count on a SPORK message, causing the vector to be resized in chunks well before we had any chance to reject the message on signature verification. Shorter well-formed signatures continue to deserialize and are rejected by the existing cryptographic invalid-spork path. Catch malformed / truncated / oversized SPORK deserialization locally in the SPORK handler and Misbehaving(100) the peer so replay is punished rather than silently dropped by the generic ProcessMessages catch. The catch is narrowed to std::ios_base::failure so unrelated internal failures during message dispatch are not attributed to the peer. Wire format for valid 65-byte messages is unchanged on every network, including testnet's different signing preimage. --- src/net_processing.cpp | 8 +++++++- src/spork.h | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2aabdc199172..f61add3b6d95 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5524,7 +5524,13 @@ void PeerManagerImpl::ProcessMessage( if (msg_type == NetMsgType::SPORK) { CSporkMessage spork; - vRecv >> spork; + try { + vRecv >> spork; + } catch (const std::ios_base::failure& e) { + // Attribute deserialization failures to the peer; the outer catch would otherwise silently drop. + Misbehaving(*peer, 100, strprintf("malformed spork received. peer=%d error=%s", pfrom.GetId(), e.what())); + return; + } uint256 hash = spork.GetHash(); CInv spork_inv{MSG_SPORK, hash}; diff --git a/src/spork.h b/src/spork.h index bb1c03bef74b..9d23fa68b240 100644 --- a/src/spork.h +++ b/src/spork.h @@ -109,7 +109,8 @@ class CSporkMessage SERIALIZE_METHODS(CSporkMessage, obj) { - READWRITE(obj.nSporkID, obj.nValue, obj.nTimeSigned, obj.vchSig); + READWRITE(obj.nSporkID, obj.nValue, obj.nTimeSigned, + LIMITED_VECTOR(obj.vchSig, CPubKey::COMPACT_SIGNATURE_SIZE)); } /** From 44a8fe16567368c6e8cb0af2668b5d172eb281d7 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 9 Jul 2026 20:43:02 -0500 Subject: [PATCH 2/3] test: regression coverage for CSporkMessage signature-size bound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a Boost unit suite that pins the LimitedVectorFormatter gate applied to CSporkMessage::vchSig: - the vulnerability report's exact trigger — a canonical CompactSize encoding of MAX_SIZE with no signature bytes — must be rejected before allocation; the predicate matches on the formatter's "Vector length limit exceeded" so the assertion fails on the vulnerable implementation (which throws a downstream stream-underflow) and passes only on the fix; - a +1 boundary with a full payload short-circuits before element decoding even when every promised byte is on the wire; - a valid 65-byte signature round-trips through serialize/unserialize with the wire bytes unchanged, guaranteeing wire compatibility. Shorter well-formed signatures are intentionally left to the existing cryptographic invalid-spork punishment path, so no deserialize-time too-short assertion is added. Extend feature_sporks.py with the smallest practical p2p assertion: send a malformed SPORK carrying the oversized count through a dedicated peer and require the node to disconnect it. Existing propagation, GETSPORKS, persistence, and multi-node relay coverage is preserved. --- src/Makefile.test.include | 1 + src/test/spork_tests.cpp | 93 +++++++++++++++++++++++++++++++ test/functional/feature_sporks.py | 11 ++++ 3 files changed, 105 insertions(+) create mode 100644 src/test/spork_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index e2f1782e5d42..96e58b347d89 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -185,6 +185,7 @@ BITCOIN_TESTS =\ test/sigopcount_tests.cpp \ test/skiplist_tests.cpp \ test/sock_tests.cpp \ + test/spork_tests.cpp \ test/streams_tests.cpp \ test/subsidy_tests.cpp \ test/sync_tests.cpp \ diff --git a/src/test/spork_tests.cpp b/src/test/spork_tests.cpp new file mode 100644 index 000000000000..ebed7fc33abc --- /dev/null +++ b/src/test/spork_tests.cpp @@ -0,0 +1,93 @@ +// 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 + +BOOST_FIXTURE_TEST_SUITE(spork_tests, BasicTestingSetup) + +namespace { +// Build a CSporkMessage wire encoding by hand: 4-byte SporkId, 8-byte value, +// 8-byte timestamp, then a CompactSize-prefixed signature vector. +CDataStream MakeSporkStream(int32_t sporkId, int64_t nValue, int64_t nTimeSigned, + uint64_t sig_compact_size, size_t sig_bytes_to_write) +{ + CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); + ss << sporkId << nValue << nTimeSigned; + WriteCompactSize(ss, sig_compact_size); + for (size_t i = 0; i < sig_bytes_to_write; ++i) { + ss << static_cast(0xab); + } + return ss; +} + +// Distinguishes the pre-allocation size-gate throw from a downstream stream underflow. +bool IsLimitExceededFailure(const std::ios_base::failure& e) +{ + return std::string_view{e.what()}.find("Vector length limit exceeded") != std::string_view::npos; +} +} // namespace + +// The vulnerability report's exact trigger: a canonical CompactSize(MAX_SIZE) +// with no signature bytes. On the vulnerable implementation this reaches the +// stock vector Unserialize path, which resizes the destination in chunks +// (allocating before it discovers there is no data) and then throws a generic +// stream underflow. On the fix, LimitedVectorFormatter rejects the count +// before any allocation via ios_base::failure. +BOOST_AUTO_TEST_CASE(spork_signature_max_size_rejected_before_allocation) +{ + CDataStream ss = MakeSporkStream(/*sporkId=*/10001, 0, 0, + /*sig_compact_size=*/MAX_SIZE, + /*sig_bytes_to_write=*/0); + CSporkMessage spork; + BOOST_CHECK_EXCEPTION(ss >> spork, std::ios_base::failure, IsLimitExceededFailure); +} + +// A +1 boundary with a full payload — the bound must short-circuit before +// element decoding even when every promised byte is on the wire. +BOOST_AUTO_TEST_CASE(spork_signature_oversized_rejected) +{ + CDataStream ss = MakeSporkStream(/*sporkId=*/10001, 0, 0, + /*sig_compact_size=*/CPubKey::COMPACT_SIGNATURE_SIZE + 1, + /*sig_bytes_to_write=*/CPubKey::COMPACT_SIGNATURE_SIZE + 1); + CSporkMessage spork; + BOOST_CHECK_EXCEPTION(ss >> spork, std::ios_base::failure, IsLimitExceededFailure); +} + +// A valid 65-byte signature vector round-trips through serialize/unserialize +// with the wire bytes unchanged — the fix must not have moved the wire format +// for legit messages. +BOOST_AUTO_TEST_CASE(spork_signature_exact_size_roundtrip) +{ + CDataStream ss = MakeSporkStream(/*sporkId=*/10001, /*nValue=*/123456789, + /*nTimeSigned=*/1'700'000'000, + /*sig_compact_size=*/CPubKey::COMPACT_SIGNATURE_SIZE, + /*sig_bytes_to_write=*/CPubKey::COMPACT_SIGNATURE_SIZE); + const std::vector wire(ss.begin(), ss.end()); + + CSporkMessage decoded; + BOOST_REQUIRE_NO_THROW(ss >> decoded); + BOOST_CHECK_EQUAL(ss.size(), 0U); + BOOST_CHECK_EQUAL(decoded.nSporkID, static_cast(10001)); + BOOST_CHECK_EQUAL(decoded.nValue, 123456789); + BOOST_CHECK_EQUAL(decoded.nTimeSigned, 1'700'000'000); + + CDataStream re(SER_NETWORK, PROTOCOL_VERSION); + re << decoded; + BOOST_REQUIRE_EQUAL(re.size(), wire.size()); + BOOST_CHECK(std::equal(re.begin(), re.end(), wire.begin())); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/test/functional/feature_sporks.py b/test/functional/feature_sporks.py index 7940c1aa9b86..522d8f7a58d9 100755 --- a/test/functional/feature_sporks.py +++ b/test/functional/feature_sporks.py @@ -3,6 +3,9 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. +import struct + +from test_framework.messages import ser_compact_size from test_framework.p2p import MESSAGEMAP, P2PInterface from test_framework.test_framework import BitcoinTestFramework @@ -117,6 +120,14 @@ def run_test(self): assert "" not in self.nodes[0].spork('show').keys() + # Oversized signature length prefix must trigger disconnect, not silent drop. + MAX_SIZE = 0x02000000 + bad_spork = msg_spork_raw() + bad_spork.raw = struct.pack(" Date: Fri, 10 Jul 2026 09:05:13 -0500 Subject: [PATCH 3/3] test: drop redundant spork_tests unit suite Reviewer feedback (kwvg) on #7438: the CSporkMessage unit checks in spork_tests.cpp are redundant and unclear. The generic bounded-vector primitive is already exercised thoroughly by serialize_tests.cpp (within-limit round trip, wire compatibility, exact/one-over boundary, no-element-decode-on-reject, MAX_SIZE / 0xfe / 0xff CompactSize gate, unbounded serialization), and the concrete CSporkMessage oversized-signature path is covered end-to-end by feature_sporks.py, which sends a MAX_SIZE signature-length prefix and asserts the peer is disconnected rather than silently dropped. Remove the file and its src/Makefile.test.include entry. The deserialization bound itself (LIMITED_VECTOR on CSporkMessage::vchSig), the serialize.h primitives, the net_processing peer-attribution, and the serialize_tests / feature_sporks coverage are all retained. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/Makefile.test.include | 1 - src/test/spork_tests.cpp | 93 --------------------------------------- 2 files changed, 94 deletions(-) delete mode 100644 src/test/spork_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 96e58b347d89..e2f1782e5d42 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -185,7 +185,6 @@ BITCOIN_TESTS =\ test/sigopcount_tests.cpp \ test/skiplist_tests.cpp \ test/sock_tests.cpp \ - test/spork_tests.cpp \ test/streams_tests.cpp \ test/subsidy_tests.cpp \ test/sync_tests.cpp \ diff --git a/src/test/spork_tests.cpp b/src/test/spork_tests.cpp deleted file mode 100644 index ebed7fc33abc..000000000000 --- a/src/test/spork_tests.cpp +++ /dev/null @@ -1,93 +0,0 @@ -// 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 - -BOOST_FIXTURE_TEST_SUITE(spork_tests, BasicTestingSetup) - -namespace { -// Build a CSporkMessage wire encoding by hand: 4-byte SporkId, 8-byte value, -// 8-byte timestamp, then a CompactSize-prefixed signature vector. -CDataStream MakeSporkStream(int32_t sporkId, int64_t nValue, int64_t nTimeSigned, - uint64_t sig_compact_size, size_t sig_bytes_to_write) -{ - CDataStream ss(SER_NETWORK, PROTOCOL_VERSION); - ss << sporkId << nValue << nTimeSigned; - WriteCompactSize(ss, sig_compact_size); - for (size_t i = 0; i < sig_bytes_to_write; ++i) { - ss << static_cast(0xab); - } - return ss; -} - -// Distinguishes the pre-allocation size-gate throw from a downstream stream underflow. -bool IsLimitExceededFailure(const std::ios_base::failure& e) -{ - return std::string_view{e.what()}.find("Vector length limit exceeded") != std::string_view::npos; -} -} // namespace - -// The vulnerability report's exact trigger: a canonical CompactSize(MAX_SIZE) -// with no signature bytes. On the vulnerable implementation this reaches the -// stock vector Unserialize path, which resizes the destination in chunks -// (allocating before it discovers there is no data) and then throws a generic -// stream underflow. On the fix, LimitedVectorFormatter rejects the count -// before any allocation via ios_base::failure. -BOOST_AUTO_TEST_CASE(spork_signature_max_size_rejected_before_allocation) -{ - CDataStream ss = MakeSporkStream(/*sporkId=*/10001, 0, 0, - /*sig_compact_size=*/MAX_SIZE, - /*sig_bytes_to_write=*/0); - CSporkMessage spork; - BOOST_CHECK_EXCEPTION(ss >> spork, std::ios_base::failure, IsLimitExceededFailure); -} - -// A +1 boundary with a full payload — the bound must short-circuit before -// element decoding even when every promised byte is on the wire. -BOOST_AUTO_TEST_CASE(spork_signature_oversized_rejected) -{ - CDataStream ss = MakeSporkStream(/*sporkId=*/10001, 0, 0, - /*sig_compact_size=*/CPubKey::COMPACT_SIGNATURE_SIZE + 1, - /*sig_bytes_to_write=*/CPubKey::COMPACT_SIGNATURE_SIZE + 1); - CSporkMessage spork; - BOOST_CHECK_EXCEPTION(ss >> spork, std::ios_base::failure, IsLimitExceededFailure); -} - -// A valid 65-byte signature vector round-trips through serialize/unserialize -// with the wire bytes unchanged — the fix must not have moved the wire format -// for legit messages. -BOOST_AUTO_TEST_CASE(spork_signature_exact_size_roundtrip) -{ - CDataStream ss = MakeSporkStream(/*sporkId=*/10001, /*nValue=*/123456789, - /*nTimeSigned=*/1'700'000'000, - /*sig_compact_size=*/CPubKey::COMPACT_SIGNATURE_SIZE, - /*sig_bytes_to_write=*/CPubKey::COMPACT_SIGNATURE_SIZE); - const std::vector wire(ss.begin(), ss.end()); - - CSporkMessage decoded; - BOOST_REQUIRE_NO_THROW(ss >> decoded); - BOOST_CHECK_EQUAL(ss.size(), 0U); - BOOST_CHECK_EQUAL(decoded.nSporkID, static_cast(10001)); - BOOST_CHECK_EQUAL(decoded.nValue, 123456789); - BOOST_CHECK_EQUAL(decoded.nTimeSigned, 1'700'000'000); - - CDataStream re(SER_NETWORK, PROTOCOL_VERSION); - re << decoded; - BOOST_REQUIRE_EQUAL(re.size(), wire.size()); - BOOST_CHECK(std::equal(re.begin(), re.end(), wire.begin())); -} - -BOOST_AUTO_TEST_SUITE_END()