Skip to content

Commit

Permalink
Merge remote-tracking branch 'origin/macos-fix-dns-redir-backport' in…
Browse files Browse the repository at this point in the history 
…to prepare-2024.7
  • Loading branch information
@dlon
dlon committed Oct 24, 2024
2 parents ef6c862 + 41487a0 commit f07fbf0
Show file tree
Hide file tree
Showing 7 changed files with 78 additions and 10 deletions.
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ Line wrap the file at 100 chars. Th
* **Security**: in case of vulnerabilities.

## [Unreleased]
### Fixed
#### macOS
- Fix DNS not working due to broken PF redirect.


## [2024.6] - 2024-10-23
### Fixed
#### macOS
- Disable DNS redirect when custom DNS is set to localhost.


## [2024.6] - 2024-10-23
Expand Down
43 changes: 39 additions & 4 deletions talpid-core/src/dns/macos.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,24 @@ use system_configuration::{
array::CFArray,
base::{CFType, TCFType, ToVoid},
dictionary::{CFDictionary, CFMutableDictionary},
number::CFNumber,
propertylist::CFPropertyList,
runloop::{kCFRunLoopCommonModes, CFRunLoop},
string::CFString,
},
dynamic_store::{SCDynamicStore, SCDynamicStoreBuilder, SCDynamicStoreCallBackContext},
sys::schema_definitions::{kSCPropNetDNSServerAddresses, kSCPropNetInterfaceDeviceName},
sys::schema_definitions::{
kSCPropNetDNSServerAddresses, kSCPropNetDNSServerPort, kSCPropNetInterfaceDeviceName,
},
};
use talpid_routing::debounce::BurstGuard;

use super::ResolvedDnsConfig;

pub type Result<T> = std::result::Result<T, Error>;

const DNS_PORT: u16 = 53;

/// Errors that can happen when setting/monitoring DNS on macOS.
#[derive(thiserror::Error, Debug)]
pub enum Error {
Expand Down Expand Up @@ -78,11 +83,13 @@ impl State {
store: &SCDynamicStore,
interface: &str,
servers: &[IpAddr],
port: u16,
) -> Result<()> {
talpid_types::detect_flood!();

let servers: Vec<DnsServer> = servers.iter().map(|ip| ip.to_string()).collect();
let new_settings = DnsSettings::from_server_addresses(&servers, interface.to_string());
let new_settings =
DnsSettings::from_server_addresses(&servers, interface.to_string(), port);
match &self.dns_settings {
None => {
self.dns_settings = Some(new_settings);
Expand Down Expand Up @@ -221,7 +228,7 @@ struct DnsSettings {
unsafe impl Send for DnsSettings {}

impl DnsSettings {
pub fn from_server_addresses(server_addresses: &[DnsServer], name: String) -> Self {
pub fn from_server_addresses(server_addresses: &[DnsServer], name: String, port: u16) -> Self {
let mut mut_dict = CFMutableDictionary::new();
if !server_addresses.is_empty() {
let cf_string_servers: Vec<CFString> =
Expand All @@ -233,6 +240,14 @@ impl DnsSettings {
&server_addresses_key.to_void(),
&server_addresses_value.to_void(),
);

// Set port if non-standard
if port != DNS_PORT {
let server_port_key =
unsafe { CFString::wrap_under_get_rule(kSCPropNetDNSServerPort) };
let server_port_value = CFNumber::from(i32::from(port));
mut_dict.add(&server_port_key.to_void(), &server_port_value.to_void());
}
}
let dict = mut_dict.to_immutable();
DnsSettings { dict, name }
Expand Down Expand Up @@ -370,10 +385,11 @@ impl super::DnsMonitorT for DnsMonitor {
}

fn set(&mut self, interface: &str, config: ResolvedDnsConfig) -> Result<()> {
let port = config.port;
let servers: Vec<_> = config.addresses().collect();

let mut state = self.state.lock();
state.apply_new_config(&self.store, interface, &servers)
state.apply_new_config(&self.store, interface, &servers, port)
}

fn reset(&mut self) -> Result<()> {
Expand Down Expand Up @@ -508,6 +524,8 @@ fn state_to_setup_path(state_path: &str) -> Option<String> {

#[cfg(test)]
mod test {
use crate::dns::imp::DNS_PORT;

use super::{DnsSettings, State};
use std::collections::{BTreeSet, HashMap};

Expand All @@ -523,6 +541,7 @@ mod test {
Some(DnsSettings::from_server_addresses(
&["1.2.3.4".to_owned()],
"iface_b".to_owned(),
DNS_PORT,
)),
),
// One of our states already equals the desired state. It should be stored regardless.
Expand All @@ -531,6 +550,7 @@ mod test {
Some(DnsSettings::from_server_addresses(
&["10.64.0.1".to_owned()],
"iface_c".to_owned(),
DNS_PORT,
)),
),
]);
Expand All @@ -552,20 +572,23 @@ mod test {
Some(DnsSettings::from_server_addresses(
&["1.2.3.4".to_owned()],
"iface_b".to_owned(),
DNS_PORT,
)),
),
(
"c".to_owned(),
Some(DnsSettings::from_server_addresses(
&["10.64.0.1".to_owned()],
"iface_c".to_owned(),
DNS_PORT,
)),
),
(
"d".to_owned(),
Some(DnsSettings::from_server_addresses(
&["1.3.3.7".to_owned()],
"iface_d".to_owned(),
DNS_PORT,
)),
),
]);
Expand All @@ -576,6 +599,7 @@ mod test {
Some(DnsSettings::from_server_addresses(
&["10.64.0.1".to_owned()],
"iface_a".to_owned(),
DNS_PORT,
)),
),
// This change should be ignored
Expand All @@ -584,6 +608,7 @@ mod test {
Some(DnsSettings::from_server_addresses(
&["10.64.0.1".to_owned()],
"iface_b".to_owned(),
DNS_PORT,
)),
),
// This change should be ignored
Expand All @@ -592,6 +617,7 @@ mod test {
Some(DnsSettings::from_server_addresses(
&["4.3.2.1".to_owned()],
"iface_c".to_owned(),
DNS_PORT,
)),
),
// This change should NOT be ignored
Expand All @@ -600,6 +626,7 @@ mod test {
Some(DnsSettings::from_server_addresses(
&["4.3.2.1".to_owned()],
"iface_d".to_owned(),
DNS_PORT,
)),
),
]);
Expand All @@ -610,20 +637,23 @@ mod test {
Some(DnsSettings::from_server_addresses(
&["1.2.3.4".to_owned()],
"iface_b".to_owned(),
DNS_PORT,
)),
),
(
"c".to_owned(),
Some(DnsSettings::from_server_addresses(
&["4.3.2.1".to_owned()],
"iface_c".to_owned(),
DNS_PORT,
)),
),
(
"d".to_owned(),
Some(DnsSettings::from_server_addresses(
&["4.3.2.1".to_owned()],
"iface_d".to_owned(),
DNS_PORT,
)),
),
]);
Expand All @@ -644,13 +674,15 @@ mod test {
Some(DnsSettings::from_server_addresses(
&["10.64.0.1".to_owned()],
"iface_a".to_owned(),
DNS_PORT,
)),
),
(
"b".to_owned(),
Some(DnsSettings::from_server_addresses(
&["1.2.3.4".to_owned()],
"iface_b".to_owned(),
DNS_PORT,
)),
),
("c".to_owned(), None),
Expand All @@ -677,20 +709,23 @@ mod test {
Some(DnsSettings::from_server_addresses(
&["192.168.100.1".to_owned()],
"iface_a".to_owned(),
DNS_PORT,
)),
)]);
let new_state = HashMap::from([(
"a".to_owned(),
Some(DnsSettings::from_server_addresses(
&["192.168.1.1".to_owned()],
"iface_a".to_owned(),
DNS_PORT,
)),
)]);
let expect_state = HashMap::from([(
"a".to_owned(),
Some(DnsSettings::from_server_addresses(
&["192.168.1.1".to_owned()],
"iface_a".to_owned(),
DNS_PORT,
)),
)]);

Expand Down
13 changes: 12 additions & 1 deletion talpid-core/src/dns/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,18 +67,26 @@ enum InnerDnsConfig {
}

impl DnsConfig {
pub(crate) fn resolve(&self, default_tun_config: &[IpAddr]) -> ResolvedDnsConfig {
pub(crate) fn resolve(
&self,
default_tun_config: &[IpAddr],
#[cfg(target_os = "macos")] port: u16,
) -> ResolvedDnsConfig {
match &self.config {
InnerDnsConfig::Default => ResolvedDnsConfig {
tunnel_config: default_tun_config.to_owned(),
non_tunnel_config: vec![],
#[cfg(target_os = "macos")]
port,
},
InnerDnsConfig::Override {
tunnel_config,
non_tunnel_config,
} => ResolvedDnsConfig {
tunnel_config: tunnel_config.to_owned(),
non_tunnel_config: non_tunnel_config.to_owned(),
#[cfg(target_os = "macos")]
port,
},
}
}
Expand All @@ -93,6 +101,9 @@ pub struct ResolvedDnsConfig {
/// For the most part, the tunnel state machine will not handle any of this configuration
/// on non-tunnel interface, only allow them in the firewall.
non_tunnel_config: Vec<IpAddr>,
/// Port to use
#[cfg(target_os = "macos")]
port: u16,
}

impl fmt::Display for ResolvedDnsConfig {
Expand Down
8 changes: 6 additions & 2 deletions talpid-core/src/tunnel_state_machine/connected_state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,15 @@ impl ConnectedState {
metadata: &TunnelMetadata,
shared_values: &SharedTunnelStateValues,
) -> ResolvedDnsConfig {
shared_values.dns_config.resolve(&metadata.gateways())
shared_values.dns_config.resolve(
&metadata.gateways(),
#[cfg(target_os = "macos")]
53,
)
}

fn set_dns(&self, shared_values: &mut SharedTunnelStateValues) -> Result<(), BoxedError> {
let dns_config = Self::resolve_dns(&self.metadata, shared_values);
let dns_config: ResolvedDnsConfig = Self::resolve_dns(&self.metadata, shared_values);

#[cfg(not(target_os = "macos"))]
shared_values
Expand Down
5 changes: 4 additions & 1 deletion talpid-core/src/tunnel_state_machine/connecting_state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ impl ConnectingState {
#[cfg(target_os = "macos")]
if let Err(err) = shared_values.dns_monitor.set(
"lo",
crate::dns::DnsConfig::default().resolve(&[std::net::Ipv4Addr::LOCALHOST.into()]),
crate::dns::DnsConfig::default().resolve(
&[std::net::Ipv4Addr::LOCALHOST.into()],
shared_values.filtering_resolver.listening_port(),
),
) {
log::error!(
"{}",
Expand Down
5 changes: 4 additions & 1 deletion talpid-core/src/tunnel_state_machine/disconnected_state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,10 @@ impl DisconnectedState {

shared_values.dns_monitor.set(
"lo",
dns::DnsConfig::default().resolve(&[Ipv4Addr::LOCALHOST.into()]),
dns::DnsConfig::default().resolve(
&[Ipv4Addr::LOCALHOST.into()],
shared_values.filtering_resolver.listening_port(),
),
)
}
}
Expand Down
5 changes: 4 additions & 1 deletion talpid-core/src/tunnel_state_machine/error_state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ impl ErrorState {
if !block_reason.prevents_filtering_resolver() {
if let Err(err) = shared_values.dns_monitor.set(
"lo",
DnsConfig::default().resolve(&[Ipv4Addr::LOCALHOST.into()]),
DnsConfig::default().resolve(
&[Ipv4Addr::LOCALHOST.into()],
shared_values.filtering_resolver.listening_port(),
),
) {
log::error!(
"{}",
Expand Down

0 comments on commit f07fbf0

Please sign in to comment.