fix(net): bound bloom message vectors before allocation#7444
Conversation
A peer-controlled CBloomFilter and the FILTERADD data element were decoded in full before their size was checked. The length is a CompactSize up to MAX_SIZE, so a raw count with the bytes omitted forced a large speculative allocation and then threw std::ios_base::failure into net_processing's outer message-processing catch, which drops without punishing the peer and so allowed indefinite replay. Bound the wire count before any element is decoded using LIMITED_VECTOR: CBloomFilter::vData at MAX_BLOOM_FILTER_SIZE in its serialization method and the FILTERADD data element at MAX_SCRIPT_ELEMENT_SIZE at the call site. The wire format is unchanged, so IsWithinSizeConstraints() still guards the exact boundary and nHashFuncs. At FILTERLOAD, FILTERADD, and the MNGOVERNANCESYNC consumer of the shared type, catch the local std::ios_base::failure and apply the existing 100-point punishment; the govsync catch is scoped to the filter read so unrelated nProp handling is unchanged. Because CBloomFilter is now globally bounded, getmerkleblocks prechecks the leading vData count without consuming the stream: a fully present oversized filter keeps its historical RPC_INVALID_PARAMETER response, a truncated oversized declaration reproduces the original DataStream end-of-data error, and malformed, noncanonical, and above-MAX_SIZE prefixes fall through to normal deserialization unchanged. Add exact-trigger regressions to p2p_filter.py and p2p_govsync_bloom.py (via msg_generic) that declare a CompactSize(MAX_SIZE) length with the bytes omitted and assert the peer is punished; existing valid-boundary coverage is retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Review complete (commit 5b6ac38) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughBloom-filter deserialization now bounds vector allocation through Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR closes a pre-allocation DoS gap by switching CBloomFilter's vData, FILTERADD's data, and MNGOVERNANCESYNC's filter deserialization to bounded reads, with local try/catch blocks applying the Misbehaving(100) punishment that the outer message-processing catch (net_processing.cpp:5761-5772) silently skips. All boundary math was independently verified: SpanReader::size() correctly reflects post-read remaining bytes (confirmed via read()'s subspan advance), the LimitedVectorFormatter throws before allocation, and the getmerkleblocks RPC precheck's OVERSIZED_COMPLETE/OVERSIZED_TRUNCATED logic exactly reproduces prior behavior. The msg_generic slots fix is a correct necessary prerequisite for the new P2P tests. Both reviewer lanes agree the change is sound; the only outstanding item is a missing automated regression test for the RPC's non-trivial byte-level precheck.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 3): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); sonnet5/dash-core-commit-history=claude-sonnet-5(completed); verifier=verifier-sonnet5-7444-1783722240=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🟡 1 suggestion(s)
🤖 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/rpc/blockchain.cpp`:
- [SUGGESTION] src/rpc/blockchain.cpp:855-882: getmerkleblocks oversized/truncated-filter precheck has no automated test
This block hand-rolls CompactSize parsing via a manual SpanReader to distinguish OVERSIZED_COMPLETE from OVERSIZED_TRUNCATED and preserve the exact pre-PR error for each case. It's the most intricate addition in the PR — byte-offset arithmetic, trailer-size accounting (FILTER_TRAILER_SIZE), and manual exception construction — yet per the PR's own validation notes it was only exercised with a manual RPC matrix. There's no functional test file for getmerkleblocks (test_runner.py still has a 'TODO: implement functional tests for getmerkleblocks') and no C++ unit test covers this path. A future refactor of CBloomFilter's serialization or this precheck could silently change RPC error behavior (e.g. RPC_INVALID_PARAMETER vs a raw ios_base::failure) without CI catching it.
getmerkleblocks hand-parses the leading CompactSize vData count so that, now
that CBloomFilter deserialization is bounded, an oversized filter still
reproduces the historical RPC errors instead of a generic length-limit
failure. That byte-level precheck (distinguishing a fully present oversized
filter from a truncated one, including the trailing fixed-field boundary) had
no automated coverage.
Add rpc_getmerkleblocks.py, which asserts:
- a fully present oversized filter -> RPC_INVALID_PARAMETER
("Filter is not within size constraints");
- an oversized declaration with the vData bytes omitted, or with the trailer
short by one byte, -> the original DataStream end-of-data RPC_MISC_ERROR;
- a well-formed in-bounds filter falls through and is handled normally.
Register the test in BASE_SCRIPTS and drop the now-satisfied getmerkleblocks
RPC-coverage TODO, since the passing filter call exercises the command.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest test-only commit resolves the sole prior finding by adding functional coverage for the complete-oversized, truncated-body, one-byte-short trailer, and valid-filter paths. The assertions match the current C++ boundary arithmetic and RPC error mapping; Python lint passes and the two-commit history is clean.
Prior finding reconciliation: FIXED — “getmerkleblocks oversized/truncated-filter precheck has no automated test” is covered by test/functional/rpc_getmerkleblocks.py.
Carried-forward prior findings: None.
New findings in latest delta: None.
Source: reviewers codex/general=gpt-5.6-sol (parseable output, nonzero ACP wrapper exit); sonnet5/general=claude-sonnet-5 (completed); codex/dash-core-commit-history=gpt-5.6-sol (parseable output, nonzero ACP wrapper exit); sonnet5/dash-core-commit-history=claude-sonnet-5 (completed); verifier sonnet5=claude-sonnet-5 (completed); orchestrator openai/gpt-5.6-sol reasoning=high (orchestration-only, not reviewer/verifier). Experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 2.
| } catch (const std::ios_base::failure& e) { | ||
| // An oversized filter now throws pre-allocation; punish here instead of the outer catch. | ||
| m_peer_manager->PeerMisbehaving(peer.GetId(), 100, strprintf("misformatted govsync bloom filter. peer=%d error=%s", peer.GetId(), e.what())); | ||
| return; |
There was a problem hiding this comment.
nit: why won't re-throw exception here? instead return; ?
So this exception will be caught in the call-stack higher and logged:
LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(msg.m_type), msg.m_message_size, ....
There was a problem hiding this comment.
PeerMisbehaving(..., e.what()) already logs the exception detail through Misbehaving, so rethrowing would produce a second outer-catch log for the same failure. Returning here is intentional and matches the filterload/filteradd deserialization handlers added by this PR.
| @@ -0,0 +1,90 @@ | |||
| #!/usr/bin/env python3 | |||
| # Copyright (c) 2024 The Dash Core developers | |||
There was a problem hiding this comment.
@thepastaclaw for new files specify 2026 year.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Current-head cumulative review found no code-correctness, security, or test-coverage issues in the bloom-vector bounds or their regression coverage. Prior reconciliation: the review at 862e7b0 had no findings, so there are no carried-forward prior findings. New finding in the latest delta: the one-line copyright-year correction is a standalone fixup commit that should be squashed into the immediately preceding test commit. Additional cumulative findings: none.
Source: Sol reviewer (gpt-5.6-sol), Sonnet reviewer (claude-sonnet-5), and Opus reviewer (claude-opus-4-8) for general and dash-core-commit-history lanes; final verifier: claude-sonnet-5. Orchestrator: openai/gpt-5.6-sol, reasoning high, orchestration-only (not reviewer/verifier evidence).
🟡 1 suggestion(s)
🤖 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/functional/rpc_getmerkleblocks.py`:
- [SUGGESTION] test/functional/rpc_getmerkleblocks.py:1-2: Squash the copyright-year fixup into the commit that introduced the file
Commit 5b6ac384d7f ("test: fix getmerkleblocks copyright year") only changes the header of test/functional/rpc_getmerkleblocks.py from '# Copyright (c) 2024' to '# Copyright (c) 2026'. That file was created one commit earlier in this same, unmerged PR by 862e7b0e890 ("test: cover getmerkleblocks oversized bloom-filter prechecks") with the wrong year — it has never shipped to develop. Since this only corrects a mistake made earlier in the same stack rather than being an independently reviewable change, and Dash merges commits without squashing (so this becomes permanent git log/blame/bisect noise on a file born one commit earlier), fold it into 862e7b0e890 via `git rebase -i --autosquash` (or a manual fixup) so the file lands with the correct copyright year in a single commit. Verified directly: `git show 862e7b0e890` shows the file added with '(c) 2024', and `git show 5b6ac384d7f` shows the sole change is the year correction, with 5b6ac384d7f not yet reachable from develop.
| #!/usr/bin/env python3 | ||
| # Copyright (c) 2026 The Dash Core developers |
There was a problem hiding this comment.
🟡 Suggestion: Squash the copyright-year fixup into the commit that introduced the file
Commit 5b6ac38 ("test: fix getmerkleblocks copyright year") only changes the header of test/functional/rpc_getmerkleblocks.py from '# Copyright (c) 2024' to '# Copyright (c) 2026'. That file was created one commit earlier in this same, unmerged PR by 862e7b0 ("test: cover getmerkleblocks oversized bloom-filter prechecks") with the wrong year — it has never shipped to develop. Since this only corrects a mistake made earlier in the same stack rather than being an independently reviewable change, and Dash merges commits without squashing (so this becomes permanent git log/blame/bisect noise on a file born one commit earlier), fold it into 862e7b0 via git rebase -i --autosquash (or a manual fixup) so the file lands with the correct copyright year in a single commit. Verified directly: git show 862e7b0e890 shows the file added with '(c) 2024', and git show 5b6ac384d7f shows the sole change is the year correction, with 5b6ac38 not yet reachable from develop.
source: ['codex-dash-core-commit-history', 'sonnet5-dash-core-commit-history', 'opus-dash-core-commit-history']
3 participants
Prevent peer-declared FILTERADD and CBloomFilter vector lengths from allocating before their command-specific limits are checked.
The shared CBloomFilter bound covers FILTERLOAD and governance sync. Each P2P entrypoint catches malformed bounded reads locally and applies the existing 100-point punishment, preventing replay through the outer message-processing catch. The getmerkleblocks RPC keeps its prior complete-oversized, truncated, noncanonical, and above-MAX_SIZE behavior.
Adds exact CompactSize(MAX_SIZE) regressions for FILTERADD, FILTERLOAD, and governance sync while retaining valid boundary coverage.
Validation: