From d8f9ccd2147aba4f434858a1ae3e3bd5f5e9c845 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Tue, 24 Oct 2023 15:36:39 +0200 Subject: [PATCH] Configure firewall rules to allow proxy clients The default setting will (always) be to only allow processes with root-privilege to send/receive traffic from an allowed endpoint. This change is only supposed to be used with the local SOCKS5 api access method. --- mullvad-api/src/lib.rs | 6 +- mullvad-api/src/proxy.rs | 33 ++++++++- mullvad-api/src/rest.rs | 10 ++- mullvad-daemon/src/api.rs | 15 ++-- talpid-core/src/firewall/linux.rs | 24 +++--- talpid-core/src/firewall/macos.rs | 30 ++++---- talpid-types/src/net/mod.rs | 119 ++++++++++++++++++++++++++---- 7 files changed, 187 insertions(+), 50 deletions(-) diff --git a/mullvad-api/src/lib.rs b/mullvad-api/src/lib.rs index 267fdfd8114e..5a436fe1bf7f 100644 --- a/mullvad-api/src/lib.rs +++ b/mullvad-api/src/lib.rs @@ -21,7 +21,7 @@ use std::{ ops::Deref, path::Path, }; -use talpid_types::{net::Endpoint, ErrorExt}; +use talpid_types::{net::AllowedEndpoint, ErrorExt}; pub mod availability; use availability::{ApiAvailability, ApiAvailabilityHandle}; @@ -221,13 +221,13 @@ pub enum Error { /// Closure that receives the next API (real or proxy) endpoint to use for `api.mullvad.net`. /// It should return a future that determines whether to reject the new endpoint or not. -pub trait ApiEndpointUpdateCallback: Fn(Endpoint) -> Self::AcceptedNewEndpoint { +pub trait ApiEndpointUpdateCallback: Fn(AllowedEndpoint) -> Self::AcceptedNewEndpoint { type AcceptedNewEndpoint: Future + Send; } impl + Send> ApiEndpointUpdateCallback for U where - U: Fn(Endpoint) -> T, + U: Fn(AllowedEndpoint) -> T, { type AcceptedNewEndpoint = T; } diff --git a/mullvad-api/src/proxy.rs b/mullvad-api/src/proxy.rs index 07276285969e..aa3f088e6f50 100644 --- a/mullvad-api/src/proxy.rs +++ b/mullvad-api/src/proxy.rs @@ -9,7 +9,7 @@ use std::{ task::{self, Poll}, }; use talpid_types::{ - net::{Endpoint, TransportProtocol}, + net::{AllowedClients, Endpoint, TransportProtocol}, ErrorExt, }; use tokio::{ @@ -143,6 +143,37 @@ impl ApiConnectionMode { } } + /// TODO(markus): + /// 1. Implement seperately for windows + #[cfg(unix)] + pub fn allowed_clients(&self) -> AllowedClients { + use access_method::Socks5; + match self { + ApiConnectionMode::Proxied(ProxyConfig::Socks(Socks5::Local(_))) => AllowedClients::All, + ApiConnectionMode::Direct | ApiConnectionMode::Proxied(_) => AllowedClients::Root, + } + } + + #[cfg(windows)] + pub fn allowed_clients(&self) -> AllowedClients { + use access_method::Socks5; + let clients = match self { + // Specifying no clients means that *all* clients are allowed. Probably should be abstracted. + ApiConnectionMode::Proxied(ProxyConfig::Socks(Socks5::Local(_))) => vec![], + ApiConnectionMode::Direct | ApiConnectionMode::Proxied(_) => { + let daemon_exe = std::env::current_exe().expect("failed to obtain executable path"); + vec![ + daemon_exe + .parent() + .expect("missing executable parent directory") + .join("mullvad-problem-report.exe"), + daemon_exe, + ] + } + }; + clients.into() + } + pub fn is_proxy(&self) -> bool { *self != ApiConnectionMode::Direct } diff --git a/mullvad-api/src/rest.rs b/mullvad-api/src/rest.rs index a973c32f2a75..9ee479d1d3d0 100644 --- a/mullvad-api/src/rest.rs +++ b/mullvad-api/src/rest.rs @@ -25,7 +25,7 @@ use std::{ time::Duration, }; use talpid_types::{ - net::{Endpoint, TransportProtocol}, + net::{AllowedEndpoint, Endpoint, TransportProtocol}, ErrorExt, }; @@ -217,8 +217,14 @@ impl< TransportProtocol::Tcp, ), }; + let allowed_clients = new_config.allowed_clients(); + // TODO(markus): Move this to !one! function in `ApiConnectionMode` + let allowed_endpoint = AllowedEndpoint { + endpoint, + clients: allowed_clients, + }; // Switch to new connection mode unless rejected by address change callback - if (self.new_address_callback)(endpoint).await { + if (self.new_address_callback)(allowed_endpoint).await { self.connector_handle.set_connection_mode(new_config); } } diff --git a/mullvad-daemon/src/api.rs b/mullvad-daemon/src/api.rs index a6dd0dead66b..dd6e33fc5d34 100644 --- a/mullvad-daemon/src/api.rs +++ b/mullvad-daemon/src/api.rs @@ -239,7 +239,7 @@ impl ApiEndpointUpdaterHandle { pub fn callback(&self) -> impl ApiEndpointUpdateCallback { let tunnel_tx = self.tunnel_cmd_tx.clone(); - move |endpoint: Endpoint| { + move |allowed_endpoint: AllowedEndpoint| { let inner_tx = tunnel_tx.clone(); async move { let tunnel_tx = if let Some(tunnel_tx) = { inner_tx.lock().unwrap().as_ref() } @@ -252,12 +252,15 @@ impl ApiEndpointUpdaterHandle { }; let (result_tx, result_rx) = oneshot::channel(); let _ = tunnel_tx.unbounded_send(TunnelCommand::AllowEndpoint( - get_allowed_endpoint(endpoint), + allowed_endpoint.clone(), result_tx, )); // Wait for the firewall policy to be updated. let _ = result_rx.await; - log::debug!("API endpoint: {endpoint}"); + log::debug!( + "API endpoint: {endpoint}", + endpoint = allowed_endpoint.endpoint + ); true } } @@ -276,11 +279,11 @@ pub(super) fn get_allowed_endpoint(endpoint: Endpoint) -> AllowedEndpoint { daemon_exe, ]; - AllowedEndpoint { + AllowedEndpoint::new( + endpoint, #[cfg(windows)] clients, - endpoint, - } + ) } pub(crate) fn forward_offline_state( diff --git a/talpid-core/src/firewall/linux.rs b/talpid-core/src/firewall/linux.rs index 1b74fb30bd80..a74aa1ce4a9c 100644 --- a/talpid-core/src/firewall/linux.rs +++ b/talpid-core/src/firewall/linux.rs @@ -14,7 +14,7 @@ use std::{ fs, io, net::{IpAddr, Ipv4Addr}, }; -use talpid_types::net::{AllowedTunnelTraffic, Endpoint, TransportProtocol}; +use talpid_types::net::{AllowedEndpoint, AllowedTunnelTraffic, Endpoint, TransportProtocol}; /// Priority for rules that tag split tunneling packets. Equals NF_IP_PRI_MANGLE. const MANGLE_CHAIN_PRIORITY: i32 = libc::NF_IP_PRI_MANGLE; @@ -519,7 +519,7 @@ impl<'a> PolicyBatch<'a> { allowed_tunnel_traffic, } => { self.add_allow_tunnel_endpoint_rules(peer_endpoint, fwmark); - self.add_allow_endpoint_rules(&allowed_endpoint.endpoint); + self.add_allow_endpoint_rules(allowed_endpoint); // Important to block DNS after allow relay rule (so the relay can operate // over port 53) but before allow LAN (so DNS does not leak to the LAN) @@ -568,7 +568,7 @@ impl<'a> PolicyBatch<'a> { allowed_endpoint, } => { if let Some(endpoint) = allowed_endpoint { - self.add_allow_endpoint_rules(&endpoint.endpoint); + self.add_allow_endpoint_rules(endpoint); } // Important to drop DNS before allowing LAN (to stop DNS leaking to the LAN) @@ -628,24 +628,28 @@ impl<'a> PolicyBatch<'a> { /// Adds firewall rules allow traffic to flow to the API. Allows the app to reach the API in /// blocked states. - fn add_allow_endpoint_rules(&mut self, endpoint: &Endpoint) { + fn add_allow_endpoint_rules(&mut self, endpoint: &AllowedEndpoint) { let mut in_rule = Rule::new(&self.in_chain); - check_endpoint(&mut in_rule, End::Src, endpoint); + check_endpoint(&mut in_rule, End::Src, &endpoint.endpoint); let allowed_states = nftnl::expr::ct::States::ESTABLISHED.bits(); in_rule.add_expr(&nft_expr!(ct state)); in_rule.add_expr(&nft_expr!(bitwise mask allowed_states, xor 0u32)); in_rule.add_expr(&nft_expr!(cmp != 0u32)); - in_rule.add_expr(&nft_expr!(meta skuid)); - in_rule.add_expr(&nft_expr!(cmp == super::ROOT_UID)); + if !endpoint.clients.allow_all() { + in_rule.add_expr(&nft_expr!(meta skuid)); + in_rule.add_expr(&nft_expr!(cmp == super::ROOT_UID)); + } add_verdict(&mut in_rule, &Verdict::Accept); self.batch.add(&in_rule, nftnl::MsgType::Add); let mut out_rule = Rule::new(&self.out_chain); - check_endpoint(&mut out_rule, End::Dst, endpoint); - out_rule.add_expr(&nft_expr!(meta skuid)); - out_rule.add_expr(&nft_expr!(cmp == super::ROOT_UID)); + check_endpoint(&mut out_rule, End::Dst, &endpoint.endpoint); + if !endpoint.clients.allow_all() { + out_rule.add_expr(&nft_expr!(meta skuid)); + out_rule.add_expr(&nft_expr!(cmp == super::ROOT_UID)); + } add_verdict(&mut out_rule, &Verdict::Accept); self.batch.add(&out_rule, nftnl::MsgType::Add); diff --git a/talpid-core/src/firewall/macos.rs b/talpid-core/src/firewall/macos.rs index 4f95309890ca..54f72a79c26a 100644 --- a/talpid-core/src/firewall/macos.rs +++ b/talpid-core/src/firewall/macos.rs @@ -6,7 +6,7 @@ use std::{ net::{IpAddr, Ipv4Addr}, }; use subslice::SubsliceExt; -use talpid_types::net::{self, AllowedTunnelTraffic}; +use talpid_types::net::{self, AllowedEndpoint, AllowedTunnelTraffic}; pub use pfctl::Error; @@ -122,7 +122,7 @@ impl Firewall { allowed_tunnel_traffic, } => { let mut rules = vec![self.get_allow_relay_rule(*peer_endpoint)?]; - rules.push(self.get_allowed_endpoint_rule(allowed_endpoint.endpoint)?); + rules.push(self.get_allowed_endpoint_rule(allowed_endpoint)?); // Important to block DNS after allow relay rule (so the relay can operate // over port 53) but before allow LAN (so DNS does not leak to the LAN) @@ -175,7 +175,7 @@ impl Firewall { } => { let mut rules = Vec::new(); if let Some(allowed_endpoint) = allowed_endpoint { - rules.push(self.get_allowed_endpoint_rule(allowed_endpoint.endpoint)?); + rules.push(self.get_allowed_endpoint_rule(allowed_endpoint)?); } if *allow_lan { @@ -287,22 +287,26 @@ impl Firewall { .build() } - /// Produces a rule that allows traffic to flow to the API. Allows the app to reach the API in - /// blocked states. + /// Produces a rule that allows traffic to flow to the API. Allows the app (or other apps if configured) + /// to reach the API in blocked states. fn get_allowed_endpoint_rule( &self, - allowed_endpoint: net::Endpoint, + allowed_endpoint: &AllowedEndpoint, ) -> Result { - let pfctl_proto = as_pfctl_proto(allowed_endpoint.protocol); + let pfctl_proto = as_pfctl_proto(allowed_endpoint.endpoint.protocol); - self.create_rule_builder(FilterRuleAction::Pass) - .direction(pfctl::Direction::Out) - .to(allowed_endpoint.address) + let mut rule = self.create_rule_builder(FilterRuleAction::Pass); + rule.direction(pfctl::Direction::Out) + .to(allowed_endpoint.endpoint.address) .proto(pfctl_proto) .keep_state(pfctl::StatePolicy::Keep) - .user(Uid::from(super::ROOT_UID)) - .quick(true) - .build() + .quick(true); + + if !allowed_endpoint.clients.allow_all() { + rule.user(Uid::from(super::ROOT_UID)).build()?; + } + + rule.build() } fn get_block_dns_rules(&self) -> Result> { diff --git a/talpid-types/src/net/mod.rs b/talpid-types/src/net/mod.rs index 6ff56175cc65..a99d861a3658 100644 --- a/talpid-types/src/net/mod.rs +++ b/talpid-types/src/net/mod.rs @@ -276,10 +276,32 @@ impl fmt::Display for Endpoint { /// Host that should be reachable in any tunnel state. #[derive(Debug, Clone, Eq, PartialEq)] pub struct AllowedEndpoint { - /// Paths that should be allowed to communicate with `endpoint`. - #[cfg(windows)] - pub clients: Vec, + /// How to connect to a certain `endpoint`. pub endpoint: Endpoint, + /// Clients that should be allowed to communicate with `endpoint`. + pub clients: AllowedClients, +} + +#[cfg(windows)] +impl AllowedEndpoint { + /// Create a new [`AllowedEndpoint`]. + pub fn new>(endpoint: Endpoint, clients: Clients) -> Self { + Self { + endpoint, + clients: clients.into(), + } + } +} + +#[cfg(unix)] +impl AllowedEndpoint { + /// Create a new [`AllowedEndpoint`]. + pub fn new(endpoint: Endpoint) -> Self { + Self { + endpoint, + clients: AllowedClients::default(), + } + } } impl fmt::Display for AllowedEndpoint { @@ -288,23 +310,90 @@ impl fmt::Display for AllowedEndpoint { write!(f, "{}", self.endpoint)?; #[cfg(windows)] { - write!(f, "{} for", self.endpoint)?; - #[cfg(windows)] - for client in &self.clients { - write!( - f, - " {}", - client - .file_name() - .map(|s| s.to_string_lossy()) - .unwrap_or(std::borrow::Cow::Borrowed("")) - )?; - } + let clients = if self.clients.allow_all() { + "any executable".to_string() + } else { + self.clients + .iter() + .map(|client| { + client + .file_name() + .map(|s| s.to_string_lossy()) + .unwrap_or(std::borrow::Cow::Borrowed("")) + }) + .collect::>() + .join(" ".into()) + }; + write!( + f, + "{endpoint} for {clients}", + endpoint = self.endpoint, + clients = clients + )?; } Ok(()) } } +/// Clients which should be able to reach an allowed host in any tunnel state. +/// +/// # Note +/// On Windows, there is no predetermined binary which should be allowed to leak +/// traffic outside of the tunnel. Thus, [`std::default::Default`] is not +/// implemented for [`AllowedClients`]. +#[cfg(windows)] +#[derive(Debug, Clone, Eq, PartialEq)] +pub struct AllowedClients(std::sync::Arc<[PathBuf]>); + +#[cfg(windows)] +impl std::ops::Deref for AllowedClients { + type Target = [PathBuf]; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +#[cfg(windows)] +impl From> for AllowedClients { + fn from(value: Vec) -> Self { + Self(value.into()) + } +} + +#[cfg(windows)] +impl AllowedClients { + pub fn allow_all(&self) -> bool { + self.is_empty() + } +} + +/// Clients which should be able to reach an allowed host in any tunnel state. +#[cfg(unix)] +#[derive(Debug, Default, Clone, Eq, PartialEq)] +pub enum AllowedClients { + /// Allow only clients running as `root` to leak traffic to an allowed [`Endpoint`]. + /// + /// # Note + /// The most secure client(s) is our own, which runs as root. Therefore, + /// [`AllowedClients::Root`] is allowed to be the default value. + #[default] + Root, + /// Allow *all* clients to leak traffic to an allowed [`Endpoint`]. + /// + /// This is necessary on platforms which does not have proper support for + /// split-tunneling, but which wants to support running local SOCKS5-proxies. + /// See TODO(REFER TO LOCAL SOCKS5 PROXY SUPPORT DOCS) for further information. + All, +} + +#[cfg(unix)] +impl AllowedClients { + pub fn allow_all(&self) -> bool { + matches!(self, AllowedClients::All) + } +} + #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub enum AllowedTunnelTraffic { None,