Fix WireGuard verdict caching and TCP overlap handling - #913
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8e5e625c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| route_flow_store(version, protocol, saddr, sport, daddr, dport, | ||
| route_default_is_tunnel(), 0); |
There was a problem hiding this comment.
When a TCP flow's per-app route differs from the global default, resolve_tunnel_uid() first stores the correct route, but this unconditional call overwrites it with route_default_is_tunnel() and also clears uid_known. On the next packet, selected-mode flows can fall back to direct forwarding and reset because no native session exists, while an app explicitly excluded from a default tunnel can instead be sent through WireGuard. Store the verdict on the existing entry without replacing its route metadata.
Useful? React with 👍 / 👎.
| if depth < MAX_CNAME_CHAIN_DEPTH { | ||
| if let Some(links) = cname_links.get(name) { | ||
| for link in links { | ||
| path.push(link.clone()); | ||
| collect_chain( |
There was a problem hiding this comment.
A DNS response containing repeated or branching CNAME links can make this recursive loop perform exponentially many visits: visited prevents only cycles on the current path and is cleared on return, while output deduplication happens after each path has already been traversed. For example, ten duplicate links at each of eight levels fit easily in one DNS response but induce roughly 100 million recursive walks, allowing an attacker-controlled lookup response to stall the packet path and consume substantial CPU. Deduplicate edges and memoize completed nodes, or enforce a global traversal budget.
Useful? React with 👍 / 👎.
c8e5e62 to
88d1489
Compare
…path-912 # Conflicts: # app/src/test/native/host_compat/jni.h # app/src/test/native/host_compat/netinet/in6.h # app/src/test/native/host_compat/sys/epoll.h
…th-913 # Conflicts: # app/src/test/native/host_compat/netinet/tcp.h
kasnder
changed the title
1 participant
The remaining changes cache and revoke WireGuard flow verdicts and normalise overlapping TCP segments. DNS provenance, UDP lifetime, socket defence and other bounded fixes have already landed in master through #924–#926.
Reconciled with master
5addee60after the combined Pixel-tested batch #926. The remaining chain is #913 → #915 → #916 → #919; #912, #917 and #918 are superseded. These remaining layers are held for a separate combined validation and merge decision. Passing the bounded batch does not establish full readiness for TCP state-machine and WireGuard policy/recovery changes.Reconciliation validation: the combined remaining tip passed GitHub JVM tests and a four-ABI GitHub debug build; native defensive UBSan suites passed on Pixel, and the DNS framing/real-parser suites passed host ASan/UBSan. The TCP half-close/real-epoll fixtures passed on Pixel after adapting their no-op DNS stubs to the merged replay API. Only #926's APK was installed on the Pixel; the held stack's APK was built, not installed. New CI runs are not being waited on. Older-Android, controlled tunnel faults, per-app TCP/QUIC revocation, battery and rollout coverage remain outstanding.