Merge remote-tracking branch 'origin/fix-st-resolvconf-dns'

3 files changed, 83 insertions, 103 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f30be7bfd7..e4c66689c9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -46,6 +46,9 @@ Line wrap the file at 100 chars.                                              Th
 - Move OpenVPN and WireGuard settings in the advanced settings view into separate settings views.
 - Return to main view in desktop app after being hidden/closed for two minutes.
 
+#### Linux
+- Always send DNS requests inside the tunnel for excluded processes when using public custom DNS.
+
 #### Windows
 - Upgrade Wintun from 0.10.4 to 0.13.
 
@@ -70,6 +73,7 @@ Line wrap the file at 100 chars.                                              Th
 - Make offline monitor aware of routing table changes.
 - Assign local DNS servers to more appropriate interfaces when using systemd-resolved.
 - Disable DNS over TLS for tunnel's DNS config when using systemd-resolved.
+- Fix DNS when combining a static resolv.conf with ad blocking DNS.
 
 #### Windows
 - Fix failure to restart the daemon when resuming from "fast startup" hibernation.
diff --git a/docs/split-tunneling.md b/docs/split-tunneling.md
index ff9838f81d..bda2ac5de6 100644
--- a/docs/split-tunneling.md
+++ b/docs/split-tunneling.md
@@ -28,7 +28,7 @@ excluded, DNS lookups **will fail** in the connecting, disconnecting, and error
 Some definitions of terms used later to describe behavior:
 
 * **In tunnel** - DNS requests are sent in the VPN tunnel. Firewall rules ensure they
-    are not allowed outside the tunnel*.
+    are not allowed outside the tunnel for non-excluded apps*.
 * **Outside tunnel** - DNS requests are sent outside the VPN tunnel. Firewall rules ensure
     they cannot go inside the tunnel*.
 * **LAN** - Same as **Outside tunnel** with the addition that the firewall rules ensure
@@ -43,7 +43,7 @@ Some definitions of terms used later to describe behavior:
 *: On platforms where we have custom firewall integration. This is currently on desktop operating
   systems, and not mobile.
 
-### Windows
+### Windows and Linux
 
 | In-app DNS setting | Normal & Excluded app |
 |-|-|
@@ -51,24 +51,13 @@ Some definitions of terms used later to describe behavior:
 | **Private custom DNS** (e.g. 10.0.1.1) | LAN (to 10.0.1.1) |
 | **Public custom DNS** (e.g. 8.8.8.8) | In tunnel (to 8.8.8.8) |
 
-In other words: Normal and excluded processes always behave the same. This is due to the
-Windows DNS cache service is the single origin for all DNS requests.
+In other words: Normal and excluded processes always behave the same. This is because DNS is
+typically handled by a service, e.g. DNS cache on Windows or systemd-resolved's resolver on Linux,
+which is not an excluded process.
 
-### Linux
-
-| In-app DNS setting | Normal app | Excluded app |
-|-|-|-|
-| **Default DNS** | In tunnel (to relay) | In tunnel (to relay) |
-| **Private custom DNS** (e.g. 10.0.1.1) | LAN (to 10.0.1.1) | LAN (to 10.0.1.1) |
-| **Public custom DNS** (e.g. 8.8.8.8) | In tunnel (to 8.8.8.8) | Outside tunnel* (to 8.8.8.8) |
-
-*: Only if a local DNS resolver, such as systemd-resolved is **not in use**. Because if a
-local DNS resolver is in use the requests will go there and that resolver in turn will then
-send requests in the tunnel.
-
-In other words: Normal and excluded processes behave the same in all cases except when Custom DNS
-is enabled, pointed to a publicly available IP and the system is not set up to use a localhost DNS
-resolver.
+For the sake of simplicity and consistency, requests to public custom DNS resolvers are also sent
+inside the tunnel when using a plain old static `resolv.conf`, even though it is technically
+possible to exclude public custom DNS in that case.
 
 ### Android
 
diff --git a/talpid-core/src/firewall/linux.rs b/talpid-core/src/firewall/linux.rs
index d9f6825431..674a95c9b6 100644
--- a/talpid-core/src/firewall/linux.rs
+++ b/talpid-core/src/firewall/linux.rs
@@ -325,6 +325,39 @@ impl<'a> PolicyBatch<'a> {
     }
 
     fn add_split_tunneling_rules(&mut self, policy: &FirewallPolicy) -> Result<()> {
+        // Send select DNS requests in the tunnel
+        if let FirewallPolicy::Connected {
+            tunnel,
+            dns_servers,
+            ..
+        } = policy
+        {
+            for server in dns_servers
+                .iter()
+                .filter(|server| !is_local_dns_address(&tunnel, server))
+            {
+                let chain = if server.is_ipv4() {
+                    &self.mangle_chain_v4
+                } else {
+                    &self.mangle_chain_v6
+                };
+                let allow_rule = allow_tunnel_dns_rule(
+                    chain,
+                    &tunnel.interface,
+                    TransportProtocol::Udp,
+                    *server,
+                )?;
+                self.batch.add(&allow_rule, nftnl::MsgType::Add);
+                let allow_rule = allow_tunnel_dns_rule(
+                    chain,
+                    &tunnel.interface,
+                    TransportProtocol::Tcp,
+                    *server,
+                )?;
+                self.batch.add(&allow_rule, nftnl::MsgType::Add);
+            }
+        }
+
         let mangle_chains = [&self.mangle_chain_v4, &self.mangle_chain_v6];
         for chain in &mangle_chains {
             let mut rule = Rule::new(chain);
@@ -345,36 +378,6 @@ impl<'a> PolicyBatch<'a> {
             self.batch.add(&rule, nftnl::MsgType::Add);
         }
 
-        // Allow some DNS requests to pass through the tunnel
-        if let FirewallPolicy::Connected {
-            tunnel,
-            dns_servers,
-            ..
-        } = policy
-        {
-            let gateway = IpAddr::V4(tunnel.ipv4_gateway);
-            if dns_servers.contains(&gateway) {
-                self.add_nat_tunnel_dns_rule(&tunnel.interface, TransportProtocol::Udp, gateway)?;
-                self.add_nat_tunnel_dns_rule(&tunnel.interface, TransportProtocol::Tcp, gateway)?;
-            }
-
-            if let Some(ref gateway) = tunnel.ipv6_gateway {
-                let gateway = IpAddr::V6(*gateway);
-                if dns_servers.contains(&gateway) {
-                    self.add_nat_tunnel_dns_rule(
-                        &tunnel.interface,
-                        TransportProtocol::Udp,
-                        gateway,
-                    )?;
-                    self.add_nat_tunnel_dns_rule(
-                        &tunnel.interface,
-                        TransportProtocol::Tcp,
-                        gateway,
-                    )?;
-                }
-            }
-        }
-
         let nat_chains = [&self.nat_chain_v4, &self.nat_chain_v6];
         for chain in &nat_chains {
             // Block remaining marked outgoing in-tunnel traffic
@@ -424,36 +427,6 @@ impl<'a> PolicyBatch<'a> {
         Ok(())
     }
 
-    fn add_nat_tunnel_dns_rule(
-        &mut self,
-        interface: &str,
-        protocol: TransportProtocol,
-        host: IpAddr,
-    ) -> Result<()> {
-        let mut allow_rule = match host {
-            IpAddr::V4(_) => Rule::new(&self.nat_chain_v4),
-            IpAddr::V6(_) => Rule::new(&self.nat_chain_v6),
-        };
-        let daddr = match host {
-            IpAddr::V4(_) => nft_expr!(payload ipv4 daddr),
-            IpAddr::V6(_) => nft_expr!(payload ipv6 daddr),
-        };
-
-        check_iface(&mut allow_rule, Direction::Out, interface)?;
-        check_port(&mut allow_rule, protocol, End::Dst, 53);
-
-        allow_rule.add_expr(&daddr);
-        allow_rule.add_expr(&nft_expr!(cmp == host));
-
-        allow_rule.add_expr(&nft_expr!(ct mark));
-        allow_rule.add_expr(&nft_expr!(cmp == split_tunnel::MARK));
-
-        add_verdict(&mut allow_rule, &Verdict::Accept);
-
-        self.batch.add(&allow_rule, nftnl::MsgType::Add);
-        Ok(())
-    }
-
     fn add_loopback_rules(&mut self) -> Result<()> {
         const LOOPBACK_IFACE_NAME: &str = "lo";
         self.batch.add(
@@ -590,6 +563,9 @@ impl<'a> PolicyBatch<'a> {
 
                 if let Some(tunnel) = tunnel {
                     self.add_allow_tunnel_rules(&tunnel.interface)?;
+                    if *allow_lan {
+                        self.add_block_cve_2019_14899(tunnel);
+                    }
                 }
                 *allow_lan
             }
@@ -692,15 +668,9 @@ impl<'a> PolicyBatch<'a> {
         dns_servers: &[IpAddr],
         protocol: TransportProtocol,
     ) -> Result<()> {
-        let (local_resolvers, remote_resolvers): (Vec<IpAddr>, Vec<IpAddr>) =
-            dns_servers.iter().partition(|server| {
-                super::is_local_address(server)
-                    && *server != &tunnel.ipv4_gateway
-                    && !tunnel
-                        .ipv6_gateway
-                        .map(|ref gateway| *server == gateway)
-                        .unwrap_or(false)
-            });
+        let (local_resolvers, remote_resolvers): (Vec<IpAddr>, Vec<IpAddr>) = dns_servers
+            .iter()
+            .partition(|server| is_local_dns_address(tunnel, server));
 
         for resolver in &local_resolvers {
             self.add_allow_local_dns_rule(&tunnel.interface, protocol, *resolver)?;
@@ -720,23 +690,9 @@ impl<'a> PolicyBatch<'a> {
         host: IpAddr,
     ) -> Result<()> {
         for chain in &[&self.out_chain, &self.forward_chain] {
-            let mut allow_rule = Rule::new(chain);
-            let daddr = match host {
-                IpAddr::V4(_) => nft_expr!(payload ipv4 daddr),
-                IpAddr::V6(_) => nft_expr!(payload ipv6 daddr),
-            };
-
-            check_iface(&mut allow_rule, Direction::Out, interface)?;
-            check_port(&mut allow_rule, protocol, End::Dst, 53);
-            check_l3proto(&mut allow_rule, host);
-
-            allow_rule.add_expr(&daddr);
-            allow_rule.add_expr(&nft_expr!(cmp == host));
-            add_verdict(&mut allow_rule, &Verdict::Accept);
-
+            let allow_rule = allow_tunnel_dns_rule(chain, interface, protocol, host)?;
             self.batch.add(&allow_rule, nftnl::MsgType::Add);
         }
-
         Ok(())
     }
 
@@ -895,6 +851,37 @@ impl<'a> PolicyBatch<'a> {
     }
 }
 
+fn is_local_dns_address(tunnel: &tunnel::TunnelMetadata, server: &IpAddr) -> bool {
+    super::is_local_address(server)
+        && server != &tunnel.ipv4_gateway
+        && Some(server) != tunnel.ipv6_gateway.map(IpAddr::from).as_ref()
+}
+
+fn allow_tunnel_dns_rule<'a>(
+    chain: &'a Chain<'_>,
+    iface: &str,
+    protocol: TransportProtocol,
+    host: IpAddr,
+) -> Result<Rule<'a>> {
+    let mut rule = Rule::new(chain);
+    check_iface(&mut rule, Direction::Out, iface)?;
+    check_port(&mut rule, protocol, End::Dst, 53);
+
+    let daddr = match host {
+        IpAddr::V4(_) => nft_expr!(payload ipv4 daddr),
+        IpAddr::V6(_) => nft_expr!(payload ipv6 daddr),
+    };
+    if chain.get_table().get_family() == ProtoFamily::Inet {
+        check_l3proto(&mut rule, host);
+    }
+
+    rule.add_expr(&daddr);
+    rule.add_expr(&nft_expr!(cmp == host));
+    add_verdict(&mut rule, &Verdict::Accept);
+
+    Ok(rule)
+}
+
 fn allow_interface_rule<'a>(
     chain: &'a Chain<'_>,
     direction: Direction,