Merge branch 'handle-invalid-dns-server-addresses'

9 files changed, 162 insertions, 45 deletions

diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/notification/TunnelStateNotification.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/notification/TunnelStateNotification.kt
index d352b3fc2a..80ae9da5e4 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/notification/TunnelStateNotification.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/notification/TunnelStateNotification.kt
@@ -8,6 +8,7 @@ import net.mullvad.talpid.tunnel.ActionAfterDisconnect
 import net.mullvad.talpid.tunnel.ErrorState
 import net.mullvad.talpid.tunnel.ErrorStateCause
 import net.mullvad.talpid.tunnel.ParameterGenerationError
+import net.mullvad.talpid.util.addressString
 
 class TunnelStateNotification(
     private val context: Context,
@@ -53,10 +54,27 @@ class TunnelStateNotification(
     }
 
     private fun show(error: ErrorState?) {
-        val cause = error?.cause
+        // if the error state is null, we can assume that we are secure
+        if (error?.isBlocking ?: true) {
+            title = blockingTitle
+            message = error?.cause?.let { cause -> blockingErrorMessage(cause) }
+        } else {
+            title = notBlockingTitle
+            message = notBlockingErrorMessage(error?.cause)
+        }
+
+        shouldShow = true
+    }
+
+    private fun blockingErrorMessage(cause: ErrorStateCause): String {
+        val messageId = when (cause) {
+            is ErrorStateCause.InvalidDnsServers -> {
+                val addresses = cause.addresses
+                    .map { address -> address.addressString() }
+                    .joinToString()
 
-        val messageText = when (cause) {
-            null -> null
+                return context.getString(R.string.invalid_dns_servers, addresses)
+            }
             is ErrorStateCause.AuthFailed -> R.string.auth_failed
             is ErrorStateCause.Ipv6Unavailable -> R.string.ipv6_unavailable
             is ErrorStateCause.SetFirewallPolicyError -> R.string.set_firewall_policy_error
@@ -78,21 +96,16 @@ class TunnelStateNotification(
             is ErrorStateCause.VpnPermissionDenied -> R.string.vpn_permission_denied_error
         }
 
-        // if the error state is null, we can assume that we are secure
-        if (error?.isBlocking ?: true) {
-            title = blockingTitle
-            message = messageText?.let { id -> context.getString(id) }
-        } else {
-            val updatedMessageText = when (cause) {
-                is ErrorStateCause.VpnPermissionDenied -> messageText
-                else -> R.string.failed_to_block_internet
-            }
+        return context.getString(messageId)
+    }
 
-            title = notBlockingTitle
-            message = updatedMessageText?.let { id -> context.getString(id) }
+    private fun notBlockingErrorMessage(cause: ErrorStateCause?): String {
+        val messageId = when (cause) {
+            is ErrorStateCause.VpnPermissionDenied -> R.string.vpn_permission_denied_error
+            else -> R.string.failed_to_block_internet
         }
 
-        shouldShow = true
+        return context.getString(messageId)
     }
 
     private fun hide() {
diff --git a/android/src/main/kotlin/net/mullvad/talpid/CreateTunResult.kt b/android/src/main/kotlin/net/mullvad/talpid/CreateTunResult.kt
new file mode 100644
index 0000000000..dfa82852f1
--- /dev/null
+++ b/android/src/main/kotlin/net/mullvad/talpid/CreateTunResult.kt
@@ -0,0 +1,24 @@
+package net.mullvad.talpid
+
+import java.net.InetAddress
+
+sealed class CreateTunResult {
+    open val isOpen
+        get() = false
+
+    class Success(val tunFd: Int) : CreateTunResult() {
+        override val isOpen
+            get() = true
+    }
+
+    class InvalidDnsServers(
+        val addresses: ArrayList<InetAddress>,
+        val tunFd: Int
+    ) : CreateTunResult() {
+        override val isOpen
+            get() = true
+    }
+
+    class PermissionDenied : CreateTunResult()
+    class TunnelDeviceError : CreateTunResult()
+}
diff --git a/android/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt b/android/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt
index a0dfd029f2..7da32ad71b 100644
--- a/android/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt
+++ b/android/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt
@@ -10,12 +10,21 @@ import kotlin.properties.Delegates.observable
 import net.mullvad.talpid.tun_provider.TunConfig
 
 open class TalpidVpnService : VpnService() {
-    private var activeTunDevice by observable<Int?>(null) { _, oldTunDevice, _ ->
-        oldTunDevice?.let { oldTunFd ->
+    private var activeTunStatus by observable<CreateTunResult?>(null) { _, oldTunStatus, _ ->
+        val oldTunFd = when (oldTunStatus) {
+            is CreateTunResult.Success -> oldTunStatus.tunFd
+            is CreateTunResult.InvalidDnsServers -> oldTunStatus.tunFd
+            else -> null
+        }
+
+        if (oldTunFd != null) {
             ParcelFileDescriptor.adoptFd(oldTunFd).close()
         }
     }
 
+    private val tunIsOpen
+        get() = activeTunStatus?.isOpen ?: false
+
     private var currentTunConfig = defaultTunConfig()
     private var tunIsStale = false
 
@@ -31,52 +40,52 @@ open class TalpidVpnService : VpnService() {
         connectivityListener.unregister()
     }
 
-    fun getTun(config: TunConfig): Int {
+    fun getTun(config: TunConfig): CreateTunResult {
         synchronized(this) {
-            val tunDevice = activeTunDevice
+            val tunStatus = activeTunStatus
 
-            if (config == currentTunConfig && tunDevice != null && !tunIsStale) {
-                return tunDevice
+            if (config == currentTunConfig && tunIsOpen && !tunIsStale) {
+                return tunStatus!!
             } else {
-                val newTunDevice = createTun(config)
+                val newTunStatus = createTun(config)
 
                 currentTunConfig = config
-                activeTunDevice = newTunDevice
+                activeTunStatus = newTunStatus
                 tunIsStale = false
 
-                return newTunDevice
+                return newTunStatus
             }
         }
     }
 
     fun createTun() {
         synchronized(this) {
-            activeTunDevice = createTun(currentTunConfig)
+            activeTunStatus = createTun(currentTunConfig)
         }
     }
 
     fun createTunIfClosed(): Boolean {
         synchronized(this) {
-            if (activeTunDevice == null) {
-                activeTunDevice = createTun(currentTunConfig)
+            if (!tunIsOpen) {
+                activeTunStatus = createTun(currentTunConfig)
             }
 
-            return activeTunDevice?.let { tunFd -> tunFd > 0 } ?: false
+            return tunIsOpen
         }
     }
 
     fun recreateTunIfOpen(config: TunConfig) {
         synchronized(this) {
-            if (activeTunDevice != null) {
+            if (tunIsOpen) {
                 currentTunConfig = config
-                activeTunDevice = createTun(config)
+                activeTunStatus = createTun(config)
             }
         }
     }
 
     fun closeTun() {
         synchronized(this) {
-            activeTunDevice = null
+            activeTunStatus = null
         }
     }
 
@@ -86,19 +95,25 @@ open class TalpidVpnService : VpnService() {
         }
     }
 
-    private fun createTun(config: TunConfig): Int {
+    private fun createTun(config: TunConfig): CreateTunResult {
         if (VpnService.prepare(this) != null) {
             // VPN permission wasn't granted
-            return -1
+            return CreateTunResult.PermissionDenied()
         }
 
+        var invalidDnsServerAddresses = ArrayList<InetAddress>()
+
         val builder = Builder().apply {
             for (address in config.addresses) {
                 addAddress(address, prefixForAddress(address))
             }
 
             for (dnsServer in config.dnsServers) {
-                addDnsServer(dnsServer)
+                try {
+                    addDnsServer(dnsServer)
+                } catch (exception: IllegalArgumentException) {
+                    invalidDnsServerAddresses.add(dnsServer)
+                }
             }
 
             for (route in config.routes) {
@@ -122,12 +137,17 @@ open class TalpidVpnService : VpnService() {
         val vpnInterface = builder.establish()
         val tunFd = vpnInterface?.detachFd()
 
-        if (tunFd != null) {
-            waitForTunnelUp(tunFd, config.routes.any { route -> route.isIpv6 })
-            return tunFd
-        } else {
-            return 0
+        if (tunFd == null) {
+            return CreateTunResult.TunnelDeviceError()
         }
+
+        waitForTunnelUp(tunFd, config.routes.any { route -> route.isIpv6 })
+
+        if (!invalidDnsServerAddresses.isEmpty()) {
+            return CreateTunResult.InvalidDnsServers(invalidDnsServerAddresses, tunFd)
+        }
+
+        return CreateTunResult.Success(tunFd)
     }
 
     fun bypass(socket: Int): Boolean {
diff --git a/android/src/main/kotlin/net/mullvad/talpid/tunnel/ErrorStateCause.kt b/android/src/main/kotlin/net/mullvad/talpid/tunnel/ErrorStateCause.kt
index e65f70be63..22962630b6 100644
--- a/android/src/main/kotlin/net/mullvad/talpid/tunnel/ErrorStateCause.kt
+++ b/android/src/main/kotlin/net/mullvad/talpid/tunnel/ErrorStateCause.kt
@@ -1,10 +1,13 @@
 package net.mullvad.talpid.tunnel
 
+import java.net.InetAddress
+
 sealed class ErrorStateCause {
     class AuthFailed(val reason: String?) : ErrorStateCause()
     class Ipv6Unavailable : ErrorStateCause()
     class SetFirewallPolicyError : ErrorStateCause()
     class SetDnsError : ErrorStateCause()
+    class InvalidDnsServers(val addresses: ArrayList<InetAddress>) : ErrorStateCause()
     class StartTunnelError : ErrorStateCause()
     class TunnelParameterError(val error: ParameterGenerationError) : ErrorStateCause()
     class IsOffline : ErrorStateCause()
diff --git a/android/src/main/res/values/strings.xml b/android/src/main/res/values/strings.xml
index fbb8182bf4..720a406465 100644
--- a/android/src/main/res/values/strings.xml
+++ b/android/src/main/res/values/strings.xml
@@ -118,6 +118,7 @@
     <string name="set_firewall_policy_error">Failed to apply firewall rules. The device might
     currently be unsecured</string>
     <string name="set_dns_error">Failed to set system DNS server</string>
+    <string name="invalid_dns_servers">Custom DNS server addresses %1$s are invalid</string>
     <string name="start_tunnel_error">Failed to start tunnel connection</string>
     <string name="vpn_permission_denied_error">VPN permission was denied when creating the tunnel.
     Please try connecting again.</string>
diff --git a/mullvad-jni/src/classes.rs b/mullvad-jni/src/classes.rs
index d402d1a8a3..eaa5d215ae 100644
--- a/mullvad-jni/src/classes.rs
+++ b/mullvad-jni/src/classes.rs
@@ -56,8 +56,13 @@ pub const CLASSES: &[&str] = &[
     "net/mullvad/talpid/tunnel/ErrorStateCause$StartTunnelError",
     "net/mullvad/talpid/tunnel/ErrorStateCause$TunnelParameterError",
     "net/mullvad/talpid/tunnel/ErrorStateCause$IsOffline",
+    "net/mullvad/talpid/tunnel/ErrorStateCause$InvalidDnsServers",
     "net/mullvad/talpid/tunnel/ErrorStateCause$VpnPermissionDenied",
     "net/mullvad/talpid/tunnel/ParameterGenerationError",
     "net/mullvad/talpid/ConnectivityListener",
+    "net/mullvad/talpid/CreateTunResult$Success",
+    "net/mullvad/talpid/CreateTunResult$InvalidDnsServers",
+    "net/mullvad/talpid/CreateTunResult$PermissionDenied",
+    "net/mullvad/talpid/CreateTunResult$TunnelDeviceError",
     "net/mullvad/talpid/TalpidVpnService",
 ];
diff --git a/talpid-core/src/tunnel/tun_provider/android/mod.rs b/talpid-core/src/tunnel/tun_provider/android/mod.rs
index 4c136ef189..b9385f13a7 100644
--- a/talpid-core/src/tunnel/tun_provider/android/mod.rs
+++ b/talpid-core/src/tunnel/tun_provider/android/mod.rs
@@ -10,7 +10,7 @@ use jnix::{
         sys::JNI_FALSE,
         JavaVM,
     },
-    IntoJava, JnixEnv,
+    FromJava, IntoJava, JnixEnv,
 };
 use std::{
     net::{IpAddr, Ipv4Addr, Ipv6Addr},
@@ -40,6 +40,12 @@ pub enum Error {
     FindMethod(&'static str, #[error(source)] jnix::jni::errors::Error),
 
     #[error(
+        display = "Attempt to configure the tunnel with an invalid DNS server address(es): {:?}",
+        _0
+    )]
+    InvalidDnsServers(Vec<IpAddr>),
+
+    #[error(
         display = "Received an invalid result from TalpidVpnService.{}: {}",
         _0,
         _1
@@ -193,15 +199,13 @@ impl AndroidTunProvider {
 
         let result = self.call_method(
             "getTun",
-            "(Lnet/mullvad/talpid/tun_provider/TunConfig;)I",
-            JavaType::Primitive(Primitive::Int),
+            "(Lnet/mullvad/talpid/tun_provider/TunConfig;)Lnet/mullvad/talpid/CreateTunResult;",
+            JavaType::Object("net/mullvad/talpid/CreateTunResult".to_owned()),
             &[JValue::Object(java_config.as_obj())],
         )?;
 
         match result {
-            JValue::Int(0) => Err(Error::TunnelDeviceError),
-            JValue::Int(-1) => Err(Error::PermissionDenied),
-            JValue::Int(fd) => Ok(fd),
+            JValue::Object(result) => CreateTunResult::from_java(&env, result).into(),
             value => Err(Error::InvalidMethodResult("getTun", format!("{:?}", value))),
         }
     }
@@ -376,3 +380,25 @@ impl Default for TunConfig {
         }
     }
 }
+
+#[derive(FromJava)]
+#[jnix(package = "net.mullvad.talpid")]
+enum CreateTunResult {
+    Success { tun_fd: i32 },
+    InvalidDnsServers { addresses: Vec<IpAddr> },
+    PermissionDenied,
+    TunnelDeviceError,
+}
+
+impl From<CreateTunResult> for Result<RawFd, Error> {
+    fn from(result: CreateTunResult) -> Self {
+        match result {
+            CreateTunResult::Success { tun_fd } => Ok(tun_fd),
+            CreateTunResult::InvalidDnsServers { addresses } => {
+                Err(Error::InvalidDnsServers(addresses))
+            }
+            CreateTunResult::PermissionDenied => Err(Error::PermissionDenied),
+            CreateTunResult::TunnelDeviceError => Err(Error::TunnelDeviceError),
+        }
+    }
+}
diff --git a/talpid-core/src/tunnel_state_machine/connecting_state.rs b/talpid-core/src/tunnel_state_machine/connecting_state.rs
index 6e561acc6b..44dcd9f153 100644
--- a/talpid-core/src/tunnel_state_machine/connecting_state.rs
+++ b/talpid-core/src/tunnel_state_machine/connecting_state.rs
@@ -422,6 +422,14 @@ impl TunnelState for ConnectingState {
                                             ),
                                         ),
                                     ) => ErrorStateCause::VpnPermissionDenied,
+                                    #[cfg(target_os = "android")]
+                                    tunnel::Error::WireguardTunnelMonitoringError(
+                                        tunnel::wireguard::Error::TunnelError(
+                                            tunnel::wireguard::TunnelError::SetupTunnelDeviceError(
+                                                tun_provider::Error::InvalidDnsServers(addresses),
+                                            ),
+                                        ),
+                                    ) => ErrorStateCause::InvalidDnsServers(addresses),
                                     _ => ErrorStateCause::StartTunnelError,
                                 };
                                 ErrorState::enter(shared_values, block_reason)
diff --git a/talpid-types/src/tunnel.rs b/talpid-types/src/tunnel.rs
index 5a0dfd0b3c..b3a75999b4 100644
--- a/talpid-types/src/tunnel.rs
+++ b/talpid-types/src/tunnel.rs
@@ -3,6 +3,8 @@ use crate::net::TunnelEndpoint;
 use jnix::IntoJava;
 use serde::{Deserialize, Serialize};
 use std::fmt;
+#[cfg(target_os = "android")]
+use std::net::IpAddr;
 
 /// Event emitted from the states in `talpid_core::tunnel_state_machine` when the tunnel state
 /// machine enters a new state.
@@ -90,6 +92,9 @@ pub enum ErrorStateCause {
     SetFirewallPolicyError(FirewallPolicyError),
     /// Failed to set system DNS server.
     SetDnsError,
+    /// Android has rejected one or more DNS server addresses.
+    #[cfg(target_os = "android")]
+    InvalidDnsServers(Vec<IpAddr>),
     /// Failed to start connection to remote server.
     StartTunnelError,
     /// Tunnel parameter generation failure
@@ -170,6 +175,18 @@ impl fmt::Display for ErrorStateCause {
                 };
             }
             SetDnsError => "Failed to set system DNS server",
+            #[cfg(target_os = "android")]
+            InvalidDnsServers(ref addresses) => {
+                return write!(
+                    f,
+                    "Invalid DNS server addresses used in tunnel configuration: {}",
+                    addresses
+                        .iter()
+                        .map(IpAddr::to_string)
+                        .collect::<Vec<_>>()
+                        .join(", ")
+                );
+            }
             StartTunnelError => "Failed to start connection to remote server",
             TunnelParameterError(ref err) => {
                 return write!(f, "Failure to generate tunnel parameters: {}", err);