Skip to content

fix(zebrad): key the mempool per-peer download cap on IpAddr - #11233

Open
natalieesk wants to merge 1 commit into
mempool_timeout_peer_slot_10684from
mempool_per_peer_ipaddr_10685
Open

fix(zebrad): key the mempool per-peer download cap on IpAddr#11233
natalieesk wants to merge 1 commit into
mempool_timeout_peer_slot_10684from
mempool_per_peer_ipaddr_10685

Conversation

@natalieesk

@natalieesk natalieesk commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #10685.

Warning

Blocked on #11229 (#10684). This PR is stacked on the mempool_timeout_peer_slot_10684 branch, not main, and must not merge before #11229. Re-keying the per-peer cap on IP is only safe once the verification-timeout path releases the per-peer slot (fixed in #10684); without it, a single timed-out transaction would permanently lock out the sending host's IP. Merge #11229 first — GitHub then auto-retargets this PR to main and its Closes #10685 link activates. See "Dependency" below.

  • zebrad/src/components/mempool/downloads.rs: re-key pending_per_peer from HashMap<SocketAddr, usize> to HashMap<IpAddr, usize>, using source.ip() at the cap check, the increment, and release_peer_slot.
  • CHANGELOG.md: Security entry.
  • Regression test in zebrad/src/components/mempool/tests/vector.rs.

Approach & Key Decisions

The per-peer download cap (MAX_INBOUND_CONCURRENCY_PER_PEER) was keyed on the full SocketAddr. The source is the peer's transient (IP, ephemeral port) (connected_addr.get_transient_addr(), explicitly not a permanent identifier), so two connections from one host landed in two separate 5-slot buckets and a single host could exceed the intended per-host bound. The sibling inbound-block path already keys on IpAddr (in_flight_ips); this brings the mempool path in line. cancel_handles still stores the Option<SocketAddr>; only its .ip() is used for the map, so metrics/logging/error paths are unaffected.

Dependency

This change must not ship without #10684 (#11229). The verification-timeout arm releases the per-peer slot only via #10684's fix. Without it, re-keying on IP turns that latent per-SocketAddr slot leak (harmless, since ephemeral-port keys are never reused) into a permanent per-IP lockout: each timed-out transaction would leave a residual +1 on the host's IP bucket, and after MAX_INBOUND_CONCURRENCY_PER_PEER cumulative timeouts the host is rejected from all further mempool downloads until a downloader reset. Stacking on #11229 guarantees the timeout path releases the slot, so the map invariant (an IP's count equals its live cancel_handles entries) holds. Once #11229 merges, this PR retargets to main.

Testing & Verification

per_peer_cap_is_keyed_on_ip_not_socket_addr (mempool/tests/vector.rs): queues MAX_INBOUND_CONCURRENCY_PER_PEER transactions from one IP:port, then queues one more from the same IP, different port and asserts it is rejected with FullQueue.

cargo test -p zebrad --lib -- components::mempool::tests::vector::per_peer_cap_is_keyed_on_ip_not_socket_addr components::mempool::tests::vector::verification_timeout_releases_peer_slot components::mempool::tests::vector::cancel_handles_drained_after_verification_timeout
test result: ok. 3 passed

cargo fmt and cargo clippy -p zebrad --lib --all-features are clean.

Risk & Impact

Low, confined to mempool inbound-download accounting; no consensus, state-format, RPC, or config change; no DB format bump. Tightens a per-host DoS control (a host can no longer multiply its budget by opening connections from many source ports).

Changelog

Added under ### Security: the mempool per-peer download cap is now keyed on IP.

AI Disclosure

Claude (Claude Code) wrote the fix and the regression test.

PR Checklist

The mempool inbound-download manager keyed its per-peer concurrency cap
(`MAX_INBOUND_CONCURRENCY_PER_PEER`) on the full `SocketAddr`. The `source` is a
transient `(IP, ephemeral port)`, so two connections from the same host landed
in two distinct `pending_per_peer` buckets, each with its own 5-slot budget, and
a single host could exceed the intended per-host bound. `get_transient_addr` is
explicitly not a permanent identifier.

Re-key `pending_per_peer` on `IpAddr` at the cap check, the increment, and
`release_peer_slot`, mirroring the inbound-block download path (`in_flight_ips`).
Adds a regression test asserting a sixth transaction from the same IP on a
different port is rejected once the IP holds `MAX_INBOUND_CONCURRENCY_PER_PEER`.
@v12-auditor

v12-auditor Bot commented Aug 10, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. V12 found eight issues worth reviewing.

Open the full results here.

FindingSeverityDetails
F-206052 🔵 Low
Cancelled tasks free host slots before exit

Downloads::cancel() removes each mined transaction's cancel_handles entry and immediately calls release_peer_slot, but it does not remove the corresponding JoinHandle from pending. The cancelled task remains resident until Tokio polls its biased cancellation branch and a later Downloads::poll_next() reaps the completed handle. On a tip-growth readiness cycle, the mempool drains completions before invoking cancel(), so the service's immediately following QueueFromPeer call observes the decremented per-IP count while the old tasks can still be present in the global pending set. A peer with five pending transactions that are included in the new block can therefore submit five unrelated candidates and temporarily have ten task records attributed to its IP despite MAX_INBOUND_CONCURRENCY_PER_PEER being five. Later completion cleanup does not double-release because cancel() already removed the handle, and the next drain eventually restores actual residency to the accounted level.

F-206053 🟡 Medium
IPv6 address rotation bypasses host cap

The new limiter assigns each full IpAddr an independent five-task budget, but neither the mempool nor the network's connection controls aggregate native IPv6 addresses by prefix. The default listener accepts IPv6, while accept-side and peer-set limits also compare complete /128 addresses, so a host that controls many addresses in one routed prefix can establish one connection per address without hitting the per-IP controls. After establishing 100 persistent connections, the host can advertise five unique nonexistent transaction IDs from each address; each address receives its own bucket and the resulting 500 requests fill MAX_INBOUND_CONCURRENCY. Transaction downloads are preferentially routed to the sole advertiser for each ID, allowing the peers to withhold responses and retain those slots until timeout while periodically replenishing them. Because the global queue check precedes the per-IP check, saturation rejects all subsequent honest gossip candidates with FullQueue, although source-less crawler and local paths remain available.

F-206054 🟠 High
Timeouts release slots before proof work ends

The downloader wraps the entire verification future in a 73-second timeout and releases both global and per-IP admission accounting when that wrapper expires. Before expiry, however, transaction verification can transfer an attacker-supplied Sapling bundle into the independent global tower-batch-control worker; that worker calls the batch service even when the request's response receiver has already been dropped. The Sapling service retains the bundle in its shared batch, and flushing moves it into a spawn_blocking validation job that is not cancelled when the original downloader future disappears. During verifier backlog, peers can therefore submit proof-heavy transactions, wait for their accounted wrapper tasks to time out, and immediately submit replacements while the previous batches remain queued or executing. Repeating this with enough source IPs overlaps successive waves of cryptographic work beyond the downloader's intended 500-global and five-per-IP task limits.

F-206055 🟠 High
Retries shed host attribution and bypass cap

When a peer-attributed transaction finishes verification after the chain tip has advanced, Downloads::poll_next() first removes its cancellation record and releases the source IP's slot. The mempool then requeues that same transaction with source = None, so the replacement skips both the per-IP cap check and the per-IP increment. The originating host can immediately refill all five charged slots while its earlier transaction remains globally pending for re-verification without attribution. This path is reachable by timing valid, verification-heavy transactions so their initial checks straddle a block arrival; chain-fork reset requeueing similarly serializes pending gossip without retaining its source. Global admission remains capped at 500, and the attacker must provide transactions that progress far enough to be retried, which bounds the resulting amplification.

F-206056 🔵 Low
Rejected connections can poison shared host bucket

A newly handshaken inbound connection starts its Connection task before the resulting client is screened by the peer set's per-IP duplicate limit. The listener's recent-IP gate counts inbound attempts only, so it can accept an inbound connection from an IP for which Zebra already has an outbound peer; after the handshake, the new connection can pipeline transaction advertisements before the peer set drops it as a duplicate. With the new IP-keyed accounting, five tasks admitted from that transient connection charge the same bucket used by the pre-existing honest peer at that IP and remain charged until task completion even after the duplicate connection is closed. An attacker sharing a NAT/CGNAT address with an honest peer can therefore spend the honest peer's entire bucket during this admission race, whereas the prior socket-address key kept their distinct ports separate. The default recent-IP window limits this to one inbound attempt per IP per 119 seconds, and the affected downloads time out, so the lockout is intermittent.

F-206057 🔵 Low
Proxy peers collapse into one latent bucket

ConnectedAddr::get_transient_addr() returns a local/proxy endpoint for both proxy variants rather than the remote peer's address. The variant documentation identifies the ephemeral port as the connection-distinguishing component, but the changed accounting discards that component and keys admission, increment, and release only on source.ip(). If peer proxy support is enabled, every outbound proxied connection using the same local interface and every inbound connection through the same proxy would therefore share one five-slot bucket, allowing one proxied peer to block gossip admission for all others. The proxy constructors are currently unused outside a handshake test and production constructs direct or isolated variants, so this defect is latent in the present tree. Existing TODOs indicate proxy support is contemplated, making the incompatible key semantics important to preserve before those paths become reachable.

F-206058 🔵 Low
Tracing exposes raw peer addresses

download_if_needed_and_verify is instrumented without excluding its source: Option<SocketAddr> argument, so tracing records the raw peer socket address through SocketAddr's Debug implementation. Zebra otherwise wraps peer addresses in PeerSocketAddr, whose Debug and Display implementations deliberately redact the IP. This function keeps the source as a bare socket address for accounting and converts it to the privacy-preserving type only for later misbehavior reporting. When debug or trace logging renders events inside the span, the raw external peer IP and port can therefore appear in logs despite the project's address-redaction abstraction. The issue predates the IP-keying hunk, but it exposes the same untrusted address now used as the host key.

F-206060 Info
Regression misses key independence and release

The new regression test fills the budget from one address and verifies rejection from the same IP on another port, but it never checks that a different IP retains an independent budget. A mistaken future implementation using a constant or global key would satisfy this test and every existing single-source cap test while reducing all peer-sourced mempool admission to five tasks node-wide. The test also never drives Downloads to completion, so it does not cover the changed .ip() logic in release_peer_slot. The adjacent timeout-release regression uses only one SocketAddr, leaving the stacked change's key safety property untested: a timeout charged on port A must release the shared IP bucket so port B can subsequently queue. Consequently, both over-collapsing distinct hosts and leaving a same-IP residual count can regress without these focused tests failing.

And two more auto-invalidated findings.

Analyzed one file, diff 31e555e...3aeaa2f.

@mergify

mergify Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🟠 1 of 1 protections blocking · waiting on 🕒 schedule

Protection Waiting on
🟠 ❄️ 6.3.0 release [Scheduled Freeze] 🕒 schedule

🟠 ❄️ 6.3.0 release [Scheduled Freeze]

Waiting for

  • current-datetime < 2026-08-10T14:25:11[America/Sao_Paulo]
This freeze has no end date and must be removed manually.

A freeze on the repository is scheduled for the following reason: 6.3.0 release

  • current-datetime < 2026-08-10T14:25:11[America/Sao_Paulo]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-mempool Area: Memory pool transactions A-network Area: Network protocol updates or fixes C-audit Category: Issues arising from audit findings C-bug Category: This is a bug C-security Category: Security issues S-blocked Status: Blocked on other tasks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant