From 84e59b0e682d772174cc65f1e93c16411ba79783 Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Thu, 20 Aug 2026 11:52:39 +0200 Subject: [PATCH 1/2] Fix WireGuard DNS policy parity --- .../eu/faircode/netguard/ServiceSinkhole.java | 26 +- .../missioncontrol/wgbridge/DnsRecorder.java | 18 +- app/src/main/res/values/strings.xml | 2 +- .../ServiceSinkholeSecureDnsTest.java | 89 +++ wgbridge-rs/README.md | 24 +- wgbridge-rs/src/callbacks.rs | 21 +- wgbridge-rs/src/dns.rs | 749 +++++++++++++++++- wgbridge-rs/src/jni_bindings.rs | 24 + wgbridge-rs/src/transport/ip_send.rs | 18 +- 9 files changed, 913 insertions(+), 58 deletions(-) create mode 100644 app/src/test/java/eu/faircode/netguard/ServiceSinkholeSecureDnsTest.java diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 7f4532f11..baef0c5c7 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -1464,7 +1464,15 @@ private static List getWireGuardDns(net.kollnig.missioncontrol.wg.W return listDns; } - private static boolean hasActiveWireGuardDns(SharedPreferences prefs) { + /** + * Whether the configured WireGuard egress will own DNS routing. + * + * The tunnel supplies a protected public fallback when the config has no + * usable DNS entry, so Secure DNS must be paused for that case too. The + * parser check keeps a malformed or incomplete preference from suppressing + * DoH when no tunnel can actually be started. + */ + static boolean hasActiveWireGuard(SharedPreferences prefs) { if (!prefs.getBoolean("wg_enabled", false)) return false; @@ -1473,9 +1481,8 @@ private static boolean hasActiveWireGuardDns(SharedPreferences prefs) { return false; try { - net.kollnig.missioncontrol.wg.WgConfig config = - net.kollnig.missioncontrol.wg.WgConfigParser.INSTANCE.parse(wgConfigText); - return !getWireGuardDns(config).isEmpty(); + net.kollnig.missioncontrol.wg.WgConfigParser.INSTANCE.parse(wgConfigText); + return true; } catch (Throwable ignored) { return false; } @@ -1486,8 +1493,8 @@ private void updateDnsProxyState() { net.kollnig.missioncontrol.dns.DnsProxyServer proxy = net.kollnig.missioncontrol.dns.DnsProxyServer.getInstance(this); - if (prefs.getBoolean("doh_enabled", false) && hasActiveWireGuardDns(prefs)) { - Log.i(TAG, "Secure DNS proxy disabled while WireGuard DNS is active"); + if (prefs.getBoolean("doh_enabled", false) && hasActiveWireGuard(prefs)) { + Log.i(TAG, "Secure DNS proxy disabled while WireGuard egress is active"); proxy.stop(); } else { proxy.checkAndUpdateState(); @@ -2149,8 +2156,11 @@ private void prepareForwarding() { } } - // Add DoH DNS forwarding when enabled and not superseded by WireGuard DNS. - if (prefs.getBoolean("doh_enabled", false) && !hasActiveWireGuardDns(prefs)) { + // Add DoH DNS forwarding only when WireGuard is not owning DNS. This + // includes configs without a DNS line: getBuilder() installs a + // protected public fallback for those configs, and the DoH client is + // excluded from this VPN so it cannot be safely chained through WG. + if (prefs.getBoolean("doh_enabled", false) && !hasActiveWireGuard(prefs)) { Forward dnsFwd = new Forward(); dnsFwd.protocol = 17; // UDP dnsFwd.dport = 53; diff --git a/app/src/main/java/net/kollnig/missioncontrol/wgbridge/DnsRecorder.java b/app/src/main/java/net/kollnig/missioncontrol/wgbridge/DnsRecorder.java index 935e8fa41..c5b47a0d0 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/wgbridge/DnsRecorder.java +++ b/app/src/main/java/net/kollnig/missioncontrol/wgbridge/DnsRecorder.java @@ -1,11 +1,23 @@ package net.kollnig.missioncontrol.wgbridge; /** - * Receives DNS answers observed on decrypted inbound packets. Passive: - * TrackerControl uses this mapping later when deciding on app connections, - * but the DNS response is not blocked or rewritten. Called from native + * Receives DNS answers observed on decrypted inbound packets and exposes the + * DNS policy used when a response is sent back to the app. Called from native * tunnel threads. */ public interface DnsRecorder { void recordDns(String qname, String aname, String resource, int ttl); + + /** + * Returns whether a response for {@code qname} should be returned without + * answers. The current app policy is intentionally a no-op. + */ + default boolean isDomainBlocked(String qname) { + return false; + } + + /** RCODE for a response blanked by the DNS policy (NXDOMAIN by default). */ + default int blockedRcode() { + return 3; + } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 9f7af62bc..4d830c382 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -166,7 +166,7 @@ Secure DNS (DoH) - Encrypt DNS queries using DNS-over-HTTPS. Automatically paused when WireGuard provides DNS, because those queries use the WireGuard tunnel instead. + Encrypt DNS queries using DNS-over-HTTPS. Automatically paused when WireGuard is active, because those queries use the WireGuard tunnel instead. Beta feature. May not work as expected. DoH Endpoint URL HTTPS URL for DNS-over-HTTPS queries diff --git a/app/src/test/java/eu/faircode/netguard/ServiceSinkholeSecureDnsTest.java b/app/src/test/java/eu/faircode/netguard/ServiceSinkholeSecureDnsTest.java new file mode 100644 index 000000000..d760c0b18 --- /dev/null +++ b/app/src/test/java/eu/faircode/netguard/ServiceSinkholeSecureDnsTest.java @@ -0,0 +1,89 @@ +/* + * This file is part of TrackerControl. + * + * TrackerControl is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + */ + +package eu.faircode.netguard; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import android.content.SharedPreferences; + +import androidx.preference.PreferenceManager; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.RuntimeEnvironment; + +/** + * Secure DNS must not be started alongside the userspace WireGuard egress. + * The egress supplies the VPN DNS path, including its public fallback when a + * config omits {@code DNS =}; the app process itself is excluded from the VPN. + */ +@RunWith(RobolectricTestRunner.class) +public class ServiceSinkholeSecureDnsTest { + private static final String KEY = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="; + private static final String WG_CONFIG = + "[Interface]\n" + + "PrivateKey = " + KEY + "\n" + + "Address = 10.64.0.2/32\n" + + "%s" + + "\n[Peer]\n" + + "PublicKey = " + KEY + "\n" + + "AllowedIPs = 0.0.0.0/0\n" + + "Endpoint = 198.51.100.1:51820\n"; + + private SharedPreferences prefs; + + @Before + public void setUp() { + prefs = PreferenceManager.getDefaultSharedPreferences(RuntimeEnvironment.getApplication()); + prefs.edit().clear().commit(); + } + + @Test + public void wireGuardWithoutDnsStillOwnsDnsPath() { + prefs.edit() + .putBoolean("wg_enabled", true) + .putString("wg_config", String.format(WG_CONFIG, "")) + .commit(); + + assertTrue(ServiceSinkhole.hasActiveWireGuard(prefs)); + } + + @Test + public void wireGuardWithDnsOwnsDnsPath() { + prefs.edit() + .putBoolean("wg_enabled", true) + .putString("wg_config", String.format(WG_CONFIG, "DNS = 10.64.0.1\n")) + .commit(); + + assertTrue(ServiceSinkhole.hasActiveWireGuard(prefs)); + } + + @Test + public void disabledOrIncompleteWireGuardDoesNotSuppressDoh() { + prefs.edit().putString("wg_config", String.format(WG_CONFIG, "")).commit(); + assertFalse(ServiceSinkhole.hasActiveWireGuard(prefs)); + + prefs.edit().putBoolean("wg_enabled", true).putString("wg_config", "").commit(); + assertFalse(ServiceSinkhole.hasActiveWireGuard(prefs)); + } + + @Test + public void malformedWireGuardDoesNotSuppressDoh() { + prefs.edit() + .putBoolean("wg_enabled", true) + .putString("wg_config", "not a WireGuard config") + .commit(); + + assertFalse(ServiceSinkhole.hasActiveWireGuard(prefs)); + } +} diff --git a/wgbridge-rs/README.md b/wgbridge-rs/README.md index a9996a560..f857fae26 100644 --- a/wgbridge-rs/README.md +++ b/wgbridge-rs/README.md @@ -26,8 +26,9 @@ encrypted side), so we plug in: - `SocketpairRecv` — reads outbound raw IP packets from the socketpair fd written by `jni/netguard/ip.c` (batched, via tokio's `AsyncFd`); - `TunFdSend` — writes decrypted inbound packets to the VpnService TUN fd, - running passive DNS inspection (A/AAAA answers feed TrackerControl's - tracker mapping) on the way through; + recording A/AAAA answers for TrackerControl's mapping and applying its DNS + response policy (SVCB/HTTPS blanking and the explicit domain-policy hook) on + the way through; - `ProtectedUdpFactory` — binds the outer UDP sockets and protects them via the Java `Protector` callback. Because gotatun re-invokes the factory on every reconfigure, `Tunnel.rebind()` doubles as "move the encrypted @@ -128,7 +129,11 @@ class Wgbridge { interface Protector { boolean protect(int fd); } interface Logger { void verbosef(String s); void errorf(String s); } -interface DnsRecorder { void recordDns(String qname, String aname, String resource, int ttl); } +interface DnsRecorder { + void recordDns(String qname, String aname, String resource, int ttl); + default boolean isDomainBlocked(String qname) { return false; } + default int blockedRcode() { return 3; } +} class Tunnel { void setConfig(String uapiConfig); @@ -148,11 +153,8 @@ hostnames, and re-resolves them on network changes via `updateEndpoint`). ## Potential improvements -- **DNS upstream privacy**: app DNS packets to port 53 currently stay on the - local NetGuard path so DNS forwarding, tracker lookup, and local resolvers - keep working. That is fine for interception, but the upstream resolver path - should be revisited when WireGuard is enabled. Ideally, TrackerControl would - still intercept app DNS locally while sending DoH/plain-DNS fallback upstream - through WireGuard, except for deliberately local-network DNS such as a router - or Pi-hole. This is not urgent, but it matters for a complete IP-privacy - story because TrackerControl itself is excluded from the VPN route. +- **Split DNS-over-TCP rewriting**: inbound TCP DNS is recorded with bounded + reassembly, but response rewriting is limited to complete frames that begin + and end in one sequence-aligned segment. Rewriting a frame split across + packets needs buffering plus TCP sequence translation; continuation segments + are deliberately forwarded unchanged until that exists. diff --git a/wgbridge-rs/src/callbacks.rs b/wgbridge-rs/src/callbacks.rs index 7c18e581b..7b559887f 100644 --- a/wgbridge-rs/src/callbacks.rs +++ b/wgbridge-rs/src/callbacks.rs @@ -7,11 +7,26 @@ pub trait SocketProtector: Send + Sync + 'static { fn protect(&self, fd: i32) -> bool; } -/// Receives DNS answers observed on decrypted inbound packets. Passive: -/// TrackerControl uses this mapping later when deciding on app connections, -/// but the DNS response is not blocked or rewritten here. +/// Receives DNS answers observed on decrypted inbound packets and exposes the +/// DNS policy used when the response is sent back to the app. pub trait DnsSink: Send + Sync + 'static { fn record_dns(&self, qname: &str, aname: &str, resource: &str, ttl: i32); + + /// Whether a response for `qname` should be returned without answers. + /// + /// The current Android callback deliberately returns `false`, matching + /// ServiceSinkhole.isDomainBlocked on the master branch. Keeping this as + /// an explicit policy hook lets the packet rewriter preserve the native + /// path's semantics without inventing a second blocklist in Rust. + fn is_domain_blocked(&self, _qname: &str) -> bool { + false + } + + /// RCODE used for a response blanked by the DNS policy. DNS RCODE is four + /// bits; invalid callback values are clamped by the packet rewriter. + fn blocked_rcode(&self) -> u8 { + 3 // NXDOMAIN, matching the native default preference. + } } /// Bridge-level log lines destined for the Java side. diff --git a/wgbridge-rs/src/dns.rs b/wgbridge-rs/src/dns.rs index 9e73d0ea2..9166a10ec 100644 --- a/wgbridge-rs/src/dns.rs +++ b/wgbridge-rs/src/dns.rs @@ -1,9 +1,11 @@ -//! Passive DNS inspection of decrypted inbound packets (port of the old Go -//! wgbridge/dns.go). Extracts A/AAAA answers from UDP:53 responses and hands -//! them to the [`DnsSink`]; packets are never modified or blocked here. +//! DNS inspection and response policy for decrypted inbound packets (port of +//! the old Go wgbridge/dns.go). Extracts A/AAAA answers from UDP:53 responses, +//! hands them to the [`DnsSink`], and applies the native SVCB/HTTPS and +//! explicit-domain response policy before packets reach the TUN. use std::collections::HashMap; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; +use std::ops::Range; use std::panic::{catch_unwind, AssertUnwindSafe}; use std::time::{Duration, Instant}; @@ -12,6 +14,9 @@ use crate::callbacks::DnsSink; const DNS_TYPE_A: u16 = 1; const DNS_TYPE_AAAA: u16 = 28; const DNS_CLASS_IN: u16 = 1; +const DNS_TYPE_SVCB: u16 = 64; +const DNS_TYPE_HTTPS: u16 = 65; +const DNS_HEADER_LEN: usize = 12; const IP_PROTO_HOP_BY_HOP: u8 = 0; const IP_PROTO_TCP: u8 = 6; @@ -36,11 +41,18 @@ struct TcpDnsFlow { next_seq: u32, buffer: Vec, last_seen: Instant, + framing_known: bool, } -/// Stateful passive DNS inspector. UDP messages are handled directly; TCP -/// messages are reassembled per flow using sequence numbers and the DNS-over- -/// TCP two-byte length prefix. +struct TcpRewriteContext { + /// Complete DNS message ranges within the current TCP segment. The + /// two-byte DNS-over-TCP length prefix is outside each range. + frames: Vec>, +} + +/// Stateful DNS inspector. UDP messages are handled directly; TCP messages +/// are reassembled per flow using sequence numbers and the DNS-over-TCP +/// two-byte length prefix. The same framing state gates TCP response rewrite. #[derive(Default)] pub struct DnsInspector { tcp_flows: HashMap, @@ -60,22 +72,30 @@ impl DnsInspector { record_answers(msg, recorder); } } - IP_PROTO_TCP => self.inspect_tcp(packet, segment, recorder), + IP_PROTO_TCP => { + let _ = self.inspect_tcp(packet, segment, recorder); + } _ => {} } } - fn inspect_tcp(&mut self, packet: &[u8], tcp: &[u8], recorder: &dyn DnsSink) { + fn inspect_tcp( + &mut self, + packet: &[u8], + tcp: &[u8], + recorder: &dyn DnsSink, + ) -> Option { let Some(segment) = tcp_segment(packet, tcp) else { - return; + return None; }; + let mut context = TcpRewriteContext { frames: Vec::new() }; let now = Instant::now(); self.tcp_flows .retain(|_, flow| now.duration_since(flow.last_seen) <= TCP_DNS_IDLE_TIMEOUT); if segment.rst { self.tcp_flows.remove(&segment.key); - return; + return Some(context); } if segment.syn { if !self.tcp_flows.contains_key(&segment.key) { @@ -87,6 +107,7 @@ impl DnsInspector { next_seq: segment.seq.wrapping_add(1), buffer: Vec::new(), last_seen: now, + framing_known: true, }, ); } @@ -96,39 +117,60 @@ impl DnsInspector { self.ensure_capacity(); } let data_seq = segment.seq.wrapping_add(u32::from(segment.syn)); - let flow = self - .tcp_flows - .entry(segment.key.clone()) - .or_insert_with(|| TcpDnsFlow { - // Best-effort bootstrap for a connection that pre-dates the - // inspector. A later gap invalidates the stream until a SYN. - next_seq: data_seq, - buffer: Vec::new(), - last_seen: now, - }); - flow.last_seen = now; + let next_seq = if let Some(next_seq) = + self.tcp_flows.get(&segment.key).map(|flow| flow.next_seq) + { + next_seq + } else { + self.tcp_flows.insert( + segment.key.clone(), + TcpDnsFlow { + // Best-effort bootstrap for a connection that pre-dates the + // inspector. It is intentionally not rewrite-eligible until + // a SYN establishes a known DNS frame boundary. + next_seq: data_seq, + buffer: Vec::new(), + last_seen: now, + framing_known: false, + }, + ); + data_seq + }; - let already_seen = flow.next_seq.wrapping_sub(data_seq); - let payload = if data_seq == flow.next_seq { - segment.payload + let already_seen = next_seq.wrapping_sub(data_seq); + let payload_start = if data_seq == next_seq { + 0 } else if already_seen >= 0x8000_0000 { // A forward gap means bytes needed to find message boundaries // are missing. Drop the flow instead of parsing a mid-message // payload as a new length prefix. self.tcp_flows.remove(&segment.key); - return; + return Some(context); } else { // A wholly-old or partially-overlapping retransmission: skip // the bytes we've already consumed and keep the flow alive. // If the retransmission lies entirely behind the frontier, // this yields an empty tail, which is a harmless no-op. - &segment.payload[(already_seen as usize).min(segment.payload.len())..] + (already_seen as usize).min(segment.payload.len()) }; + let payload = &segment.payload[payload_start..]; + let flow = self.tcp_flows.get_mut(&segment.key).expect("flow inserted"); + flow.last_seen = now; if flow.buffer.len() + payload.len() > MAX_TCP_DNS_BUFFER { self.tcp_flows.remove(&segment.key); - return; + return Some(context); + } + if flow.framing_known { + let buffer_before = flow.buffer.clone(); + for range in complete_frame_ranges(&buffer_before, payload) { + context.frames.push( + (segment.payload_offset + payload_start + range.start) + ..(segment.payload_offset + payload_start + range.end), + ); + } } + flow.buffer.extend_from_slice(payload); flow.next_seq = flow.next_seq.wrapping_add(payload.len() as u32); @@ -150,6 +192,7 @@ impl DnsInspector { if segment.fin { self.tcp_flows.remove(&segment.key); } + Some(context) } fn ensure_capacity(&mut self) { @@ -173,13 +216,458 @@ struct TcpSegment<'a> { syn: bool, fin: bool, rst: bool, + payload_offset: usize, payload: &'a [u8], } +/// Finds complete DNS frames whose prefix starts in `incoming` after any +/// previously buffered frame has been consumed. Returning only those ranges +/// prevents a continuation or retransmission from being parsed as a fresh +/// DNS-over-TCP message. +fn complete_frame_ranges(buffer: &[u8], incoming: &[u8]) -> Vec> { + let mut combined = Vec::with_capacity(buffer.len() + incoming.len()); + combined.extend_from_slice(buffer); + combined.extend_from_slice(incoming); + let prefix_len = buffer.len(); + let mut cursor = 0usize; + + // Consume the frame that began in the previous segment. It may complete + // in `incoming`; until it does, no byte in this segment is frame-aligned. + while cursor < prefix_len { + if cursor + 2 > combined.len() { + return Vec::new(); + } + let msg_len = u16::from_be_bytes([combined[cursor], combined[cursor + 1]]) as usize; + let frame_len = if msg_len == 0 { 2 } else { 2 + msg_len }; + if cursor + frame_len > combined.len() { + return Vec::new(); + } + cursor += frame_len; + } + + let mut ranges = Vec::new(); + while cursor + 2 <= combined.len() { + let msg_len = u16::from_be_bytes([combined[cursor], combined[cursor + 1]]) as usize; + if msg_len == 0 { + cursor += 2; + continue; + } + let frame_end = cursor + 2 + msg_len; + if frame_end > combined.len() { + break; + } + if cursor >= prefix_len { + ranges.push((cursor - prefix_len + 2)..(frame_end - prefix_len)); + } + cursor = frame_end; + } + ranges +} + pub fn inspect_dns_response(packet: &[u8], recorder: &dyn DnsSink) { DnsInspector::default().inspect(packet, recorder); } +impl DnsInspector { + /// Inspects a decrypted packet and applies DNS policy before it reaches + /// the TUN. TCP rewriting is deliberately coupled to this inspector's + /// sequence/framing state; callers must not use the stateless UDP helper + /// for TCP segments. + pub fn inspect_and_rewrite( + &mut self, + packet: &mut [u8], + policy: &dyn DnsSink, + ) -> Option { + let Some(view) = dns_packet_view(packet) else { + self.inspect(packet, policy); + return None; + }; + if view.is_udp { + self.inspect(packet, policy); + return rewrite_dns_response(packet, policy); + } + + let tcp = &packet[view.transport_offset..view.ip_total_len]; + let Some(context) = self.inspect_tcp(packet, tcp, policy) else { + return None; + }; + let mut rewritten = false; + for range in context.frames { + let msg_start = view.transport_offset + range.start; + let msg_end = view.transport_offset + range.end; + if msg_end > packet.len() { + continue; + } + let msg = &packet[msg_start..msg_end]; + let Some(layout) = parse_dns_layout(msg) else { + continue; + }; + if layout.contains_svcb || policy_is_domain_blocked(policy, &layout.qname) { + blank_dns_message( + &mut packet[msg_start..msg_end], + policy_blocked_rcode(policy), + ); + rewritten = true; + } + } + if !rewritten { + return None; + } + + repair_packet(packet, &view, view.ip_total_len); + Some(view.ip_total_len) + } +} + +/// Applies the native DNS response policy to one decrypted IP packet. +/// +/// The caller must run [`DnsInspector::inspect`] first. That preserves the +/// native ordering where A/AAAA answers are recorded before a response is +/// blanked. UDP responses can be shortened because they are datagrams. TCP +/// packets are never shortened: changing a DNS-over-TCP payload length would +/// require sequence-number translation for every later segment. Instead, a +/// complete UDP response has its counts cleared and is trimmed to the question +/// section. TCP must go through [`DnsInspector::inspect_and_rewrite`], which +/// aligns complete frames to the tracked TCP sequence frontier. +/// +/// Returns the packet length to write when a response was rewritten. `None` +/// means that the packet was not a DNS response or policy left it unchanged. +pub fn rewrite_dns_response(packet: &mut [u8], policy: &dyn DnsSink) -> Option { + let view = dns_packet_view(packet)?; + if !view.is_udp { + return None; + } + + let msg = &packet[view.dns_start..view.dns_end]; + let layout = parse_dns_layout(msg)?; + if !layout.contains_svcb && !policy_is_domain_blocked(policy, &layout.qname) { + return None; + } + + let msg = &mut packet[view.dns_start..view.dns_end]; + blank_dns_message(msg, policy_blocked_rcode(policy)); + let new_total = view.dns_start + layout.question_end; + repair_packet(packet, &view, new_total); + Some(new_total) +} + +#[derive(Clone, Copy, Debug)] +struct DnsPacketView { + ip_version: u8, + ip_header_len: usize, + transport_offset: usize, + dns_start: usize, + dns_end: usize, + ip_total_len: usize, + is_udp: bool, +} + +fn dns_packet_view(packet: &[u8]) -> Option { + if packet.is_empty() { + return None; + } + match packet[0] >> 4 { + 4 => { + if packet.len() < 20 { + return None; + } + let ihl = (packet[0] & 0x0f) as usize * 4; + if ihl < 20 || ihl > packet.len() { + return None; + } + let total = u16::from_be_bytes([packet[2], packet[3]]) as usize; + let total = if total == 0 { packet.len() } else { total }; + if total < ihl || total > packet.len() { + return None; + } + // A non-zero fragment offset has no complete transport header. + // The first fragment is also left alone when MF is set: a DNS + // response cannot be safely rewritten without the full datagram. + let fragment = u16::from_be_bytes([packet[6], packet[7]]); + if fragment & 0x3fff != 0 || fragment & 0x2000 != 0 { + return None; + } + let proto = packet[9]; + let (is_udp, header_len) = match proto { + IP_PROTO_UDP => (true, 8), + IP_PROTO_TCP => { + if total < ihl + 20 { + return None; + } + let tcp = ihl; + let data_offset = (packet[tcp + 12] >> 4) as usize * 4; + if data_offset < 20 || total < tcp + data_offset { + return None; + } + (false, data_offset) + } + _ => return None, + }; + let transport = ihl; + if total < transport + header_len + || u16::from_be_bytes([packet[transport], packet[transport + 1]]) != 53 + { + return None; + } + let dns_start = transport + header_len; + let dns_end = if is_udp { + let udp_len = + u16::from_be_bytes([packet[transport + 4], packet[transport + 5]]) as usize; + if udp_len < 8 || transport + udp_len > total { + return None; + } + transport + udp_len + } else { + total + }; + if dns_end <= dns_start { + return None; + } + Some(DnsPacketView { + ip_version: 4, + ip_header_len: ihl, + transport_offset: transport, + dns_start, + dns_end, + ip_total_len: total, + is_udp, + }) + } + 6 => { + if packet.len() < 40 { + return None; + } + let payload_len = u16::from_be_bytes([packet[4], packet[5]]) as usize; + let total = if payload_len == 0 { + packet.len() + } else { + 40 + payload_len + }; + if total < 40 || total > packet.len() { + return None; + } + let mut next = packet[6]; + let mut transport = 40usize; + while next != IP_PROTO_UDP && next != IP_PROTO_TCP { + if next == IP_PROTO_FRAGMENT || !is_ipv6_ext_header(next) || total < transport + 2 { + return None; + } + let ext_len = (packet[transport + 1] as usize + 1) * 8; + if ext_len < 8 || total < transport + ext_len { + return None; + } + next = packet[transport]; + transport += ext_len; + } + let (is_udp, header_len) = if next == IP_PROTO_UDP { + (true, 8) + } else { + if total < transport + 20 { + return None; + } + let data_offset = (packet[transport + 12] >> 4) as usize * 4; + if data_offset < 20 || total < transport + data_offset { + return None; + } + (false, data_offset) + }; + if total < transport + header_len + || u16::from_be_bytes([packet[transport], packet[transport + 1]]) != 53 + { + return None; + } + let dns_start = transport + header_len; + let dns_end = if is_udp { + let udp_len = + u16::from_be_bytes([packet[transport + 4], packet[transport + 5]]) as usize; + if udp_len < 8 || transport + udp_len > total { + return None; + } + transport + udp_len + } else { + total + }; + if dns_end <= dns_start { + return None; + } + Some(DnsPacketView { + ip_version: 6, + ip_header_len: 40, + transport_offset: transport, + dns_start, + dns_end, + ip_total_len: total, + is_udp, + }) + } + _ => None, + } +} + +#[derive(Debug)] +struct DnsLayout { + qname: String, + question_end: usize, + contains_svcb: bool, +} + +fn parse_dns_layout(msg: &[u8]) -> Option { + if msg.len() < DNS_HEADER_LEN { + return None; + } + let flags = u16::from_be_bytes([msg[2], msg[3]]); + if flags & 0x8000 == 0 || flags & 0x7800 != 0 { + return None; + } + let qdcount = u16::from_be_bytes([msg[4], msg[5]]) as usize; + let ancount = u16::from_be_bytes([msg[6], msg[7]]) as usize; + if qdcount == 0 || ancount == 0 { + return None; + } + + let mut off = DNS_HEADER_LEN; + let mut qname = None; + for q in 0..qdcount { + let (name, next) = read_dns_name(msg, off, 0)?; + if next + 4 > msg.len() { + return None; + } + if q == 0 { + qname = Some(name); + } + off = next + 4; + } + let question_end = off; + let mut contains_svcb = false; + for _ in 0..ancount { + let (_name, next) = read_dns_name(msg, off, 0)?; + if next + 10 > msg.len() { + return None; + } + let typ = u16::from_be_bytes([msg[next], msg[next + 1]]); + let class = u16::from_be_bytes([msg[next + 2], msg[next + 3]]); + let rdlen = u16::from_be_bytes([msg[next + 8], msg[next + 9]]) as usize; + let rdata = next + 10; + if rdata + rdlen > msg.len() { + return None; + } + contains_svcb |= class == DNS_CLASS_IN && (typ == DNS_TYPE_SVCB || typ == DNS_TYPE_HTTPS); + off = rdata + rdlen; + } + Some(DnsLayout { + qname: qname?, + question_end, + contains_svcb, + }) +} + +fn blank_dns_message(msg: &mut [u8], rcode: u8) { + // Keep the ID and question section. The trailing answer bytes are left in + // place for TCP sequence safety; counts make them unreachable to DNS + // parsers. UDP callers trim the datagram at question_end afterwards. + let flags = 0x8000u16 | u16::from(rcode & 0x0f); + msg[2..4].copy_from_slice(&flags.to_be_bytes()); + msg[6..12].fill(0); +} + +fn policy_is_domain_blocked(policy: &dyn DnsSink, qname: &str) -> bool { + catch_unwind(AssertUnwindSafe(|| policy.is_domain_blocked(qname))).unwrap_or(false) +} + +fn policy_blocked_rcode(policy: &dyn DnsSink) -> u8 { + catch_unwind(AssertUnwindSafe(|| policy.blocked_rcode())) + .unwrap_or(3) + .min(15) +} + +fn repair_packet(packet: &mut [u8], view: &DnsPacketView, new_total: usize) { + let transport_len = new_total.saturating_sub(view.transport_offset); + if view.is_udp { + packet[view.transport_offset + 4..view.transport_offset + 6] + .copy_from_slice(&(transport_len as u16).to_be_bytes()); + } + if view.ip_version == 4 { + packet[2..4].copy_from_slice(&(new_total as u16).to_be_bytes()); + } else { + packet[4..6].copy_from_slice(&((new_total - view.ip_header_len) as u16).to_be_bytes()); + } + + let checksum_offset = if view.is_udp { + view.transport_offset + 6 + } else { + view.transport_offset + 16 + }; + let old_checksum = u16::from_be_bytes([packet[checksum_offset], packet[checksum_offset + 1]]); + if !(view.is_udp && view.ip_version == 4 && old_checksum == 0) { + packet[checksum_offset..checksum_offset + 2].fill(0); + let checksum = encode_transport_checksum(transport_checksum(packet, view, new_total)); + packet[checksum_offset..checksum_offset + 2].copy_from_slice(&checksum.to_be_bytes()); + } + if view.ip_version == 4 { + packet[10..12].fill(0); + let checksum = internet_checksum(&packet[..view.ip_header_len]); + packet[10..12].copy_from_slice(&checksum.to_be_bytes()); + } +} + +fn encode_transport_checksum(checksum: u16) -> u16 { + if checksum == 0 { + u16::MAX + } else { + checksum + } +} + +fn transport_checksum(packet: &[u8], view: &DnsPacketView, total: usize) -> u16 { + let transport = &packet[view.transport_offset..total]; + let mut pseudo = + Vec::with_capacity(if view.ip_version == 4 { 12 } else { 40 } + transport.len()); + if view.ip_version == 4 { + pseudo.extend_from_slice(&packet[12..16]); + pseudo.extend_from_slice(&packet[16..20]); + pseudo.extend_from_slice(&[ + 0, + if view.is_udp { + IP_PROTO_UDP + } else { + IP_PROTO_TCP + }, + ]); + pseudo.extend_from_slice(&(transport.len() as u16).to_be_bytes()); + } else { + pseudo.extend_from_slice(&packet[8..24]); + pseudo.extend_from_slice(&packet[24..40]); + pseudo.extend_from_slice(&(transport.len() as u32).to_be_bytes()); + pseudo.extend_from_slice(&[ + 0, + 0, + 0, + if view.is_udp { + IP_PROTO_UDP + } else { + IP_PROTO_TCP + }, + ]); + } + pseudo.extend_from_slice(transport); + internet_checksum(&pseudo) +} + +fn internet_checksum(data: &[u8]) -> u16 { + let mut sum = 0u32; + let mut chunks = data.chunks_exact(2); + for chunk in &mut chunks { + sum += u16::from_be_bytes([chunk[0], chunk[1]]) as u32; + } + if let Some(&byte) = chunks.remainder().first() { + sum += u32::from(byte) << 8; + } + while sum >> 16 != 0 { + sum = (sum & 0xffff) + (sum >> 16); + } + !(sum as u16) +} + fn record_answers(msg: &[u8], recorder: &dyn DnsSink) { for rr in parse_dns_answers(msg) { // The recorder crosses into Java; never let a failure there take @@ -237,6 +725,7 @@ fn tcp_segment<'a>(packet: &[u8], tcp: &'a [u8]) -> Option> { syn: flags & 0x02 != 0, fin: flags & 0x01 != 0, rst: flags & 0x04 != 0, + payload_offset: data_off, payload: &tcp[data_off..], }) } @@ -819,4 +1308,210 @@ mod tests { )] ); } + + struct BlockingSink; + + impl DnsSink for BlockingSink { + fn record_dns(&self, _: &str, _: &str, _: &str, _: i32) {} + + fn is_domain_blocked(&self, _: &str) -> bool { + true + } + } + + fn svcb_response() -> Vec { + dns_message(&[ + dns_question("tracker.example", DNS_TYPE_A), + dns_answer_bytes("tracker.example", DNS_TYPE_A, 300, &[203, 0, 113, 7]), + dns_answer_bytes("tracker.example", DNS_TYPE_HTTPS, 300, &[]), + ]) + } + + fn assert_transport_checksum_valid(packet: &[u8]) { + let view = dns_packet_view(packet).expect("DNS packet view"); + assert_eq!(transport_checksum(packet, &view, packet.len()), 0); + } + + #[test] + fn zero_transport_checksum_is_encoded_as_ffff() { + assert_eq!(encode_transport_checksum(0), u16::MAX); + assert_eq!(encode_transport_checksum(1), 1); + } + + #[test] + fn rewrite_svcb_ipv4_udp_trims_and_repairs_checksums() { + let msg = svcb_response(); + let question_end = parse_dns_layout(&msg).unwrap().question_end; + let mut packet = ipv4_udp(&msg); + let sink = CollectingSink(Mutex::new(Vec::new())); + let mut inspector = DnsInspector::default(); + + // The A record is recorded before the response is blanked. + inspector.inspect(&packet, &sink); + assert_eq!(sink.0.lock().unwrap().len(), 1); + // A non-zero incoming checksum exercises the rewrite path. IPv4 UDP + // packets with checksum zero deliberately retain zero. + packet[26..28].copy_from_slice(&0x1234u16.to_be_bytes()); + let new_len = rewrite_dns_response(&mut packet, &sink).unwrap(); + + assert_eq!(new_len, 20 + 8 + question_end); + packet.truncate(new_len); + assert_eq!(packet.len(), new_len); + let (_, segment) = transport_segment(&packet).unwrap(); + let payload = udp_dns_payload(segment).unwrap(); + assert_eq!(payload.len(), question_end); + assert_eq!(u16::from_be_bytes([payload[2], payload[3]]), 0x8003); + assert_eq!(&payload[6..12], &[0, 0, 0, 0, 0, 0]); + assert_eq!( + u16::from_be_bytes([packet[2], packet[3]]) as usize, + packet.len() + ); + assert_eq!(internet_checksum(&packet[..20]), 0); + assert_transport_checksum_valid(&packet); + } + + #[test] + fn rewrite_svcb_ipv6_udp_updates_payload_and_checksum() { + let msg = svcb_response(); + let question_end = parse_dns_layout(&msg).unwrap().question_end; + let mut packet = ipv6_udp_with_destination_options(&msg); + let sink = CollectingSink(Mutex::new(Vec::new())); + + let new_len = rewrite_dns_response(&mut packet, &sink).unwrap(); + assert_eq!(new_len, 48 + 8 + question_end); + packet.truncate(new_len); + assert_eq!( + u16::from_be_bytes([packet[4], packet[5]]) as usize, + packet.len() - 40 + ); + let (_, segment) = transport_segment(&packet).unwrap(); + assert_eq!(udp_dns_payload(segment).unwrap().len(), question_end); + assert_transport_checksum_valid(&packet); + } + + #[test] + fn rewrite_blocked_domain_ipv4_udp_uses_explicit_policy_hook() { + let msg = dns_message(&[ + dns_question("blocked.example", DNS_TYPE_A), + dns_answer_bytes("blocked.example", DNS_TYPE_A, 300, &[203, 0, 113, 8]), + ]); + let question_end = parse_dns_layout(&msg).unwrap().question_end; + let mut packet = ipv4_udp(&msg); + packet[26..28].copy_from_slice(&0x1234u16.to_be_bytes()); + + let new_len = rewrite_dns_response(&mut packet, &BlockingSink).unwrap(); + assert_eq!(new_len, 20 + 8 + question_end); + packet.truncate(new_len); + let (_, segment) = transport_segment(&packet).unwrap(); + let payload = udp_dns_payload(segment).unwrap(); + assert_eq!(payload.len(), question_end); + assert_eq!(u16::from_be_bytes([payload[2], payload[3]]), 0x8003); + assert_transport_checksum_valid(&packet); + } + + #[test] + fn rewrite_tcp_keeps_frame_and_packet_lengths() { + let msg = svcb_response(); + let framed = tcp_dns_framed(&msg); + let mut packet = ipv4_tcp(&framed); + let original_len = packet.len(); + let sink = CollectingSink(Mutex::new(Vec::new())); + let mut inspector = DnsInspector::default(); + + // A standalone data segment is not rewrite-eligible until a SYN has + // established the TCP frame boundary. + assert_eq!(inspector.inspect_and_rewrite(&mut packet, &sink), None); + let syn = ipv4_tcp_segment(&[], 999, 0x12); + let mut packet = syn; + let _ = inspector.inspect_and_rewrite(&mut packet, &sink); + let mut packet = ipv4_tcp_segment(&framed, 1000, 0x18); + assert_eq!( + inspector.inspect_and_rewrite(&mut packet, &sink), + Some(original_len) + ); + assert_eq!(packet.len(), original_len); + let (_, segment) = transport_segment(&packet).unwrap(); + let payload = &segment[20..]; + let msg_len = u16::from_be_bytes([payload[0], payload[1]]) as usize; + assert_eq!(msg_len, msg.len()); + assert_eq!(u16::from_be_bytes([payload[2 + 2], payload[2 + 3]]), 0x8003); + assert_eq!(&payload[2 + 6..2 + 12], &[0, 0, 0, 0, 0, 0]); + assert_transport_checksum_valid(&packet); + } + + #[test] + fn rewrite_tcp_split_frame_is_a_no_op() { + let framed = tcp_dns_framed(&svcb_response()); + let split = framed.len() / 2; + let sink = CollectingSink(Mutex::new(Vec::new())); + let mut inspector = DnsInspector::default(); + let syn = ipv4_tcp_segment(&[], 999, 0x12); + let mut syn_packet = syn; + inspector.inspect_and_rewrite(&mut syn_packet, &sink); + let mut packet = ipv4_tcp_segment(&framed[..split], 1000, 0x18); + let before = packet.clone(); + + assert_eq!(inspector.inspect_and_rewrite(&mut packet, &sink), None); + assert_eq!(packet, before); + } + + #[test] + fn rewrite_tcp_continuation_cannot_be_parsed_as_nested_frame() { + let nested = tcp_dns_framed(&svcb_response()); + let outer_len = 200usize; + let mut first_payload = Vec::with_capacity(2 + outer_len / 2); + first_payload.extend_from_slice(&(outer_len as u16).to_be_bytes()); + first_payload.resize(2 + outer_len / 2, 0xaa); + let first_data_len = first_payload.len() as u32; + let second_payload = nested; + // The nested-looking frame is actually still part of the outer DNS + // message, which remains incomplete after this continuation. + assert!(first_payload.len() + second_payload.len() < 2 + outer_len); + + let sink = CollectingSink(Mutex::new(Vec::new())); + let mut inspector = DnsInspector::default(); + let mut syn_packet = ipv4_tcp_segment(&[], 999, 0x12); + inspector.inspect_and_rewrite(&mut syn_packet, &sink); + let mut first = ipv4_tcp_segment(&first_payload, 1000, 0x18); + inspector.inspect_and_rewrite(&mut first, &sink); + let mut continuation = ipv4_tcp_segment(&second_payload, 1000 + first_data_len, 0x18); + let before = continuation.clone(); + + assert_eq!( + inspector.inspect_and_rewrite(&mut continuation, &sink), + None + ); + assert_eq!(continuation, before); + } + + #[test] + fn rewrite_ordinary_a_response_is_a_no_op_with_default_policy() { + let msg = dns_message(&[ + dns_question("ordinary.example", DNS_TYPE_A), + dns_answer_bytes("ordinary.example", DNS_TYPE_A, 300, &[203, 0, 113, 9]), + ]); + let mut packet = ipv4_udp(&msg); + let before = packet.clone(); + let sink = CollectingSink(Mutex::new(Vec::new())); + + assert_eq!(rewrite_dns_response(&mut packet, &sink), None); + assert_eq!(packet, before); + } + + #[test] + fn rewrite_non_in_svcb_answer_is_a_no_op() { + let mut msg = dns_message(&[ + dns_question("ordinary.example", DNS_TYPE_A), + dns_answer_bytes("ordinary.example", DNS_TYPE_HTTPS, 300, &[]), + ]); + let layout = parse_dns_layout(&msg).unwrap(); + let (_, answer_name_end) = read_dns_name(&msg, layout.question_end, 0).unwrap(); + msg[answer_name_end + 2..answer_name_end + 4].copy_from_slice(&2u16.to_be_bytes()); + let mut packet = ipv4_udp(&msg); + let before = packet.clone(); + let sink = CollectingSink(Mutex::new(Vec::new())); + + assert_eq!(rewrite_dns_response(&mut packet, &sink), None); + assert_eq!(packet, before); + } } diff --git a/wgbridge-rs/src/jni_bindings.rs b/wgbridge-rs/src/jni_bindings.rs index c3db2cbc6..c968fed3e 100644 --- a/wgbridge-rs/src/jni_bindings.rs +++ b/wgbridge-rs/src/jni_bindings.rs @@ -156,6 +156,30 @@ impl DnsSink for JavaDnsSink { .map(|_| ()) }); } + + fn is_domain_blocked(&self, qname: &str) -> bool { + self.0 + .with_env(|env, obj| { + let q = env.new_string(qname)?; + env.call_method( + obj, + jni_str!("isDomainBlocked"), + jni_sig!((qname: JString) -> jboolean), + &[JValue::Object(&q)], + )? + .z() + }) + .unwrap_or(false) + } + + fn blocked_rcode(&self) -> u8 { + self.0 + .with_env(|env, obj| { + env.call_method(obj, jni_str!("blockedRcode"), jni_sig!(() -> jint), &[]) + .map(|value| value.i().unwrap_or(3).clamp(0, 15) as u8) + }) + .unwrap_or(3) + } } fn tunnel_from_handle<'a>(handle: jlong) -> Option<&'a Tunnel> { diff --git a/wgbridge-rs/src/transport/ip_send.rs b/wgbridge-rs/src/transport/ip_send.rs index e54c3cfdc..11db53892 100644 --- a/wgbridge-rs/src/transport/ip_send.rs +++ b/wgbridge-rs/src/transport/ip_send.rs @@ -1,5 +1,5 @@ -//! IpSend to the VpnService TUN fd, with passive DNS inspection of the -//! decrypted inbound packets on the way through. +//! IpSend to the VpnService TUN fd, with DNS inspection and response policy +//! applied to decrypted inbound packets on the way through. use std::io; use std::os::fd::{AsRawFd, OwnedFd}; @@ -38,13 +38,21 @@ fn write_fd(fd: i32, buf: &[u8]) -> isize { impl IpSend for TunFdSend { async fn send(&mut self, packet: Packet) -> io::Result<()> { - let packet: Packet<[u8]> = packet.into(); - let data = packet.as_ref(); + let mut packet: Packet<[u8]> = packet.into(); if let Some(dns) = &self.dns { - self.dns_inspector.inspect(data, dns.as_ref()); + // The inspector records A/AAAA mappings before it blanks + // SVCB/HTTPS or a domain-blocked response. Its TCP sequence state + // also prevents continuation segments from being parsed as new + // DNS-over-TCP frames. + let data = packet.buf_mut().as_mut(); + if let Some(new_len) = self.dns_inspector.inspect_and_rewrite(data, dns.as_ref()) { + packet.truncate(new_len); + } } + let data = packet.as_ref(); + let n = write_fd(self.fd.as_raw_fd(), data); if n != data.len() as isize { // TUN write failures are transient (e.g. ENOBUFS under load) or From c33c0ad7b00d4d76b3661cdf1ea7a6be6de39413 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 10:17:40 +0000 Subject: [PATCH 2/2] Keep a zero TCP checksum unencoded when rewriting DNS A computed checksum of zero is mapped to 0xffff only for UDP, where zero means "no checksum" (RFC 768). TCP has no such convention, so encoding a zero there corrupted the rewritten segment; because every retransmission is rewritten identically, the receiver would have dropped each copy and the DNS-over-TCP lookup would hang instead of failing fast. Also drop the per-segment clone of the reassembly buffer: the residual frame can be up to 64 KiB, so cloning it for every arriving segment made frame detection quadratic in the size of a TCP response. --- wgbridge-rs/src/dns.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/wgbridge-rs/src/dns.rs b/wgbridge-rs/src/dns.rs index 9166a10ec..eb56d0a1e 100644 --- a/wgbridge-rs/src/dns.rs +++ b/wgbridge-rs/src/dns.rs @@ -162,8 +162,7 @@ impl DnsInspector { return Some(context); } if flow.framing_known { - let buffer_before = flow.buffer.clone(); - for range in complete_frame_ranges(&buffer_before, payload) { + for range in complete_frame_ranges(&flow.buffer, payload) { context.frames.push( (segment.payload_offset + payload_start + range.start) ..(segment.payload_offset + payload_start + range.end), @@ -600,7 +599,8 @@ fn repair_packet(packet: &mut [u8], view: &DnsPacketView, new_total: usize) { let old_checksum = u16::from_be_bytes([packet[checksum_offset], packet[checksum_offset + 1]]); if !(view.is_udp && view.ip_version == 4 && old_checksum == 0) { packet[checksum_offset..checksum_offset + 2].fill(0); - let checksum = encode_transport_checksum(transport_checksum(packet, view, new_total)); + let checksum = + encode_transport_checksum(transport_checksum(packet, view, new_total), view.is_udp); packet[checksum_offset..checksum_offset + 2].copy_from_slice(&checksum.to_be_bytes()); } if view.ip_version == 4 { @@ -610,8 +610,13 @@ fn repair_packet(packet: &mut [u8], view: &DnsPacketView, new_total: usize) { } } -fn encode_transport_checksum(checksum: u16) -> u16 { - if checksum == 0 { +/// Encodes a computed transport checksum for the wire. +/// +/// Only UDP maps a computed zero to `0xffff` (RFC 768: zero means "no +/// checksum"). TCP has no such convention, so a zero checksum is a valid +/// value there and rewriting it would corrupt the segment. +fn encode_transport_checksum(checksum: u16, is_udp: bool) -> u16 { + if is_udp && checksum == 0 { u16::MAX } else { checksum @@ -1333,9 +1338,13 @@ mod tests { } #[test] - fn zero_transport_checksum_is_encoded_as_ffff() { - assert_eq!(encode_transport_checksum(0), u16::MAX); - assert_eq!(encode_transport_checksum(1), 1); + fn zero_transport_checksum_is_encoded_as_ffff_for_udp_only() { + assert_eq!(encode_transport_checksum(0, true), u16::MAX); + assert_eq!(encode_transport_checksum(1, true), 1); + // Zero is a valid TCP checksum; encoding it as 0xffff would make the + // receiver drop every copy of the rewritten segment. + assert_eq!(encode_transport_checksum(0, false), 0); + assert_eq!(encode_transport_checksum(1, false), 1); } #[test]