Skip to content

fix: bound DomainPort string deserialization and stop swallowing deser failures - #59

Draft
PastaPastaPasta wants to merge 9 commits into
developfrom
sec/v048
Draft

fix: bound DomainPort string deserialization and stop swallowing deser failures#59
PastaPastaPasta wants to merge 9 commits into
developfrom
sec/v048

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Audit finding V048. Fixes an unauthenticated remote memory-amplification DoS in ExtNetInfo deserialization.

Issue

DomainPort::SERIALIZE_METHODS read m_addr as an unbounded std::string. An attacker could declare a multi-megabyte CompactSize length from a few bytes on the wire, forcing allocate-and-zero before any validation ran.

Worse, NetInfoEntry::Unserialize caught ios_base::failure on the Domain branch and merely called Clear(). Swallowing that left the stream position unmoved (or advanced only by the CompactSize) while the enclosing vector loop kept iterating — so a short body amplified a handful of attacker bytes into repeated allocations.

ValidateDomain already enforced the RFC 1035 253-byte ceiling, but only after decode, which is too late to prevent the allocation.

Fix

  • Move DOMAIN_MAX_LEN (253) from netinfo.cpp to netinfo.h and apply it at the serialization layer via LIMITED_STRING, so an oversized claim is rejected before allocation. ValidateDomain still enforces the same ceiling after decode.
  • Stop swallowing ios_base::failure on the Domain branch; let it propagate so the entire ProTx payload is rejected rather than the loop continuing over a desynchronized stream.

Test

test: prove DomainPort unbounded string deserialization DoS precedes the fix and fails without it.

Review notes

  • Scope is confined to src/evo/netinfo.{cpp,h} plus its unit test.
  • The removed catch is the behavioral change most worth a second look: callers now see a throw where they previously got a silently-cleared entry. Reviewed as correct — a truncated domain body is not a recoverable condition, and the sibling Service branch keeps its catch because a failed CService deser does not desynchronize the stream the same way.
  • Based on dashpay/dash develop @ 6d04c60ef36. Not rebase-tested against any newer tip; full functional suite not run.

🤖 Generated with Claude Code

thepastaclaw and others added 2 commits June 27, 2026 10:26
@thepastaclaw @claude
fix: gracefully handle HD chain type mismatch on wallet load
8fbae2b 
Replace the assert in ReadKeyValue that compared the DB record's key
type against CHDChain::IsCrypted() with a wallet load error. A wallet
whose record type and payload disagree is corrupt, not a programmer
bug, and should not abort the process.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@thepastaclaw
fix: prune ChainLock seen cache with hysteresis
64cf27d 
dashpay#7424 bounded ChainlockHandler::seenChainLocks by switching it to
an unordered_limitedmap constructed as `seenChainLocks{MAX_SEEN_CHAINLOCKS}`.
unordered_limitedmap defaults nPruneAfterSize to nMaxSize, so prune() runs on
every unique insertion past 1024 entries: it allocates a vector of all 1025
iterators, std::sort()s it in full, and erases a single element, all while
ChainlockHandler::cs is held.

ProcessNewChainLock records the CLSIG hash before both the stale-height early
return and VerifyChainLock, and stale-height CLSIGs carry no misbehavior
penalty, so a peer could turn a stream of unique, unverified CLSIG hashes into
a stream of O(n log n) sorts under cs -- CPU amplification on the ChainLocks
relay path.

Construct the cache with an explicit prune-after size of twice the retained
size instead. The map now grows to 2048 entries and is pruned back to 1024 in
one batch, amortising each sort over 1024 evictions rather than one, at the
cost of a larger transient cache. The 2x ratio matches the default already used
by unordered_lru_cache. Note that pruning less often also reduces the chance
that an entry is evicted by the same prune that inserted it, since entries
inserted within one second share a timestamp and tie under prune()'s
comparator.

Stale CLSIG duplicate suppression is unchanged: hashes are still recorded
before the stale-height return, and the 24h time-based Cleanup() is unaffected.

Naming now distinguishes the retained size from the temporary prune threshold
(MAX_SEEN_CHAINLOCKS -> SEEN_CHAINLOCKS_RETAINED_SIZE plus a new
SEEN_CHAINLOCKS_PRUNE_AFTER_SIZE), and the constructor's defaulted
nPruneAfterSize is documented as the footgun it is.

Tests: two new limitedmap cases prove the generic container semantics -- no
prune at retained max+1, growth bounded by the trigger, and a batch prune back
to the retained size on crossing it -- plus the default-threshold behavior.
The ChainLock handler test no longer asserts a strict instantaneous 1024 cap;
it now verifies how the handler wires the cache up and that growth past the
retained size is retained until the trigger is crossed. Verified the handler
test fails when the fix is reverted.
@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2cb7051f-e892-4ee5-bfc4-879f33467759

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sec/v048

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

thepastaclaw and others added 7 commits July 29, 2026 15:14
@thepastaclaw
ci: reject @mentions in pull request descriptions
9bc7ed5 
Flag GitHub @username mentions in PR bodies so they are not copied into
merge commits and re-notify people on merge, rebase, or backport.

Use a dedicated Python helper with unit tests, preserve the existing
required-check name, and allow complete email addresses.
@PastaPastaPasta
Merge dashpay#7482: fix(net): amortize ChainLock seen-cache pruning
dcc8a7b 
64cf27d fix: prune ChainLock seen cache with hysteresis (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented

  Dash dashpay#7424 bounded `ChainlockHandler::seenChainLocks` with `unordered_limitedmap`, but the single-argument constructor makes the prune trigger equal to the 1,024-entry retained size. After the cache fills, every unique insertion creates and sorts a 1,025-element iterator vector and removes one entry while holding `ChainlockHandler::cs`.

  `ProcessNewChainLock()` records the hash before its stale-height return and before signature verification. A peer can therefore submit unlimited unique stale-height CLSIGs, receive no misbehavior penalty, and trigger the full sort on every message.

  ## What was done?

  - Constructed `seenChainLocks` with a 1,024-entry retained size and a 2,048-entry prune trigger, so one sort removes a batch of 1,025 entries instead of one.
  - Renamed the constants and test accessors to distinguish the retained size from the temporary prune threshold.
  - Documented `unordered_limitedmap`'s default prune behavior and exposed its configured threshold for focused tests.
  - Updated the generic limited-map and ChainLock tests to verify growth through the hysteresis window, batch pruning back to the retained size, stale CLSIG duplicate suppression, and best-ChainLock lookup after eviction.

  Stale CLSIG hashes are still remembered before the early return, and the existing 24-hour cleanup behavior is unchanged.

  ## How Has This Been Tested?

  Tested locally on macOS against current `develop`:

  - `./autogen.sh` and `./configure` with the existing `aarch64-apple-darwin` depends prefix
  - `make -C src test/test_dash`
  - `./src/test/test_dash --run_test=limitedmap_tests,llmq_chainlock_tests`
  - `./src/test/test_dash --run_test=limitedmap_tests,llmq_chainlock_tests,llmq_signing_tests,llmq_dkg_tests,evo_islock_tests`
  - `git diff --check upstream/develop..HEAD`
  - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py`
  - `test/lint/lint-circular-dependencies.py`
  - `test/lint/lint-include-guards.py`
  - Scoped clang-format verification for the changed ChainLock files
  - Negative control: restoring the single-argument constructor makes the new handler regression test fail; restoring the fix passes it
  - Exact-head pre-PR code-review gate: `ship`, zero findings

  ## 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:
  PastaPastaPasta:
    utACK 64cf27d

Tree-SHA512: 90f014827163c1e898d39b90d7c88bd238d30d4cf9dfac15717dc4f260c5e1a1049f626895bdcf55ba0c74625535001fa602ba9403e11af79eeec375159328b1
@thepastaclaw
ci: drop unit tests for PR description mention check
9cde0fa 
Reviewers asked to keep this check simple; the helper itself is
enough without a dedicated CI unit-test file.
@PastaPastaPasta
Merge dashpay#7383: fix: reject mismatched HD chain wallet records
62325fe 
8fbae2b fix: gracefully handle HD chain type mismatch on wallet load (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented
  Malformed wallet database records can currently trip a process assertion while
  loading HD chain state. A wallet database whose HD chain key type disagrees with
  the serialized chain encryption flag is corrupt input and should be reported as
  a wallet load error instead of aborting the process.

  ## What was done?
  Changed `ReadKeyValue` to validate `HDCHAIN` / `CRYPTED_HDCHAIN` records with a
  normal conditional check. When the key type and serialized `CHDChain` state do
  not match, wallet loading now returns a descriptive database error.

  Added focused unit coverage for both mismatch directions.

  ## How Has This Been Tested?
  Ran:

  ```bash
  git diff --check upstream/develop...HEAD
  src/test/test_dash --run_test=walletdb_hdchain_tests
  code-review dashpay/dash upstream/develop e85b88f74882b2ecf437cbce56441177e1955e11 "Reject mismatched HDCHAIN/CRYPTED_HDCHAIN wallet records gracefully instead of asserting during wallet load"
  ```

  The pre-PR review gate returned `ship`.

  ## Breaking Changes
  None.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] 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:
  thepastaclaw:
    @PastaPastaPasta Ready for merge consideration on head `8fbae2b3`.

- @UdjinM6 utACK'd this exact head on 2026-07-21.
- Branch is mergeable against `develop`; PastaClaw gate and CodeRabbit are clean on this SHA.
- The only remaining red jobs are the known unrelated functional-test flakes (`linux64-test` / `linux64_tsan-test`), already scoped as out-of-diff in earlier comments — no branch change needed for those.

Happy to handle any follow-up if you want a different landing path.
  UdjinM6:
    Makes sense, utACK 8fbae2b

Tree-SHA512: fdccad171b07c0878e5185a9acd09d8549d982bc637a4c025daa0cda86e37575f4996359b7402b135253a3af26bbd33c7a764febd528441203a13151ba13231e
@PastaPastaPasta
Merge dashpay#7496: ci: reject @mentions in pull request descriptions
35e9dac 
9cde0fa ci: drop unit tests for PR description mention check (PastaClaw)
9bc7ed5 ci: reject @mentions in pull request descriptions (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented

  PR descriptions that include GitHub username mentions get copied into merge commits. After that, those people get notified again whenever the PR is merged, rebased, or backported. See dashpay#7493 for the motivating case ("Thanks knst" / similar mentions in a release PR description).

  This change makes CI fail when a PR description contains those mentions.

  ## What was done?

  Extended the existing semantic PR title workflow (`.github/workflows/semantic-pull-request.yml`) so it also validates the PR description:

  - Renamed the workflow/job to cover title and description
  - Kept the existing conventional-commit title check (`amannn/action-semantic-pull-request@v6`) unchanged
  - Added a step that reads `github.event.pull_request.body` via env on `pull_request_target` (`opened` / `edited` / `synchronize`), writes it to a file, and fails on GitHub-style `@username` mentions
  - Email addresses and empty descriptions are allowed
  - Failure message explains the merge/rebase/backport notification problem

  ## How Has This Been Tested?

  - YAML parses cleanly
  - `git diff --check upstream/develop...HEAD` clean
  - Local regex checks (same pattern as the workflow step):
    - `Thanks @knst.` → fail
    - `@PastaPastaPasta please look` → fail
    - `contact dev@example.com` → pass
    - `no mentions here` / empty body / bare `@` / `@@` diff markers → pass

  ## Breaking Changes

  None. Existing PRs with `@username` mentions in their description will fail this check on the next `edited` / `synchronize` event until the mentions are removed.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [ ] 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)_

Top commit has no ACKs.

Tree-SHA512: 0fa3cb00d87f59e162add5ad62d0fd7aba2373dd4e4c01e8f4745b8e4e951d4830878ca9b7760f133fe6a5915b49ebef9967e46cc05620417a78e3d49ca26429
@PastaPastaPasta
test: prove DomainPort unbounded string deserialization DoS
e18749c 
Add regression coverage: DomainPort accepts attacker-chosen string lengths up to MAX_SIZE, and NetInfoEntry swallows ios_base::failure so ExtNetInfo vectors can force repeated large allocate-and-zero work from a small payload. These cases expect rejection (throw) and fail on the unfixed tree.
@PastaPastaPasta
fix: bound DomainPort string and stop swallowing deser failures
d962ec9 
DomainPort serialized m_addr as a bare std::string, so CompactSize claims up to MAX_SIZE (32 MiB) allocated before any body was read. NetInfoEntry::Unserialize caught ios_base::failure and Clear()ed, letting ExtNetInfo vector loops retry from an unadvanced stream position and amplify a ~10 KB ProTx payload into tens of GB of allocate-and-zero under cs_main.

Cap the domain with LIMITED_STRING(..., DOMAIN_MAX_LEN=253) at the serialization layer (matching ValidateDomain and RFC 1035 FQDN limits; hard-fork-safe because valid domains were already rejected above 253) and rethrow DomainPort deser failures so the whole ProTx payload fails instead of looping.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PastaPastaPasta @thepastaclaw