fix(proto): canonicalize remote IP in early_discard_packet's peer check (noq#738) - #783
Open
cuzic wants to merge 5 commits into
Open
fix(proto): canonicalize remote IP in early_discard_packet's peer check (noq#738)#783cuzic wants to merge 5 commits into
cuzic wants to merge 5 commits into
Conversation
…ck (noq#738) The local_ip comparison a few lines below already canonicalizes both sides via IpAddr::to_canonical() (4bae6ed) to handle dual-stack sockets reporting IPv4-mapped-IPv6 addresses. The remote comparison just above it does not, and can trip the same class of bug: an incoming datagram's reported remote SocketAddr (e.g. [::ffff:a.b.c.d]:p) compares unequal to the known path's remote (a.b.c.d:p) purely due to representation, causing early_discard_packet to silently drop every packet on that path before it ever reaches PATH_RESPONSE / frame processing.
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.
Closes #738.
Problem
4bae6edd(the hotfix posted in #738) canonicalizes thelocal_ipcomparison in
Connection::early_discard_packet, to handle dual-stacksockets that report a peer as an IPv4-mapped-IPv6 address
(
::ffff:a.b.c.d) on one side of the comparison and a plain IPv4 addresson the other. We applied it and confirmed on a real Android device
(WiFi + cellular) that it is not sufficient on its own —
open_path()with an explicit
local_ipstill gets abandoned withValidationFailed.The
remotecomparison a few lines above the one4bae6eddtoucheshas the exact same problem, and is not canonicalized:
An incoming datagram's reported remote
SocketAddrcan compare unequalto the known path's remote purely due to mapped-vs-plain representation,
so
early_discard_packetsilently drops every packet on that path beforeit ever reaches
PATH_RESPONSE/ frame processing — same failure mode asthe
local_ipbug, just one comparison earlier in the same function.Fix
Mirror the existing
local_ipfix: canonicalize both sides of theremotecomparison withIpAddr::to_canonical()before comparing.This is the minimal, surgical fix — one comparison site, matching the
style of
4bae6edd. See #784 for an alternative that canonicalizesonce at
FourTuple::new()construction time instead, which also coversa few other latent comparison sites (
Endpoint'sHashMap<FourTuple, ConnectionHandle>routing table,is_probably_same_path, PATH_CHALLENGE on-path detection, OBSERVED_ADDRESSmatching, peer migration detection) that weren't yet observed failing in
our testing but share the same root cause.
Testing
noq-738branch(
noq-proto/src/tests/multipath.rs::open_path_with_explicit_local_ip),adapted to build its
ManyToManyRoutingviaadd_client_route/add_server_routeinstead offrom_routes—from_routesnow rejectsthe duplicate
server_addrthis test intentionally uses (invariantadded by feat(proto): use path idle timeout for validation when opening a new path #721 after the regression test was originally written).
Fails on unpatched
main, passes with this fix.cargo test -p noq-proto: 388 passed, 0 failed (full suite, not justthe new test).
Secondary(local_ip: None)established, then
PhysicalWifi/PhysicalCellular(local_ipexplicit) both validate on the first attempt instead of retrying
3x and getting abandoned: