fix(network): canonicalize IPv4-mapped inbound peer addresses for bans and per-IP limits#238
Merged
Merged
Conversation
|
Note Complete: Audit complete. V12 did not find any issues that need review. Open the full results here. Analyzed two files, diff |
evan-forbes
force-pushed
the
fix/dualstack-canonicalize-inbound
branch
from
July 20, 2026 19:54
0ae4355 to
bcf91c0
Compare
…s and per-IP limits On a dual-stack listener (`[::]` with `IPV6_V6ONLY=false`), Linux reports an inbound IPv4 peer as an IPv4-mapped IPv6 address `::ffff:A.B.C.D`. The ban map and address book are keyed on the canonical IPv4 form, but the inbound accept path and the legacy `PeerSet` compared the raw mapped address. Insert-canonical / lookup-raw meant the lookups missed: a banned peer reconnecting as `::ffff:A.B.C.D` was not re-banned, and the recent-inbound rate limiter and `max_conns_per_ip` (default 1) were evadable by alternating the mapped and canonical forms. Canonicalize the inbound address once at the accept boundary via `canonical_peer_addr`, so the ban check, the recent-inbound limiter, and the key forwarded into the `PeerSet` all use the canonical form. As defense-in-depth (and for testability), the peer-set ban re-checks (`poll_unready`, `poll_ready_peer_errors`, and the `broadcast_all_queued` ban filter) and the per-IP accounting (`num_peers_with_ip`) also canonicalize before comparing, via a shared `canonical_ip` helper, so the invariant holds even if a future path inserts a raw key. Only dual-stack binds are affected; nodes that bind `0.0.0.0` never see mapped addresses. The zcashd-compat reserved-slot check already canonicalized and is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
evan-forbes
force-pushed
the
fix/dualstack-canonicalize-inbound
branch
from
July 20, 2026 23:10
bcf91c0 to
9ae206b
Compare
evan-forbes
marked this pull request as ready for review
July 20, 2026 23:10
ValarDragon
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
On a dual-stack listener (
listen_addr = "[::]:port"withIPV6_V6ONLY=false), Linux reports an inbound IPv4 peer as an IPv4-mapped IPv6 address::ffff:A.B.C.D. Zakura's ban map and address book are keyed on the canonical IPv4 form (A.B.C.D), but the inbound accept path and the legacyPeerSetcompared the rawPeerSocketAddr::ip(), which keeps the mapped form. Insert-canonical / lookup-raw ⇒ the lookups miss.Exploit (dual-stack binds only): a banned or rate-limited peer evades enforcement by reconnecting in the other address form:
::ffff:A.B.C.Dis not re-banned (both the accept path and the peer set miss the canonical ban entry).max_conns_per_ipcap (default1) are evadable: the mapped and canonical forms count as two different hosts.Nodes that bind
0.0.0.0never see mapped addresses and are unaffected. The bug is invisible in regtest/e2e (those bind127.0.0.1).Solution
canonical_peer_addrinaccept_inbound_connections. Every later use ofaddr— the ban check, the recent-inbound limiter, and the key forwarded into the handshake /PeerSet— is then canonical.PeerSetban re-checks (poll_unready,poll_ready_peer_errors, and thebroadcast_all_queuedban filter) and the per-IP accounting (num_peers_with_ip) also canonicalize before comparing, via a small sharedcanonical_iphelper. The boundary fix already makes all keys canonical; this enforces the invariant even if some future path inserts a raw key, and lets the equivalence be unit-tested directly by injecting a mapped key.The zcashd-compat reserved-slot check already canonicalized (
canonical_socket_addr(addr.remove_socket_addr_privacy())) and is unchanged. Genuine IPv6 peers are left untouched bycanonical_peer_addr. Outbound keys come from the address book and are already canonical.Testing
cargo fmt --all -- --check,cargo clippy -p zakura-network --all-targets -- -D warnings, andcargo test -p zakura-network(1000 passed, 0 failed) all green, including the zcashd-compat carve-out tests andprop::sidecar_peer_always_receives_block_gossip.Two new deterministic unit tests in
peer_set::set::tests::vectors:per_ip_accounting_treats_mapped_and_canonical_as_same_host— inserts a connection keyed by the canonical form and asserts both the canonical and the mapped query see it as one host, then reverses the key/query roles. This is whatpoll_discover'smax_conns_per_ipgate relies on.broadcast_all_queued_bans_mapped_ipv6_against_canonical_ban— a peer keyed::ffff:A.B.C.Dagainst a ban map holding canonicalA.B.C.Dis dropped by the ban filter. All three peer-set ban re-checks share thecanonical_iphelper, so this exercises that shared path.Both tests were confirmed to fail against the un-canonicalized code (test 1 with
left: 0, right: 1; test 2 with the banned mapped peer surviving the filter) and pass with the fix.Follow-up Work
The async
accept_inbound_connectionsloop (over a realTcpListener) isn't unit-tested directly — aTODO(test)marks the boundary canonicalization, and the equivalence is covered by the peer-set tests above. Extracting the accept decision into a pure(bans, addr) -> accept/rejecthelper for direct testing was left out of scope to keep the diff focused; happy to add it if a reviewer prefers.Independent of #231 (
fix/sidecar-block-gossip-unready) and #236 — different functions, no shared lines. Branched offmainat v1.0.1.