From a06f5a7995e3b2305e6f49f347b887490c51ec70 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Wed, 27 Nov 2024 14:09:25 +0100 Subject: [PATCH] Disable Apple services workaroudns for unaffected macOS versions Do not apply redirect rules for DNS requests to port 53 to loopback on macOS versions that do not need the apple services NAT-redirect workaround. This effectively reverts to the old behaviour were a local DNS resolver is used only in the blocked and connecting states for macOS versions *not* in the version range 14.6 <= version < 15.1. Revert the nat redirect rules that were added to force traffic through the tunnel interface. This hack is no longer needed since it was fixed by apple in macOS 15.1. --- talpid-core/src/dns/macos.rs | 8 +- talpid-core/src/firewall/macos.rs | 95 ++++++++++++++----- talpid-core/src/resolver.rs | 35 ++++++- .../tunnel_state_machine/connected_state.rs | 37 ++++---- .../tunnel_state_machine/connecting_state.rs | 24 +++-- .../src/tunnel_state_machine/error_state.rs | 13 ++- 6 files changed, 155 insertions(+), 57 deletions(-) diff --git a/talpid-core/src/dns/macos.rs b/talpid-core/src/dns/macos.rs index 21c243451262..7d8170aea47f 100644 --- a/talpid-core/src/dns/macos.rs +++ b/talpid-core/src/dns/macos.rs @@ -78,6 +78,7 @@ impl State { } } + /// Construct a [DnsConfig] from the arguments and apply the desired addresses to all network services. fn apply_new_config( &mut self, store: &SCDynamicStore, @@ -375,8 +376,9 @@ impl InterfaceSettings { unsafe impl Send for InterfaceSettings {} pub struct DnsMonitor { + /// The backing "System Configuration framework" store, which allow us to access and detect + /// changes to the device's network configuration. store: SCDynamicStore, - /// The current DNS injection state. If this is `None` it means we are not injecting any DNS. /// When it's `Some(state)` we are actively making sure `state.dns_settings` is configured /// on all network interfaces. @@ -403,6 +405,10 @@ impl super::DnsMonitorT for DnsMonitor { }) } + /// Update the system config to use the DNS [config]. + /// + /// Note that the [interface] parameter does nothing on macOS. Since we can't configure DNS + /// on the tunnel interface, we have to configure all interfaces. fn set(&mut self, interface: &str, config: ResolvedDnsConfig) -> Result<()> { let port = config.port; let servers: Vec<_> = config.addresses().collect(); diff --git a/talpid-core/src/firewall/macos.rs b/talpid-core/src/firewall/macos.rs index 53d9ebb8f634..39a034bfcb9b 100644 --- a/talpid-core/src/firewall/macos.rs +++ b/talpid-core/src/firewall/macos.rs @@ -2,10 +2,11 @@ use std::env; use std::io; use std::net::{IpAddr, Ipv4Addr}; use std::ptr; +use std::sync::LazyLock; use ipnetwork::IpNetwork; use libc::{c_int, sysctlbyname}; -use pfctl::{DropAction, FilterRuleAction, Ip, Uid}; +use pfctl::{DropAction, FilterRuleAction, Ip, RedirectRule, Uid}; use subslice::SubsliceExt; use talpid_types::net::{ AllowedEndpoint, AllowedTunnelTraffic, TransportProtocol, ALLOWED_LAN_MULTICAST_NETS, @@ -22,6 +23,27 @@ type Result = std::result::Result; /// replaced by allowing the anchor name to be configured from the public API of this crate. const ANCHOR_NAME: &str = "mullvad"; +/// If NAT firewall rules should be applied to force Apple services through the tunnel. +/// +/// macOS versions 14.6 <= x < 15.1 were affected by a bug where Apple services tried to bypass the +/// tunnel by going out on the physical interface instead. To mitigate this and force all traffic +/// to go through the tunnel we added NAT filtering rules to redirect traffic all deviating traffic +/// to the tunnel. +/// +/// This is not something that we deem is necessary otherwise, and as such we disable NAT filtering +/// on macOS versions that are unaffected by this naughty bug, but keep it were it is necessary for +/// Apple services to function properly together with a VPN. +pub static NAT_WORKAROUND: LazyLock = LazyLock::new(|| { + use talpid_platform_metadata::MacosVersion; + let version = MacosVersion::new().expect("Could not detect macOS version"); + let v = |s| MacosVersion::from_raw_version(s).unwrap(); + let apply_workaround = v("14.6") <= version && version < v("15.1"); + if apply_workaround { + log::debug!("Using NAT redirect workaround"); + }; + apply_workaround +}); + pub struct Firewall { pf: pfctl::PfCtl, pf_was_enabled: Option, @@ -191,7 +213,9 @@ impl Firewall { anchor_change.set_scrub_rules(Self::get_scrub_rules()?); anchor_change.set_filter_rules(new_filter_rules); anchor_change.set_redirect_rules(self.get_dns_redirect_rules(policy)?); - anchor_change.set_nat_rules(self.get_nat_rules(policy)?); + if *NAT_WORKAROUND { + anchor_change.set_nat_rules(self.get_nat_rules(policy)?); + } self.pf.set_rules(ANCHOR_NAME, anchor_change)?; Ok(()) @@ -210,24 +234,41 @@ impl Firewall { &mut self, policy: &FirewallPolicy, ) -> Result> { - let redirect_rules = match policy { - FirewallPolicy::Connected { dns_config, .. } if dns_config.is_loopback() => vec![], - FirewallPolicy::Blocked { - dns_redirect_port, .. - } - | FirewallPolicy::Connecting { - dns_redirect_port, .. + fn redirect_to(port: u16) -> Result> { + let redirect_dns = pfctl::RedirectRuleBuilder::default() + .action(pfctl::RedirectRuleAction::Redirect) + .interface("lo0") + .proto(pfctl::Proto::Udp) + .to(pfctl::Port::from(53)) + .redirect_to(pfctl::Port::from(port)) + .build()?; + Ok(vec![redirect_dns]) + } + + let redirect_rules = if *crate::resolver::LOCAL_DNS_RESOLVER { + match policy { + FirewallPolicy::Connected { dns_config, .. } if dns_config.is_loopback() => { + vec![] + } + FirewallPolicy::Blocked { + dns_redirect_port, .. + } + | FirewallPolicy::Connecting { + dns_redirect_port, .. + } + | FirewallPolicy::Connected { + dns_redirect_port, .. + } => redirect_to(*dns_redirect_port)?, } - | FirewallPolicy::Connected { - dns_redirect_port, .. - } => { - vec![pfctl::RedirectRuleBuilder::default() - .action(pfctl::RedirectRuleAction::Redirect) - .interface("lo0") - .proto(pfctl::Proto::Udp) - .to(pfctl::Port::from(53)) - .redirect_to(pfctl::Port::from(*dns_redirect_port)) - .build()?] + } else { + // Only apply redirect rules in the blocked state if we should *not* use our local DNS + // resolver, since it will be running in the blocked state to work with Apple's captive + // portal check. + match policy { + FirewallPolicy::Blocked { + dns_redirect_port, .. + } => redirect_to(*dns_redirect_port)?, + FirewallPolicy::Connecting { .. } | FirewallPolicy::Connected { .. } => vec![], } }; Ok(redirect_rules) @@ -404,7 +445,9 @@ impl Firewall { &mut self.get_split_tunnel_rules(&tunnel.interface, redirect_interface)?, ); } else { - rules.push(self.route_everything_to(&tunnel.interface)?); + if *NAT_WORKAROUND { + rules.push(self.route_everything_to(&tunnel.interface)?); + } rules.extend(self.get_allow_tunnel_rules( tunnel.interface.as_str(), &AllowedTunnelTraffic::All, @@ -918,8 +961,10 @@ impl Firewall { fn add_anchor(&mut self) -> Result<()> { self.pf .try_add_anchor(ANCHOR_NAME, pfctl::AnchorKind::Scrub)?; - self.pf - .try_add_anchor(ANCHOR_NAME, pfctl::AnchorKind::Nat)?; + if *NAT_WORKAROUND { + self.pf + .try_add_anchor(ANCHOR_NAME, pfctl::AnchorKind::Nat)?; + } self.pf .try_add_anchor(ANCHOR_NAME, pfctl::AnchorKind::Filter)?; self.pf @@ -930,8 +975,10 @@ impl Firewall { fn remove_anchor(&mut self) -> Result<()> { self.pf .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Scrub)?; - self.pf - .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Nat)?; + // Opportunistically remove Nat anchor. + let _ = self + .pf + .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Nat); self.pf .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Redirect)?; self.pf diff --git a/talpid-core/src/resolver.rs b/talpid-core/src/resolver.rs index 555edfb3e068..57ddaf2cbeef 100644 --- a/talpid-core/src/resolver.rs +++ b/talpid-core/src/resolver.rs @@ -43,6 +43,39 @@ use hickory_server::{ }; use std::sync::LazyLock; +/// If a local DNS resolver should be used at all times. +/// +/// Not that we (probably) want to use a local DNS resolver at least in the connecting and blocked +/// state to make the app work with captive portals. +pub static LOCAL_DNS_RESOLVER: LazyLock = LazyLock::new(|| { + use talpid_platform_metadata::MacosVersion; + let version = MacosVersion::new().expect("Could not detect macOS version"); + let v = |s| MacosVersion::from_raw_version(s).unwrap(); + // Apple services tried to perform DNS lookups on the physical interface on some macOS + // versions, so we added redirect rules to always redirect DNS to our local DNS resolver. + // This seems to break some apps which do not like that we redirect DNS on port 53 to our local + // DNS resolver running on some other, arbitrary port, and so we disable this behaviour on + // macOS versions that are unaffected by this naughty bug. + // + // The workaround should only be applied to the affected macOS versions because some programs + // set the `skip filtering` pf flag on loopback, which meant that the pf filtering would break + // unexpectedly. We could clear the `skip filtering` flag to force pf filtering on loopback, + // but apparently it is good practice to enable `skip filtering` on loopback so we decided + // against this. Source: https://www.openbsd.org/faq/pf/filter.html + // + // It should be noted that most programs still works fine with this workaround enabled. Notably + // programs that use `getaddrinfo` would behave correctly when we redirect DNS to our local + // resolver, while some programs always used port 53 no matter what (nslookup for example). + // Also, most programs don't set the `skip filtering` pf flag on loopback, but some notable + // ones do for some reason. Orbstack is one such example, which meant that people running + // containers would run into the aforementioned issue. + let use_local_dns_resolver = v("14.6") <= version && version < v("15.1"); + if use_local_dns_resolver { + log::debug!("Using local DNS resolver"); + } + use_local_dns_resolver +}); + const ALLOWED_RECORD_TYPES: &[RecordType] = &[RecordType::A, RecordType::CNAME]; const CAPTIVE_PORTAL_DOMAINS: &[&str] = &["captive.apple.com", "netcts.cdn-apple.com"]; @@ -238,7 +271,7 @@ impl ResolverHandle { self.listening_port } - /// Set the DNS server to forward queries to + /// Set the DNS server to forward queries to [dns_servers] pub async fn enable_forward(&self, dns_servers: Vec) { let (response_tx, response_rx) = oneshot::channel(); let _ = self.tx.unbounded_send(ResolverMessage::SetConfig { diff --git a/talpid-core/src/tunnel_state_machine/connected_state.rs b/talpid-core/src/tunnel_state_machine/connected_state.rs index eadb313501e5..463f713075cc 100644 --- a/talpid-core/src/tunnel_state_machine/connected_state.rs +++ b/talpid-core/src/tunnel_state_machine/connected_state.rs @@ -4,8 +4,9 @@ use super::{ TunnelStateTransition, }; use crate::{ - dns::ResolvedDnsConfig, + dns::{DnsConfig, ResolvedDnsConfig}, firewall::FirewallPolicy, + resolver::LOCAL_DNS_RESOLVER, tunnel::{TunnelEvent, TunnelMetadata}, }; use futures::{ @@ -171,27 +172,31 @@ impl ConnectedState { // On macOS, configure only the local DNS resolver #[cfg(target_os = "macos")] - if !dns_config.is_loopback() { - shared_values.runtime.block_on( - shared_values - .filtering_resolver - .enable_forward(dns_config.addresses().collect()), - ); + // We do not want to forward DNS queries to *our* local resolver the DNS config + // is a loopback address. This is also true if we do not run our local DNS resolver. + if dns_config.is_loopback() || !*LOCAL_DNS_RESOLVER { + log::debug!("Not enabling DNS forwarding"); shared_values .dns_monitor - .set( - "lo", - crate::dns::DnsConfig::default().resolve( - &[std::net::Ipv4Addr::LOCALHOST.into()], - shared_values.filtering_resolver.listening_port(), - ), - ) + .set(&self.metadata.interface, dns_config) .map_err(BoxedError::new)?; } else { - log::debug!("Not enabling DNS forwarding since loopback is used"); + log::debug!("Enabling DNS forwarding"); + let local_resolver_config = dns_config.addresses().collect(); + // Forward all DNS requests to our local DNS resolver + shared_values.runtime.block_on( + shared_values + .filtering_resolver + .enable_forward(local_resolver_config), + ); + // Set system DNS to our local DNS resolver + let system_dns = DnsConfig::default().resolve( + &[std::net::Ipv4Addr::LOCALHOST.into()], + shared_values.filtering_resolver.listening_port(), + ); shared_values .dns_monitor - .set(&self.metadata.interface, dns_config) + .set("lo", system_dns) .map_err(BoxedError::new)?; } diff --git a/talpid-core/src/tunnel_state_machine/connecting_state.rs b/talpid-core/src/tunnel_state_machine/connecting_state.rs index 7387d51c03f4..c5f165572253 100644 --- a/talpid-core/src/tunnel_state_machine/connecting_state.rs +++ b/talpid-core/src/tunnel_state_machine/connecting_state.rs @@ -4,7 +4,9 @@ use super::{ TunnelStateTransition, }; use crate::{ + dns::DnsConfig, firewall::FirewallPolicy, + resolver::LOCAL_DNS_RESOLVER, tunnel::{self, TunnelMonitor}, }; use futures::{ @@ -55,17 +57,23 @@ impl ConnectingState { retry_attempt: u32, ) -> (Box, TunnelStateTransition) { #[cfg(target_os = "macos")] - if let Err(err) = shared_values.dns_monitor.set( - "lo", - crate::dns::DnsConfig::default().resolve( + if *LOCAL_DNS_RESOLVER { + // Set system DNS to our local DNS resolver + let system_dns = DnsConfig::default().resolve( &[std::net::Ipv4Addr::LOCALHOST.into()], shared_values.filtering_resolver.listening_port(), - ), - ) { - log::error!( - "{}", - err.display_chain_with_msg("Failed to configure system to use filtering resolver") ); + let _ = shared_values + .dns_monitor + .set("lo", system_dns) + .inspect_err(|err| { + log::error!( + "{}", + err.display_chain_with_msg( + "Failed to configure system to use filtering resolver" + ) + ); + }); } if shared_values.connectivity.is_offline() { diff --git a/talpid-core/src/tunnel_state_machine/error_state.rs b/talpid-core/src/tunnel_state_machine/error_state.rs index bdd8e9a014f8..0cf392a0e572 100644 --- a/talpid-core/src/tunnel_state_machine/error_state.rs +++ b/talpid-core/src/tunnel_state_machine/error_state.rs @@ -36,13 +36,12 @@ impl ErrorState { #[cfg(target_os = "macos")] if !block_reason.prevents_filtering_resolver() { - if let Err(err) = shared_values.dns_monitor.set( - "lo", - DnsConfig::default().resolve( - &[Ipv4Addr::LOCALHOST.into()], - shared_values.filtering_resolver.listening_port(), - ), - ) { + // Set system DNS to our local DNS resolver + let system_dns = DnsConfig::default().resolve( + &[Ipv4Addr::LOCALHOST.into()], + shared_values.filtering_resolver.listening_port(), + ); + if let Err(err) = shared_values.dns_monitor.set("lo", system_dns) { log::error!( "{}", err.display_chain_with_msg(