From 2652461f08351139db160a0fbcd0fa5bc470615f Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Fri, 15 Nov 2024 07:37:47 +0100 Subject: [PATCH] TODO(drop): Revert "Merge branch 'stop-arping-leaking-in-tunnel-ip-on-linux-des-1434'" This reverts commit 3de3c630c6f6884fea454b54f3cc81457d142db4, reversing changes made to dfb7d99822969b44e6dcad0e896971cb3ec58231. --- CHANGELOG.md | 4 --- README.md | 14 +++------- talpid-core/src/firewall/linux.rs | 44 +++---------------------------- 3 files changed, 7 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cc00fb3560af..32ea3e8b5b88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,10 +58,6 @@ Line wrap the file at 100 chars. Th leak (as easily) when the VPN tunnel is up. Previously, WSL would leak while in the blocked or connecting state, or while lockdown mode was active. -#### Linux -- Prevent attackers able to send ARP requests to the device running Mullvad from figuring out - the in-tunnel IP. Fixes 2024 audit issue `MLLVD-CR-24-03`. - ## [2024.7] - 2024-10-30 This release is identical to 2024.7-beta1. diff --git a/README.md b/README.md index 9676940fc3f7..ddfddfe3b0a5 100644 --- a/README.md +++ b/README.md @@ -136,20 +136,12 @@ See [this](Release.md) for instructions on how to make a new release. * Set to `"pass"` to add logging to rules allowing packets. * Set to `"drop"` to add logging to rules blocking packets. -* `TALPID_FIREWALL_DONT_SET_SRC_VALID_MARK` - Set this variable to `1` to stop the daemon from - setting the `net.ipv4.conf.all.src_valid_mark` kernel parameter to `1` on Linux when a tunnel - is established. - The kernel config parameter is set by default, because otherwise strict reverse path filtering - may prevent relay traffic from reaching the daemon. If `rp_filter` is set to `1` on the interface +* `TALPID_FIREWALL_DONT_SET_SRC_VALID_MARK` - Forces the daemon to not set `src_valid_mark` config + on Linux. The kernel config option is set because otherwise strict reverse path filtering may + prevent relay traffic from reaching the daemon. If `rp_filter` is set to `1` on the interface that will be receiving relay traffic, and `src_valid_mark` is not set to `1`, the daemon will not be able to receive relay traffic. -* `TALPID_FIREWALL_DONT_SET_ARP_IGNORE` - Set this variable to `1` to stop the daemon from - setting the `net.ipv4.conf.all.arp_ignore` kernel parameter to `2` on Linux when a tunnel - is established. - The kernel config parameter is set by default, because otherwise an attacker who can send ARP - requests to the device running Mullvad can figure out the in-tunnel IP. - * `TALPID_DNS_MODULE` - Allows changing the method that will be used for DNS configuration. By default this is automatically detected, but you can set it to one of the options below to choose a specific method. diff --git a/talpid-core/src/firewall/linux.rs b/talpid-core/src/firewall/linux.rs index 26d3a5cb5a68..71a13ad91429 100644 --- a/talpid-core/src/firewall/linux.rs +++ b/talpid-core/src/firewall/linux.rs @@ -21,7 +21,6 @@ use talpid_types::net::{ const MANGLE_CHAIN_PRIORITY: i32 = libc::NF_IP_PRI_MANGLE; const PREROUTING_CHAIN_PRIORITY: i32 = libc::NF_IP_PRI_CONNTRACK + 1; const PROC_SYS_NET_IPV4_CONF_SRC_VALID_MARK: &str = "/proc/sys/net/ipv4/conf/all/src_valid_mark"; -const PROC_SYS_NET_IPV4_CONF_ARP_IGNORE: &str = "/proc/sys/net/ipv4/conf/all/arp_ignore"; pub type Result = std::result::Result; @@ -77,11 +76,6 @@ static DONT_SET_SRC_VALID_MARK: LazyLock = LazyLock::new(|| { .map(|v| v != "0") .unwrap_or(false) }); -static DONT_SET_ARP_IGNORE: LazyLock = LazyLock::new(|| { - env::var("TALPID_FIREWALL_DONT_SET_ARP_IGNORE") - .map(|v| v != "0") - .unwrap_or(false) -}); #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] enum Direction { @@ -140,27 +134,12 @@ impl Firewall { fn apply_kernel_config(policy: &FirewallPolicy) { if *DONT_SET_SRC_VALID_MARK { log::debug!("Not setting src_valid_mark"); - } else if let FirewallPolicy::Connecting { .. } = policy { - if let Err(err) = set_src_valid_mark_sysctl() { - log::error!("Failed to apply src_valid_mark: {}", err); - } + return; } - // When we have a tunnel with an IP configured, we configure the system - // to not reply to arp requests for this tunnel IP *on other interfaces*. - // By default, Linux responds to incoming arp requests for any IP configured on any - // interface on the system. This makes it possible to via ARP-pinging figure out the - // VPN in-tunnel IP by spamming ARP requests to any physical interface on the device. - // - // We never store the initial value and restore it. We deem the default value - // to be too relaxed and don't see any reason why anyone would want a more relaxed - // setting than the one we are setting here. - if *DONT_SET_ARP_IGNORE { - log::debug!("Not setting arp_ignore"); - } else if let FirewallPolicy::Connecting { .. } | FirewallPolicy::Connected { .. } = policy - { - if let Err(err) = lock_down_arp_ignore_sysctl() { - log::error!("Failed to apply arp_ignore: {}", err); + if let FirewallPolicy::Connecting { .. } = policy { + if let Err(err) = set_src_valid_mark_sysctl() { + log::error!("Failed to apply src_valid_mark: {}", err); } } } @@ -1075,21 +1054,6 @@ fn set_src_valid_mark_sysctl() -> io::Result<()> { fs::write(PROC_SYS_NET_IPV4_CONF_SRC_VALID_MARK, b"1") } -/// If the `net.ipv4.conf.all.arp_ignore` setting is below 2, sets it to 2. -/// -/// 2 means: reply only if the target IP address is local address configured on the incoming -/// interface and both with the sender's IP address are part from same subnet on this interface. -fn lock_down_arp_ignore_sysctl() -> io::Result<()> { - // Should be safe to treat the content as a string, since it should always be a number. - let current_arp_ignore = fs::read_to_string(PROC_SYS_NET_IPV4_CONF_ARP_IGNORE)?; - match current_arp_ignore.trim() { - "0" | "1" => fs::write(PROC_SYS_NET_IPV4_CONF_ARP_IGNORE, b"2")?, - "2" => (), - _ => log::trace!("Not locking down arp_ignore since it is set to {current_arp_ignore}"), - } - Ok(()) -} - /// Tables that are no longer used but need to be deleted due to upgrades. /// This can be removed when upgrades from 2023.3 are no longer supported. fn batch_deprecated_tables(batch: &mut Batch) {