fix: bound DYNBITSET allocation against remaining stream size - #7532
fix: bound DYNBITSET allocation against remaining stream size#7532PastaPastaPasta wants to merge 2 commits into
Conversation
ReadFixedBitSet allocated from a wire-declared CompactSize with no bound beyond ReadCompactSize's 33,554,432 cap. Roughly five bytes on the wire (a CompactSize claiming millions of bits and no payload) therefore forced a std::vector<bool> resize plus a byte buffer totalling several MiB, all of which was only abandoned when the subsequent short read threw. MAX_PROTOCOL_MESSAGE_LENGTH does not help, because the attack uses an undersized message. Bound the declared length against the bytes actually remaining in the stream before allocating. A well-formed message always carries exactly the required bytes, so this rejects only claims that could never have been satisfied. This covers every DYNBITSET caller, including CFinalCommitment::signers and validMembers, which are reachable from an unauthenticated QFCOMMITMENT.
|
✅ Final review complete — no blockers (commit 8825c6b) |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
CheckDKGMessageStructure runs on the message-handler thread before a pushed DKG message is retained, and it validated by deserializing a copy of the payload. CBLSWrapper::Unserialize is eager: it decompresses each point, re-serializes it for the malleability check, and retries with the opposite scheme on mismatch. For llmq_400_85 a QCONTRIB carries 340 G1 points, so roughly 70 KB of wire bought hundreds of curve operations before the per-peer pending-message quota could drop the message. Replace the deserialization with a byte walk that reads compact-sizes and skips fixed-width fields, checking only bounds derived from quorum params. BLS decompression, member-list lookup and signature verification all stay on the DKG worker thread, behind the quota. The scan mirrors each message's wire layout with no type-level link to the serializers, so the acceptance tests build every message by serializing a real object with operator<< rather than hand-assembling bytes. A serializer change that the scan does not track will fail them, which matters because a message honest peers accept but this node rejects would partition it from the DKG. The DYNBITSET allocation bound that was previously bundled here is now in dashpay#7532.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/serialize_bitset_tests.cpp`:
- Around line 21-26: Strengthen RejectedBeforeAllocating to accept only the
guard-specific “exceeds remaining” error, not the generic ReadFixedBitSet
function name. In the affected regression assertions, compare each destination’s
size exactly against its pre-deserialization size using equality checks, rather
than merely asserting it differs from the claimed size.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93ef6fcc-11e9-4716-bb44-6ad96fdea032
📒 Files selected for processing (3)
src/Makefile.test.includesrc/serialize.hsrc/test/serialize_bitset_tests.cpp
| //! 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; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the regression assert the guarded failure path.
RejectedBeforeAllocating accepts any exception containing ReadFixedBitSet. A later short-read path can contain that function name and still pass. BOOST_CHECK_NE also 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
- return what.find("exceeds remaining") != std::string::npos ||
- what.find("ReadFixedBitSet") != std::string::npos;
+ return what.find("declared size exceeds remaining bytes") != std::string::npos;
...
+ const auto initial_bits_size = bits.size();
...
- BOOST_CHECK_NE(bits.size(), kClaimedBits);
+ BOOST_CHECK_EQUAL(bits.size(), initial_bits_size);
...
+ const auto initial_signers_size = qc.signers.size();
...
- BOOST_CHECK_NE(qc.signers.size(), 1'000'000u);
+ BOOST_CHECK_EQUAL(qc.signers.size(), initial_signers_size);Also applies to: 44-55, 88-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/serialize_bitset_tests.cpp` around lines 21 - 26, Strengthen
RejectedBeforeAllocating to accept only the guard-specific “exceeds remaining”
error, not the generic ReadFixedBitSet function name. In the affected regression
assertions, compare each destination’s size exactly against its
pre-deserialization size using equality checks, rather than merely asserting it
differs from the claimed size.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The core fix in ReadFixedBitSet correctly bounds the wire-declared bit count against bytes actually remaining in the stream before any vector resize or byte-buffer allocation, closing a real memory-amplification vector reachable via unauthenticated QFCOMMITMENT and every other DYNBITSET/AUTOBITSET caller. The requires{s.size();} guard properly scopes the check to network-deserialization streams without touching file-backed AutoFile/CBufferedFile paths, and the new tests demonstrate the rejection for both a raw DYNBITSET and a full CFinalCommitment. Two non-blocking test/lint-coverage issues remain: the regression test's failure-matching helper and size assertions are looser than the property being proven, and the new test file is missing from non-backported.txt.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/general=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); verifier=claude/final-verifier=claude-sonnet-5(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/serialize_bitset_tests.cpp`:
- [SUGGESTION] src/test/serialize_bitset_tests.cpp:21-26: 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.
In `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:68: New Dash-specific test file missing from non-backported.txt
src/test/serialize_bitset_tests.cpp is a brand-new file testing Dash-specific serialization (DYNBITSET, llmq::CFinalCommitment) that did not originate upstream. non-backported.txt tracks files that lint-cppcheck-dash.py uses to apply extra Dash-specific static analysis; the file isn't listed, so it's currently skipped by that lint pass. Doesn't break the build, but it's a one-line omission worth fixing before merge.
| //! 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; | ||
| } |
There was a problem hiding this comment.
🟡 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.
| //! 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; | |
| } | |
| bool RejectedBeforeAllocating(const std::string& what) | |
| { | |
| return what.find("declared size exceeds remaining bytes") != std::string::npos; | |
| } |
source: ['claude', 'codex']
…ping it The if constexpr guard failed open silently: a stream without size() compiled the bound away with no diagnostic, leaving the unbounded allocation in place. Constraining the template turns that into a build error, so the invariant is checked on every build rather than by inspection.
|
This pull request has conflicts, please rebase. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The latest delta correctly replaces the fail-open size() detection with a compile-time SizedStream requirement, and no new correctness issue was introduced at head 8825c6b. Two carried-forward suggestions remain unchanged: the regression tests do not precisely establish guard-specific rejection before destination mutation, and the new Dash-specific test file is still absent from non-backported.txt. The proposed commit squash is not retained because the two commits are coherent, independently useful changes and the project permits review follow-up commits.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:68: New Dash-specific test file missing from non-backported.txt
src/test/serialize_bitset_tests.cpp is a new Dash-originated source file, but no pattern in this list matches it. test/lint/lint-cppcheck-dash.py passes only files selected through these patterns to the additional Dash-specific cppcheck run, so the new test is currently omitted. Add the file alongside the other Dash-specific unit tests.
2 participants
Issue being fixed or feature implemented
ReadFixedBitSetallocated from a wire-declaredCompactSizewith no bound beyondReadCompactSize's 33,554,432 cap. Roughly five bytes on the wire — aCompactSizeclaiming millions of bits, with no payload following — therefore forced astd::vector<bool>resize plus a byte buffer totalling several MiB, all of which was only abandoned when the subsequent short read threw.MAX_PROTOCOL_MESSAGE_LENGTHdoes not help here, because the attack uses an undersized message.The primitive is reachable from an unauthenticated
QFCOMMITMENTviaCFinalCommitment'ssignersandvalidMembersbitsets, and applies to every otherDYNBITSETcaller as well.This was split out of #7523, where it was bundled with a much larger and more contentious DKG-intake change. The bound stands on its own, so it is offered separately for independent review.
What was done?
Bound the declared length against the bytes actually remaining in the stream, before allocating anything. A well-formed message always carries exactly the required bytes, so this rejects only claims that could never have been satisfied.
The guard is applied only for streams that expose
size(), which covers the network deserialization path; file-backed streams keep their existing behaviour.Note on what is deliberately not included: an earlier revision also checked
nbytes > MAX_SIZE. That check is dead code —sizecomes fromReadCompactSize, which already caps at 33,554,432, so(size + 7) / 8can never exceed 4 MiB and the condition is always false.How Has This Been Tested?
src/test/serialize_bitset_tests.cppcovers the amplification case (a declared length far exceeding the remaining stream must be rejected before allocating, not by a short read afterwards), a legitimate LLMQ-sized bitset round-tripping unchanged, and the same rejection through a fullCFinalCommitmentdeserialization.Built locally and
test_dash --run_test=serialize_bitset_testspasses (3 cases). Full validation is delegated to CI.Breaking Changes
None. Only messages declaring a bit count that the message could not possibly contain are affected, and those were never valid.
Checklist: