-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: bound DYNBITSET allocation against remaining stream size #7532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,103 @@ | ||||||||||||||||||||||
| // 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 <llmq/commitment.h> | ||||||||||||||||||||||
| #include <llmq/params.h> | ||||||||||||||||||||||
| #include <serialize.h> | ||||||||||||||||||||||
| #include <streams.h> | ||||||||||||||||||||||
| #include <test/util/setup_common.h> | ||||||||||||||||||||||
| #include <uint256.h> | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| #include <boost/test/unit_test.hpp> | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| #include <ios> | ||||||||||||||||||||||
| #include <string> | ||||||||||||||||||||||
| #include <vector> | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| BOOST_FIXTURE_TEST_SUITE(serialize_bitset_tests, BasicTestingSetup) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| namespace { | ||||||||||||||||||||||
| //! The bound must reject before allocating, so a short read after the fact is not good enough. | ||||||||||||||||||||||
| bool RejectedBeforeAllocating(const std::string& what) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| return what.find("exceeds remaining") != std::string::npos || | ||||||||||||||||||||||
| what.find("ReadFixedBitSet") != std::string::npos; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+21
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Test assertions don't tightly prove rejection happens before mutation RejectedBeforeAllocating() matches on either "exceeds remaining" or the bare substring "ReadFixedBitSet", the latter of which would also match any future exception message from within that function (e.g. a reworded variant of the existing "Out-of-range bits set" check, if it were ever changed to include the function name) even if the new pre-allocation guard didn't fire. Separately, BOOST_CHECK_NE(bits.size(), kClaimedBits) and BOOST_CHECK_NE(qc.signers.size(), 1'000'000u) only prove the destination wasn't resized to the exact attacker-claimed value — they don't prove it wasn't mutated to some other size, which is the actual invariant the PR is trying to assert (rejection before destination mutation/allocation). Matching only the guard-specific message text and comparing against the pre-deserialization size with BOOST_CHECK_EQUAL would make this a tight regression test for the fix rather than one that happens to pass today. This was raised by CodeRabbit on this same head and remains open.
Suggested change
source: ['claude', 'codex'] |
||||||||||||||||||||||
| } // namespace | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * DYNBITSET must not allocate from an attacker-declared CompactSize when the remaining stream | ||||||||||||||||||||||
| * is far too small to hold the claimed bit payload. A handful of bytes claiming ~1e6 bits is | ||||||||||||||||||||||
| * the amplification primitive: ReadCompactSize permits up to 33,554,432, which would resize a | ||||||||||||||||||||||
| * std::vector<bool> to ~4 MiB and allocate another ~4 MiB byte buffer before the short read | ||||||||||||||||||||||
| * throws. The claim below is deliberately modest so the pre-fix path also stays safe on CI. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| BOOST_AUTO_TEST_CASE(dynbitset_rejects_oversized_declared_length) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| constexpr uint64_t kClaimedBits = 1'000'000; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| CDataStream s(SER_NETWORK, PROTOCOL_VERSION); | ||||||||||||||||||||||
| WriteCompactSize(s, kClaimedBits); | ||||||||||||||||||||||
| // No bit payload follows, so the remaining size is zero. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| std::vector<bool> bits; | ||||||||||||||||||||||
| std::string what; | ||||||||||||||||||||||
| bool threw = false; | ||||||||||||||||||||||
| try { | ||||||||||||||||||||||
| s >> DYNBITSET(bits); | ||||||||||||||||||||||
| } catch (const std::ios_base::failure& e) { | ||||||||||||||||||||||
| threw = true; | ||||||||||||||||||||||
| what = e.what(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| BOOST_CHECK_MESSAGE(threw, "DYNBITSET must reject a declared length that exceeds remaining bytes"); | ||||||||||||||||||||||
| BOOST_CHECK_NE(bits.size(), kClaimedBits); | ||||||||||||||||||||||
| BOOST_CHECK_MESSAGE(RejectedBeforeAllocating(what), "Expected a pre-allocation rejection, got: " + what); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** A legitimately sized DYNBITSET (LLMQ max 400) must still round-trip unchanged. */ | ||||||||||||||||||||||
| BOOST_AUTO_TEST_CASE(dynbitset_accepts_legitimate_llmq_size) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| constexpr size_t kSize = Consensus::MAX_LLMQ_SIZE; | ||||||||||||||||||||||
| std::vector<bool> original(kSize, false); | ||||||||||||||||||||||
| for (size_t i = 0; i < kSize; i += 3) { | ||||||||||||||||||||||
| original[i] = true; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| CDataStream s(SER_NETWORK, PROTOCOL_VERSION); | ||||||||||||||||||||||
| s << DYNBITSET(original); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| std::vector<bool> decoded; | ||||||||||||||||||||||
| s >> DYNBITSET(decoded); | ||||||||||||||||||||||
| BOOST_CHECK(decoded == original); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * The same primitive is reachable from an unauthenticated QFCOMMITMENT via CFinalCommitment's | ||||||||||||||||||||||
| * signers bitset, so cover the real message type too. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| BOOST_AUTO_TEST_CASE(qfinalcommitment_rejects_oversized_signers_bitset) | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| CDataStream s(SER_NETWORK, PROTOCOL_VERSION); | ||||||||||||||||||||||
| // nVersion (u16) | llmqType (u8) | quorumHash (32) | signers DYNBITSET | ... | ||||||||||||||||||||||
| s << static_cast<uint16_t>(llmq::CFinalCommitment::BASIC_BLS_NON_INDEXED_QUORUM_VERSION); | ||||||||||||||||||||||
| s << Consensus::LLMQType::LLMQ_400_85; | ||||||||||||||||||||||
| s << uint256::ONE; | ||||||||||||||||||||||
| WriteCompactSize(s, 1'000'000); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| llmq::CFinalCommitment qc; | ||||||||||||||||||||||
| std::string what; | ||||||||||||||||||||||
| bool threw = false; | ||||||||||||||||||||||
| try { | ||||||||||||||||||||||
| s >> qc; | ||||||||||||||||||||||
| } catch (const std::ios_base::failure& e) { | ||||||||||||||||||||||
| threw = true; | ||||||||||||||||||||||
| what = e.what(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| BOOST_CHECK(threw); | ||||||||||||||||||||||
| BOOST_CHECK_NE(qc.signers.size(), 1'000'000u); | ||||||||||||||||||||||
| BOOST_CHECK_MESSAGE(RejectedBeforeAllocating(what), | ||||||||||||||||||||||
| "Expected a pre-allocation rejection for CFinalCommitment, got: " + what); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| BOOST_AUTO_TEST_SUITE_END() | ||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the regression assert the guarded failure path.
RejectedBeforeAllocatingaccepts any exception containingReadFixedBitSet. A later short-read path can contain that function name and still pass.BOOST_CHECK_NEalso accepts any partial resize except the exact claimed size. Match the guard-specific error and compare each destination with its size before deserialization.Proposed test assertion fix
Also applies to: 44-55, 88-100
🤖 Prompt for AI Agents