Rework firewall exclusion rules

9 files changed, 138 insertions, 167 deletions

diff --git a/mullvad-daemon/src/exclusion_gid.rs b/mullvad-daemon/src/exclusion_gid.rs
index 441b66cb2b..ec87c5a7c6 100644
--- a/mullvad-daemon/src/exclusion_gid.rs
+++ b/mullvad-daemon/src/exclusion_gid.rs
@@ -1,23 +1,18 @@
-use std::ffi::CStr;
+use std::{ffi::CStr, io};
 /// name of the group that should be excluded
 const EXCLUSION_GROUP: &[u8] = b"mullvad-exclusion\0";
 
 /// Returns the GID of `mullvad-exclusion` group if it exists.
-pub fn get_exclusion_gid() -> Option<u32> {
-    let exclusion_group_name = unsafe { CStr::from_bytes_with_nul_unchecked(EXCLUSION_GROUP) };
-    talpid_core::macos::get_group_id(exclusion_group_name)
+pub fn get_exclusion_gid() -> io::Result<u32> {
+    let exclusion_group_name = CStr::from_bytes_with_nul(EXCLUSION_GROUP).unwrap();
+    get_group_id(exclusion_group_name)
 }
 
 /// Attempts to set the GID of the current process to `mullvad-exclusion`.
-#[cfg(target_os = "macos")]
-pub fn set_exclusion_gid() {
-    if let Some(gid) = get_exclusion_gid() {
-        if let Err(err) = talpid_core::macos::set_gid(gid) {
-            log::error!("Failed to set group ID: {}", err);
-        }
-    } else {
-        log::error!("No exclusion ID available");
-    }
+pub fn set_exclusion_gid() -> io::Result<u32> {
+    let gid = get_exclusion_gid()?;
+    set_gid(gid)?;
+    Ok(gid)
 }
 
 #[cfg(test)]
@@ -27,3 +22,32 @@ mod test {
         let _ = super::get_exclusion_gid();
     }
 }
+
+/// Returns the GID of the specified group name
+fn get_group_id(group_name: &CStr) -> io::Result<u32> {
+    // SAFETY: group_name is a valid CString
+    let group = unsafe { libc::getgrnam(group_name.as_ptr() as *const _) };
+    if group.is_null() {
+        return Err(io::Error::from(io::ErrorKind::NotFound));
+    }
+    // SAFETY: group is not null
+    let gid = unsafe { (*group).gr_gid };
+    Ok(gid)
+}
+
+/// Sets group ID for the current process
+fn set_gid(gid: u32) -> io::Result<()> {
+    if unsafe { libc::setgid(gid) } == 0 {
+        Ok(())
+    } else {
+        Err(io::Error::last_os_error())
+    }
+}
+
+#[cfg(test)]
+#[test]
+fn test_unknown_group() {
+    let unknown_group = CStr::from_bytes_with_nul(b"asdunknown\0").unwrap();
+    let group_err = get_group_id(unknown_group).unwrap_err();
+    assert!(group_err.kind() == io::ErrorKind::NotFound)
+}
diff --git a/mullvad-daemon/src/lib.rs b/mullvad-daemon/src/lib.rs
index 5410ee569e..2e1d839da7 100644
--- a/mullvad-daemon/src/lib.rs
+++ b/mullvad-daemon/src/lib.rs
@@ -190,6 +190,10 @@ pub enum Error {
 
     #[error(display = "Failed to open cached target tunnel state")]
     OpenCachedTargetState(#[error(source)] io::Error),
+
+    #[cfg(target_os = "macos")]
+    #[error(display = "Failed to set exclusion group")]
+    GroupIdError(#[error(source)] io::Error),
 }
 
 /// Enum representing commands that can be sent to the daemon.
@@ -566,9 +570,9 @@ where
         #[cfg(target_os = "android")] android_context: AndroidContext,
     ) -> Result<Self, Error> {
         #[cfg(target_os = "macos")]
-        {
-            exclusion_gid::set_exclusion_gid();
-            talpid_core::macos::bump_filehandle_limit();
+        let exclusion_gid = {
+            bump_filehandle_limit();
+            exclusion_gid::set_exclusion_gid().map_err(Error::GroupIdError)?
         };
 
         let (tunnel_state_machine_shutdown_tx, tunnel_state_machine_shutdown_signal) =
@@ -680,7 +684,7 @@ where
             offline_state_tx,
             tunnel_state_machine_shutdown_tx,
             #[cfg(target_os = "macos")]
-            exclusion_gid::get_exclusion_gid(),
+            exclusion_gid,
             #[cfg(target_os = "macos")]
             settings.enable_custom_resolver,
             #[cfg(target_os = "android")]
@@ -2735,3 +2739,39 @@ impl TunnelParametersGenerator for MullvadTunnelParametersGenerator {
         }
     }
 }
+
+const INCREASED_FILEHANDLE_LIMIT: u64 = 1024;
+/// Bump filehandle limit
+pub fn bump_filehandle_limit() {
+    let mut limits = libc::rlimit {
+        rlim_cur: 0,
+        rlim_max: 0,
+    };
+    // SAFETY: `&mut limits` is a valid pointer parameter for the getrlimit syscall
+    let status = unsafe { libc::getrlimit(libc::RLIMIT_NOFILE, &mut limits) };
+    if status != 0 {
+        log::error!(
+            "Failed to get file handle limits: {}-{}",
+            io::Error::from_raw_os_error(status),
+            status
+        );
+        return;
+    }
+
+    // if file handle limit is already big enough, there's no reason to decrease it.
+    if limits.rlim_cur >= INCREASED_FILEHANDLE_LIMIT {
+        return;
+    }
+
+    limits.rlim_cur = INCREASED_FILEHANDLE_LIMIT;
+    // SAFETY: `&limits` is a valid pointer parameter for the getrlimit syscall
+    let status = unsafe { libc::setrlimit(libc::RLIMIT_NOFILE, &limits) };
+    if status != 0 {
+        log::error!(
+            "Failed to set file handle limit to {}: {}-{}",
+            INCREASED_FILEHANDLE_LIMIT,
+            io::Error::from_raw_os_error(status),
+            status
+        );
+    }
+}
diff --git a/mullvad-setup/src/main.rs b/mullvad-setup/src/main.rs
index 89c870bcfe..1e901edb71 100644
--- a/mullvad-setup/src/main.rs
+++ b/mullvad-setup/src/main.rs
@@ -161,7 +161,7 @@ async fn reset_firewall() -> Result<(), Error> {
         initial_state: InitialFirewallState::None,
         allow_lan: true,
         #[cfg(target_os = "macos")]
-        exclusion_gid: None,
+        exclusion_gid: 0,
     })
     .map_err(Error::FirewallError)?;
 
diff --git a/talpid-core/src/firewall/macos.rs b/talpid-core/src/firewall/macos.rs
index cc046bb821..0b18cc1a20 100644
--- a/talpid-core/src/firewall/macos.rs
+++ b/talpid-core/src/firewall/macos.rs
@@ -24,7 +24,7 @@ pub struct Firewall {
     pf: pfctl::PfCtl,
     pf_was_enabled: Option<bool>,
     rule_logging: RuleLogging,
-    exclusion_gid: Option<u32>,
+    exclusion_gid: u32,
 }
 
 impl FirewallT for Firewall {
@@ -176,57 +176,53 @@ impl Firewall {
         &self,
         servers: BTreeSet<IpAddr>,
     ) -> Result<Vec<pfctl::FilterRule>> {
-        if let Some(exclusion_gid) = self.exclusion_gid {
-            let allow_upstream_rules = servers
-                .iter()
-                .flat_map(|addr| {
-                    std::array::IntoIter::new([
-                        self.create_rule_builder(FilterRuleAction::Pass)
-                            .direction(pfctl::Direction::Out)
-                            .quick(true)
-                            .proto(pfctl::Proto::Tcp)
-                            .keep_state(pfctl::StatePolicy::Keep)
-                            .tcp_flags(Self::get_tcp_flags())
-                            .to(pfctl::Endpoint::new(*addr, 53))
-                            .group(exclusion_gid)
-                            .build(),
-                        self.create_rule_builder(FilterRuleAction::Pass)
-                            .direction(pfctl::Direction::Out)
-                            .quick(true)
-                            .proto(pfctl::Proto::Udp)
-                            .keep_state(pfctl::StatePolicy::Keep)
-                            .to(pfctl::Endpoint::new(*addr, 53))
-                            .group(exclusion_gid)
-                            .build(),
-                    ])
-                })
-                .collect::<Result<Vec<_>>>();
+        let allow_upstream_rules = servers
+            .iter()
+            .flat_map(|addr| {
+                std::array::IntoIter::new([
+                    self.create_rule_builder(FilterRuleAction::Pass)
+                        .direction(pfctl::Direction::Out)
+                        .quick(true)
+                        .proto(pfctl::Proto::Tcp)
+                        .keep_state(pfctl::StatePolicy::Keep)
+                        .tcp_flags(Self::get_tcp_flags())
+                        .to(pfctl::Endpoint::new(*addr, 53))
+                        .group(self.exclusion_gid)
+                        .build(),
+                    self.create_rule_builder(FilterRuleAction::Pass)
+                        .direction(pfctl::Direction::Out)
+                        .quick(true)
+                        .proto(pfctl::Proto::Udp)
+                        .keep_state(pfctl::StatePolicy::Keep)
+                        .to(pfctl::Endpoint::new(*addr, 53))
+                        .group(self.exclusion_gid)
+                        .build(),
+                ])
+            })
+            .collect::<Result<Vec<_>>>();
 
-            allow_upstream_rules.and_then(|mut rules| {
-                if !rules.is_empty() {
-                    rules.push(
-                        self.create_rule_builder(FilterRuleAction::Pass)
-                            .quick(true)
-                            .proto(pfctl::Proto::Udp)
-                            .to(pfctl::Endpoint::new(Ipv4Addr::LOCALHOST, 53))
-                            .build()?,
-                    );
-                    rules.push(
-                        self.create_rule_builder(FilterRuleAction::Pass)
-                            .quick(true)
-                            .proto(pfctl::Proto::Tcp)
-                            .keep_state(pfctl::StatePolicy::Keep)
-                            .tcp_flags(Self::get_tcp_flags())
-                            .to(pfctl::Endpoint::new(Ipv4Addr::LOCALHOST, 53))
-                            .build()?,
-                    );
-                }
+        allow_upstream_rules.and_then(|mut rules| {
+            if !rules.is_empty() {
+                rules.push(
+                    self.create_rule_builder(FilterRuleAction::Pass)
+                        .quick(true)
+                        .proto(pfctl::Proto::Udp)
+                        .to(pfctl::Endpoint::new(Ipv4Addr::LOCALHOST, 53))
+                        .build()?,
+                );
+                rules.push(
+                    self.create_rule_builder(FilterRuleAction::Pass)
+                        .quick(true)
+                        .proto(pfctl::Proto::Tcp)
+                        .keep_state(pfctl::StatePolicy::Keep)
+                        .tcp_flags(Self::get_tcp_flags())
+                        .to(pfctl::Endpoint::new(Ipv4Addr::LOCALHOST, 53))
+                        .build()?,
+                );
+            }
 
-                Ok(rules)
-            })
-        } else {
-            Ok(vec![])
-        }
+            Ok(rules)
+        })
     }
 
     fn get_allow_dns_rules_when_connected(
diff --git a/talpid-core/src/firewall/mod.rs b/talpid-core/src/firewall/mod.rs
index d981ff027a..814bd405c3 100644
--- a/talpid-core/src/firewall/mod.rs
+++ b/talpid-core/src/firewall/mod.rs
@@ -230,7 +230,7 @@ pub struct FirewallArguments {
     #[cfg(target_os = "macos")]
     /// This argument is required on macOS to know which group's traffic should be excluded, if at
     /// all.
-    pub exclusion_gid: Option<u32>,
+    pub exclusion_gid: u32,
 }
 
 /// State to enter during firewall init.
diff --git a/talpid-core/src/lib.rs b/talpid-core/src/lib.rs
index 38cffec488..73c3293fb4 100644
--- a/talpid-core/src/lib.rs
+++ b/talpid-core/src/lib.rs
@@ -63,10 +63,6 @@ mod mktemp;
 #[cfg(target_os = "linux")]
 mod linux;
 
-/// Misc utilities for the macOS platform.
-#[cfg(target_os = "macos")]
-pub mod macos;
-
 /// A pair of functions to monitor and establish connectivity with ICMP
 pub mod ping_monitor;
 
diff --git a/talpid-core/src/macos.rs b/talpid-core/src/macos.rs
deleted file mode 100644
index 49f9ee6b2f..0000000000
--- a/talpid-core/src/macos.rs
+++ /dev/null
@@ -1,67 +0,0 @@
-use std::{ffi::CStr, io};
-
-/// Returns the GID of the specified group name
-pub fn get_group_id(group_name: &CStr) -> io::Result<u32> {
-    // SAFETY: group_name is a valid CString
-    let group = unsafe { libc::getgrnam(group_name.as_ptr() as *const _) };
-    if group.is_null() {
-        return Err(io::Error::from(io::ErrorKind::NotFound));
-    }
-    // SAFETY: group is not null
-    let gid = unsafe { (*group).gr_gid };
-    Ok(gid)
-}
-
-/// Sets group ID for the current process
-pub fn set_gid(gid: u32) -> io::Result<()> {
-    if unsafe { libc::setgid(gid) } == 0 {
-        Ok(())
-    } else {
-        Err(io::Error::last_os_error())
-    }
-}
-
-const INCREASED_FILEHANDLE_LIMIT: u64 = 1024;
-/// Bump filehandle limit
-pub fn bump_filehandle_limit() {
-    let mut limits = libc::rlimit {
-        rlim_cur: 0,
-        rlim_max: 0,
-    };
-    // SAFETY: `&mut limits` is a valid pointer parameter for the getrlimit syscall
-    let status = unsafe { libc::getrlimit(libc::RLIMIT_NOFILE, &mut limits) };
-    if status != 0 {
-        log::error!(
-            "Failed to get file handle limits: {}-{}",
-            io::Error::from_raw_os_error(status),
-            status
-        );
-        return;
-    }
-
-    // if file handle limit is already big enough, there's no reason to decrease it.
-    if limits.rlim_cur >= INCREASED_FILEHANDLE_LIMIT {
-        return;
-    }
-
-    limits.rlim_cur = INCREASED_FILEHANDLE_LIMIT;
-    // SAFETY: `&limits` is a valid pointer parameter for the getrlimit syscall
-    let status = unsafe { libc::setrlimit(libc::RLIMIT_NOFILE, &limits as *const _) };
-    if status != 0 {
-        log::error!(
-            "Failed to set file handle limit to {}: {}-{}",
-            INCREASED_FILEHANDLE_LIMIT,
-            io::Error::from_raw_os_error(status),
-            status
-        );
-    }
-}
-
-#[cfg(test)]
-#[test]
-fn test_unknown_group() {
-    let unknown_group = CStr::from_bytes_with_nul(b"asdunknown\0").unwrap();
-    let group_err = get_group_id(unknown_group).unwrap_err();
-    assert!(group_err.kind() == io::ErrorKind::NotFound)
-
-}
diff --git a/talpid-core/src/resolver/mod.rs b/talpid-core/src/resolver/mod.rs
index fa026a48f3..6a52d6771f 100644
--- a/talpid-core/src/resolver/mod.rs
+++ b/talpid-core/src/resolver/mod.rs
@@ -61,35 +61,14 @@ pub(crate) async fn start_resolver(sender: TunnelCommandSender) -> Result<Resolv
 }
 
 async fn start_resolver_inner(
-    sender: TunnelCommandSender,
+    tunnel_tx: TunnelCommandSender,
     port: u16,
 ) -> Result<ResolverHandle, Error> {
-    let (tx, rx) = oneshot::channel();
-    std::thread::spawn(move || run_resolver(sender, tx, port));
-    rx.await.map_err(|_| Error::LauncherThreadPanic)?
+    let (resolver, resolver_handle) = FilteringResolver::new(tunnel_tx, port).await?;
+    tokio::spawn(resolver.run());
+    Ok(resolver_handle)
 }
 
-async fn run_resolver(
-    tunnel_tx: TunnelCommandSender,
-    done_tx: oneshot::Sender<Result<ResolverHandle, Error>>,
-    port: u16,
-) {
-    let mut builder = tokio::runtime::Builder::new_multi_thread();
-    builder.enable_all();
-    builder.worker_threads(2);
-    builder.max_blocking_threads(1);
-
-    let rt = builder.build().expect("failed to initialize tokio runtime");
-    match rt.block_on(FilteringResolver::new(tunnel_tx, port)) {
-        Ok((resolver, resolver_handle)) => {
-            let _ = done_tx.send(Ok(resolver_handle));
-            rt.block_on(resolver.run());
-        }
-        Err(err) => {
-            let _ = done_tx.send(Err(err));
-        }
-    }
-}
 /// Resolver errors
 #[derive(err_derive::Error, Debug)]
 #[error(no_from)]
@@ -244,6 +223,7 @@ impl FilteringResolver {
                     match resolver_state {
                         ResolverState::Shutdown => {
                             self.stop_server().await;
+                            self.resolver_state = ResolverState::Shutdown;
                         }
                         running_state => {
                             if self.dns_server.is_none() {
@@ -799,6 +779,8 @@ mod test {
         rt.block_on(async { handle.shutdown().await })
             .expect("failed to make resovler active");
 
+        // macOS takes it sweet time reaping the socket
+        std::thread::sleep(std::time::Duration::from_millis(300));
         UdpSocket::bind(server_sockaddr)
             .expect("Failed to bind to resolver socket addr when it should be unbound");
     }
diff --git a/talpid-core/src/tunnel_state_machine/mod.rs b/talpid-core/src/tunnel_state_machine/mod.rs
index 8b44f39dad..b222a5caa6 100644
--- a/talpid-core/src/tunnel_state_machine/mod.rs
+++ b/talpid-core/src/tunnel_state_machine/mod.rs
@@ -109,7 +109,7 @@ pub async fn spawn(
     state_change_listener: impl Sender<TunnelStateTransition> + Send + 'static,
     offline_state_listener: mpsc::UnboundedSender<bool>,
     shutdown_tx: oneshot::Sender<()>,
-    #[cfg(target_os = "macos")] exclusion_gid: Option<u32>,
+    #[cfg(target_os = "macos")] exclusion_gid: u32,
     #[cfg(target_os = "macos")] enable_resolver: bool,
     #[cfg(target_os = "android")] android_context: AndroidContext,
 ) -> Result<Arc<mpsc::UnboundedSender<TunnelCommand>>, Error> {
@@ -225,7 +225,7 @@ impl TunnelStateMachine {
         log_dir: Option<PathBuf>,
         resource_dir: PathBuf,
         commands_rx: mpsc::UnboundedReceiver<TunnelCommand>,
-        #[cfg(target_os = "macos")] exclusion_gid: Option<u32>,
+        #[cfg(target_os = "macos")] exclusion_gid: u32,
         #[cfg(target_os = "macos")] enable_resolver: bool,
         #[cfg(target_os = "android")] android_context: AndroidContext,
     ) -> Result<Self, Error> {