diff options
| -rw-r--r-- | talpid-core/src/dns/macos.rs | 8 | ||||
| -rw-r--r-- | talpid-core/src/firewall/macos.rs | 122 | ||||
| -rw-r--r-- | talpid-core/src/resolver.rs | 36 | ||||
| -rw-r--r-- | talpid-core/src/tunnel_state_machine/connected_state.rs | 71 | ||||
| -rw-r--r-- | talpid-core/src/tunnel_state_machine/connecting_state.rs | 69 | ||||
| -rw-r--r-- | talpid-core/src/tunnel_state_machine/error_state.rs | 13 |
6 files changed, 207 insertions, 112 deletions
diff --git a/talpid-core/src/dns/macos.rs b/talpid-core/src/dns/macos.rs index 21c2434512..aa40c0c4bc 100644 --- a/talpid-core/src/dns/macos.rs +++ b/talpid-core/src/dns/macos.rs @@ -78,6 +78,7 @@ impl State { } } + /// Construct [`DnsSettings`] 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 5d4831816c..a45186fa23 100644 --- a/talpid-core/src/firewall/macos.rs +++ b/talpid-core/src/firewall/macos.rs @@ -1,17 +1,20 @@ -use super::{FirewallArguments, FirewallPolicy}; +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 std::{ - env, io, - net::{IpAddr, Ipv4Addr}, - ptr, -}; +use pfctl::{DropAction, FilterRuleAction, Ip, RedirectRule, Uid}; use subslice::SubsliceExt; use talpid_types::net::{ - self, AllowedEndpoint, AllowedTunnelTraffic, ALLOWED_LAN_MULTICAST_NETS, ALLOWED_LAN_NETS, + AllowedEndpoint, AllowedTunnelTraffic, TransportProtocol, ALLOWED_LAN_MULTICAST_NETS, + ALLOWED_LAN_NETS, }; +use super::{FirewallArguments, FirewallPolicy}; + pub use pfctl::Error; type Result<T> = std::result::Result<T, Error>; @@ -20,6 +23,27 @@ type Result<T> = std::result::Result<T, Error>; /// 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<bool> = 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<bool>, @@ -189,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(()) @@ -208,24 +234,44 @@ impl Firewall { &mut self, policy: &FirewallPolicy, ) -> Result<Vec<pfctl::RedirectRule>> { - 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, .. + /// Redirect DNS requests to `port`. Technically this redirects UDP on port 53 to `port`. + /// + /// For this to work as expected, please make sure a DNS resolver is running on `port`. + fn redirect_dns_to(port: u16) -> Result<Vec<RedirectRule>> { + 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_dns_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_dns_to(*dns_redirect_port)?, + FirewallPolicy::Connecting { .. } | FirewallPolicy::Connected { .. } => vec![], } }; Ok(redirect_rules) @@ -402,7 +448,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, @@ -530,10 +578,7 @@ impl Firewall { Ok(rules) } - fn get_allow_relay_rule( - &self, - relay_endpoint: &net::AllowedEndpoint, - ) -> Result<pfctl::FilterRule> { + fn get_allow_relay_rule(&self, relay_endpoint: &AllowedEndpoint) -> Result<pfctl::FilterRule> { let pfctl_proto = as_pfctl_proto(relay_endpoint.endpoint.protocol); let mut builder = self.create_rule_builder(FilterRuleAction::Pass); @@ -919,8 +964,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 @@ -931,6 +978,9 @@ impl Firewall { fn remove_anchor(&mut self) -> Result<()> { self.pf .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Scrub)?; + // Opportunistically remove Nat anchor. + // This won't fail because `try_remove_anchor` promises to convert + // `pfctl::Error::AnchorDoesNotExist` to an `Ok(())` value. self.pf .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Nat)?; self.pf @@ -941,10 +991,10 @@ impl Firewall { } } -fn as_pfctl_proto(protocol: net::TransportProtocol) -> pfctl::Proto { +fn as_pfctl_proto(protocol: TransportProtocol) -> pfctl::Proto { match protocol { - net::TransportProtocol::Udp => pfctl::Proto::Udp, - net::TransportProtocol::Tcp => pfctl::Proto::Tcp, + TransportProtocol::Udp => pfctl::Proto::Udp, + TransportProtocol::Tcp => pfctl::Proto::Tcp, } } diff --git a/talpid-core/src/resolver.rs b/talpid-core/src/resolver.rs index 555edfb3e0..77b416fb95 100644 --- a/talpid-core/src/resolver.rs +++ b/talpid-core/src/resolver.rs @@ -43,6 +43,40 @@ use hickory_server::{ }; use std::sync::LazyLock; +/// If a local DNS resolver should be used at all times. +/// +/// This setting does not affect the error or blocked state. In those states, we will want to use +/// the local DNS resoler to work around Apple's captive portals check. Exactly how this is done is +/// documented elsewhere. +pub static LOCAL_DNS_RESOLVER: LazyLock<bool> = 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 +272,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<IpAddr>) { 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 eadb313501..0ad41b049c 100644 --- a/talpid-core/src/tunnel_state_machine/connected_state.rs +++ b/talpid-core/src/tunnel_state_machine/connected_state.rs @@ -1,30 +1,29 @@ -use super::{ - AfterDisconnect, ConnectingState, DisconnectingState, ErrorState, EventConsequence, - EventResult, SharedTunnelStateValues, TunnelCommand, TunnelCommandReceiver, TunnelState, - TunnelStateTransition, -}; -use crate::{ - dns::ResolvedDnsConfig, - firewall::FirewallPolicy, - tunnel::{TunnelEvent, TunnelMetadata}, -}; -use futures::{ - channel::{mpsc, oneshot}, - stream::Fuse, - StreamExt, -}; +use futures::channel::{mpsc, oneshot}; +use futures::stream::Fuse; +use futures::StreamExt; + #[cfg(target_os = "android")] use talpid_tunnel::tun_provider::Error; -use talpid_types::{ - net::{AllowedClients, AllowedEndpoint, TunnelParameters}, - tunnel::{ErrorStateCause, FirewallPolicyError}, - BoxedError, ErrorExt, -}; +use talpid_types::net::{AllowedClients, AllowedEndpoint, TunnelParameters}; +use talpid_types::tunnel::{ErrorStateCause, FirewallPolicyError}; +use talpid_types::{BoxedError, ErrorExt}; +#[cfg(target_os = "macos")] +use crate::dns::DnsConfig; +use crate::dns::ResolvedDnsConfig; +use crate::firewall::FirewallPolicy; +#[cfg(target_os = "macos")] +use crate::resolver::LOCAL_DNS_RESOLVER; #[cfg(windows)] use crate::tunnel::TunnelMonitor; +use crate::tunnel::{TunnelEvent, TunnelMetadata}; use super::connecting_state::TunnelCloseEvent; +use super::{ + AfterDisconnect, ConnectingState, DisconnectingState, ErrorState, EventConsequence, + EventResult, SharedTunnelStateValues, TunnelCommand, TunnelCommandReceiver, TunnelState, + TunnelStateTransition, +}; pub(crate) type TunnelEventsReceiver = Fuse<mpsc::UnboundedReceiver<(TunnelEvent, oneshot::Sender<()>)>>; @@ -171,27 +170,31 @@ impl ConnectedState { // On macOS, configure only the local DNS resolver #[cfg(target_os = "macos")] - if !dns_config.is_loopback() { + // We do not want to forward DNS queries to *our* local resolver if we do not run a local + // DNS resolver *or* if the DNS config points to a loopback address. + if dns_config.is_loopback() || !*LOCAL_DNS_RESOLVER { + log::debug!("Not enabling local DNS resolver"); + shared_values + .dns_monitor + .set(&self.metadata.interface, dns_config) + .map_err(BoxedError::new)?; + } else { + log::debug!("Enabling local DNS resolver"); + // Tell local DNS resolver to start forwarding DNS queries to whatever `dns_config` + // specifies as DNS. shared_values.runtime.block_on( shared_values .filtering_resolver .enable_forward(dns_config.addresses().collect()), ); + // 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( - "lo", - crate::dns::DnsConfig::default().resolve( - &[std::net::Ipv4Addr::LOCALHOST.into()], - shared_values.filtering_resolver.listening_port(), - ), - ) - .map_err(BoxedError::new)?; - } else { - log::debug!("Not enabling DNS forwarding since loopback is used"); - 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 53ef61475e..7c7637cd20 100644 --- a/talpid-core/src/tunnel_state_machine/connecting_state.rs +++ b/talpid-core/src/tunnel_state_machine/connecting_state.rs @@ -1,34 +1,31 @@ +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex}; +use std::thread; +use std::time::{Duration, Instant}; + +use futures::channel::{mpsc, oneshot}; +use futures::future::Fuse; +use futures::{FutureExt, StreamExt}; +use talpid_routing::RouteManagerHandle; +use talpid_tunnel::tun_provider::TunProvider; +use talpid_tunnel::{EventHook, TunnelArgs, TunnelEvent, TunnelMetadata}; +use talpid_types::net::{AllowedClients, AllowedEndpoint, AllowedTunnelTraffic, TunnelParameters}; +use talpid_types::tunnel::{ErrorStateCause, FirewallPolicyError}; +use talpid_types::ErrorExt; + +use super::connected_state::TunnelEventsReceiver; use super::{ AfterDisconnect, ConnectedState, DisconnectingState, ErrorState, EventConsequence, EventResult, SharedTunnelStateValues, TunnelCommand, TunnelCommandReceiver, TunnelState, TunnelStateTransition, }; -use crate::{ - firewall::FirewallPolicy, - tunnel::{self, TunnelMonitor}, -}; -use futures::{ - channel::{mpsc, oneshot}, - future::Fuse, - FutureExt, StreamExt, -}; -use std::{ - path::{Path, PathBuf}, - sync::{Arc, Mutex}, - thread, - time::{Duration, Instant}, -}; -use talpid_routing::RouteManagerHandle; -use talpid_tunnel::{ - tun_provider::TunProvider, EventHook, TunnelArgs, TunnelEvent, TunnelMetadata, -}; -use talpid_types::{ - net::{AllowedClients, AllowedEndpoint, AllowedTunnelTraffic, TunnelParameters}, - tunnel::{ErrorStateCause, FirewallPolicyError}, - ErrorExt, -}; -use super::connected_state::TunnelEventsReceiver; +#[cfg(target_os = "macos")] +use crate::dns::DnsConfig; +use crate::firewall::FirewallPolicy; +#[cfg(target_os = "macos")] +use crate::resolver::LOCAL_DNS_RESOLVER; +use crate::tunnel::{self, TunnelMonitor}; pub(crate) type TunnelCloseEvent = Fuse<oneshot::Receiver<Option<ErrorStateCause>>>; @@ -57,17 +54,23 @@ impl ConnectingState { retry_attempt: u32, ) -> (Box<dyn TunnelState>, 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 bdd8e9a014..0cf392a0e5 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( |