Fix VPN DNS overlap (and missing removal) with the currently connected LAN

Author: pansen

src/userspace_helper.rs

@@ -1723,6 +1723,27 @@ fn dns_target_services(policy: DnsPolicy, fp: &MacosDnsFingerprint) -> Vec<Strin
     }
 }
 
+/// A configured tunnel DNS server whose address falls inside a directly-connected
+/// LAN subnet. When that happens the route covering the server is excluded from
+/// the tunnel (see [`macos_route_excluded_by_local_subnet`]), so the server is no
+/// longer reachable through the tunnel: queries would leak onto the LAN and hit
+/// whatever host happens to hold that address. Returns the offending server and
+/// the subnet it collides with so the reconciler can pull tunnel DNS back.
+#[cfg(target_os = "macos")]
+fn macos_dns_server_in_local_subnet(
+    dns_servers: &[String],
+    local_subnets: &[(IpAddr, u8)],
+) -> Option<(IpAddr, (IpAddr, u8))> {
+    dns_servers.iter().find_map(|server| {
+        let ip: IpAddr = server.parse().ok()?;
+        let host_prefix = if ip.is_ipv4() { 32 } else { 128 };
+        local_subnets
+            .iter()
+            .find(|local| macos_subnet_contains(local, &(ip, host_prefix)))
+            .map(|local| (ip, *local))
+    })
+}
+
 /// The pure decision behind a DNS reconcile: given the tunnel's DNS, the freshly
 /// observed environment, the services we currently own, and the services we want
 /// to own, decide what to do. Kept free of I/O so it is unit-testable.
@@ -1838,11 +1859,21 @@ fn macos_reconcile_dns(state: &MacosCleanupState) -> bool {
 
     let fingerprint = macos_current_dns_fingerprint();
 
+    // If the tunnel's own DNS server now lives inside a directly-connected LAN,
+    // its route is excluded from the tunnel and the server is unreachable through
+    // it. We must pull tunnel DNS back, but joining such a LAN need not change the
+    // DNS fingerprint at all, so this is computed before the identity guard below.
+    let local_subnets = macos_local_connected_subnets(&inputs.interface);
+    let collision = macos_dns_server_in_local_subnet(&inputs.dns_servers, &local_subnets);
+
     let mut dns = state
         .dns
         .lock()
         .unwrap_or_else(|poisoned| poisoned.into_inner());
-    if fingerprint == dns.fingerprint {
+    // Normally skip when the DNS environment is unchanged. But still act when a
+    // collision means we own services that now need restoring to their originals.
+    let must_pull_back = collision.is_some() && !dns.services.is_empty();
+    if fingerprint == dns.fingerprint && !must_pull_back {
         return false; // cheap identity check, same guard as routes.
     }
 
@@ -1856,7 +1887,21 @@ fn macos_reconcile_dns(state: &MacosCleanupState) -> bool {
         "userspace_helper_dns_change_detected"
     );
 
-    let targets = dns_target_services(DNS_POLICY, &fingerprint);
+    // On a collision, target nothing so every owned service is restored to its
+    // original DNS rather than left pointing at an unreachable (or
+    // LAN-impersonatable) address.
+    let targets = match collision {
+        Some((server, (subnet_ip, subnet_prefix))) => {
+            warn!(
+                interface = inputs.interface,
+                dns_server = %server,
+                local_subnet = %format!("{subnet_ip}/{subnet_prefix}"),
+                "userspace_helper_dns_unreachable_local_lan"
+            );
+            Vec::new()
+        }
+        None => dns_target_services(DNS_POLICY, &fingerprint),
+    };
     let owned: Vec<String> = dns.services.iter().map(|s| s.service.clone()).collect();
     let actions = plan_dns_actions(&inputs.dns_servers, &fingerprint, &owned, &targets);
 
@@ -2099,11 +2144,19 @@ fn format_macos_network_overview_table(
         &macos_route_overview_rows(routes),
     ));
     lines.push(String::new());
-    lines.push(format!(
+    let mut dns_header = format!(
         "DNS tunnel_servers={} primary_service={}",
         format_list(&inputs.dns_servers),
         dns_fingerprint.primary_service.as_deref().unwrap_or("none")
-    ));
+    );
+    if let Some((server, (subnet_ip, subnet_prefix))) =
+        macos_dns_server_in_local_subnet(&inputs.dns_servers, &route_fingerprint.local_subnets)
+    {
+        dns_header.push_str(&format!(
+            " UNREACHABLE: {server} is inside local LAN {subnet_ip}/{subnet_prefix}, tunnel DNS pulled back"
+        ));
+    }
+    lines.push(dns_header);
     lines.extend(format_table_with_dimmed_rows(
         &[
             "SERVICE",
@@ -2554,6 +2607,31 @@ mod tests {
         assert!(macos_route_excluded_by_local_subnet(&gatewayed, &[local]).is_none());
     }
 
+    #[test]
+    fn dns_server_collision_with_local_subnet_is_detected() {
+        let lan = subnet("55.56.57.0/24");
+
+        // Tunnel DNS sits inside the joined LAN -> unreachable through the tunnel.
+        let hit = macos_dns_server_in_local_subnet(&["55.56.57.2".to_string()], &[lan]);
+        assert_eq!(hit, Some(("55.56.57.2".parse().unwrap(), lan)));
+
+        // A DNS server outside every connected subnet is fine.
+        assert!(
+            macos_dns_server_in_local_subnet(&["9.9.9.9".to_string()], &[lan]).is_none(),
+            "off-LAN DNS must not be treated as a collision"
+        );
+
+        // No connected subnets at all -> never a collision.
+        assert!(macos_dns_server_in_local_subnet(&["55.56.57.2".to_string()], &[]).is_none());
+
+        // Any colliding server in the list triggers pull-back; reports the first.
+        let mixed = macos_dns_server_in_local_subnet(
+            &["9.9.9.9".to_string(), "55.56.57.2".to_string()],
+            &[lan],
+        );
+        assert_eq!(mixed, Some(("55.56.57.2".parse().unwrap(), lan)));
+    }
+
     // --- DNS reconciler ---
 
     fn dns_fp(primary: Option<&str>, observed: &[(&str, Option<&[&str]>)]) -> MacosDnsFingerprint {