Merge branch 'ignore-private-ips'

2 files changed, 44 insertions, 7 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 429c270199..f7329f471d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -36,6 +36,7 @@ Line wrap the file at 100 chars.                                              Th
   version is supported.
 - Add `version` subcommand in the CLI to show information about current versions.
 - Add a flag to daemon to print log entries to standard output without timestamps.
+- Filter out and ignore DNS lookup results for api.mullvad.net that are bogus (private etc.)
 
 ### Changed
 - Change all occurrences of "MullvadVPN" into "Mullvad VPN", this affects
diff --git a/mullvad-rpc/src/cached_dns_resolver.rs b/mullvad-rpc/src/cached_dns_resolver.rs
index 141671a662..40efe14a81 100644
--- a/mullvad-rpc/src/cached_dns_resolver.rs
+++ b/mullvad-rpc/src/cached_dns_resolver.rs
@@ -71,6 +71,7 @@ impl SystemDnsResolver {
 
 impl DnsResolver for SystemDnsResolver {
     fn resolve(&mut self, host: &str) -> Result<IpAddr> {
+        debug!("Resolving IP for {}", host);
         Self::resolve_in_background_thread(host)
             .recv_timeout(DNS_TIMEOUT)
             .chain_err(|| ErrorKind::DnsTimeout(host.to_owned()))
@@ -167,6 +168,15 @@ impl<R: DnsResolver> CachedDnsResolver<R> {
 
     fn resolve_into_cache(&mut self) {
         if let Ok(address) = self.dns_resolver.resolve(&self.hostname) {
+            if Self::is_bogus_address(address) {
+                warn!(
+                    "DNS lookup for {} returned bogus address {}, ignoring",
+                    self.hostname, address
+                );
+                return;
+            }
+
+            debug!("Updating DNS cache for {} with {}", self.hostname, address);
             self.cached_address = address;
             self.last_updated = SystemTime::now();
 
@@ -176,6 +186,16 @@ impl<R: DnsResolver> CachedDnsResolver<R> {
         }
     }
 
+    /// Checks if an IP seems to be a reasonable and routable IP. Used to try to filter out and
+    /// ignore invalid IPs returned by poisoned DNS etc.
+    fn is_bogus_address(address: IpAddr) -> bool {
+        let is_private = match address {
+            IpAddr::V4(address) => address.is_private(),
+            _ => false,
+        };
+        address.is_unspecified() || address.is_loopback() || is_private
+    }
+
     fn update_cache_file(&mut self) -> io::Result<()> {
         let mut cache_file = File::create(&self.cache_file)?;
 
@@ -216,8 +236,8 @@ mod tests {
     #[test]
     fn old_cache_file_is_updated() {
         let (_temp_dir, cache_dir) = create_test_dirs();
-        let cached_address = "127.0.0.1".parse().unwrap();
-        let mock_address = "192.168.1.206".parse().unwrap();
+        let cached_address = "80.10.20.30".parse().unwrap();
+        let mock_address = "90.168.1.206".parse().unwrap();
         let mock_resolver = MockDnsResolver::with_address(mock_address);
 
         let cache_file_path = write_address(&cache_dir, cached_address);
@@ -250,7 +270,7 @@ mod tests {
     #[test]
     fn caches_resolved_ip() {
         let (_temp_dir, cache_dir) = create_test_dirs();
-        let mock_address = "192.168.1.206".parse().unwrap();
+        let mock_address = "80.10.1.206".parse().unwrap();
         let mock_resolver = MockDnsResolver::with_address(mock_address);
 
         let mut cache = create_cached_dns_resolver(mock_resolver, &cache_dir, None);
@@ -263,7 +283,7 @@ mod tests {
     #[test]
     fn resolves_even_if_impossible_to_store_in_cache() {
         let (temp_dir, cache_dir) = create_test_dirs();
-        let mock_address = "192.168.1.206".parse().unwrap();
+        let mock_address = "201.0.1.206".parse().unwrap();
         let mock_resolver = MockDnsResolver::with_address(mock_address);
 
         let mut cache = create_cached_dns_resolver(mock_resolver, &cache_dir, None);
@@ -291,8 +311,8 @@ mod tests {
     #[test]
     fn ignores_fallback_address_if_resolution_succeeds() {
         let (_temp_dir, cache_dir) = create_test_dirs();
-        let fallback_address = "192.168.1.31".parse().unwrap();
-        let mock_address = "192.168.1.206".parse().unwrap();
+        let fallback_address = "200.10.1.31".parse().unwrap();
+        let mock_address = "150.10.1.206".parse().unwrap();
         let mock_resolver = MockDnsResolver::with_address(mock_address);
 
         let mut cache =
@@ -305,7 +325,7 @@ mod tests {
     #[test]
     fn invalid_cache_file_leads_to_fallback_address_usage() {
         let (_temp_dir, cache_dir) = create_test_dirs();
-        let fallback_address = "192.168.1.31".parse().unwrap();
+        let fallback_address = "160.20.1.31".parse().unwrap();
         let mock_resolver = MockDnsResolver::that_fails();
         let mock_resolver_was_called = mock_resolver.was_called_handle();
 
@@ -319,6 +339,22 @@ mod tests {
         assert_eq!(address, fallback_address);
     }
 
+    #[test]
+    fn ignores_private_ip() {
+        let (_temp_dir, cache_dir) = create_test_dirs();
+        let fallback_address = "160.20.1.31".parse().unwrap();
+        let mock_address = "10.100.200.1".parse().unwrap();
+        let mock_resolver = MockDnsResolver::with_address(mock_address);
+
+        let mut cache =
+            create_cached_dns_resolver(mock_resolver, &cache_dir, Some(fallback_address));
+        let address = cache.resolve();
+
+        assert_eq!(address, fallback_address);
+        let cache_file_path = cache_dir.join("api_ip_address.txt");
+        assert!(!cache_file_path.exists());
+    }
+
     fn create_test_dirs() -> (TempDir, PathBuf) {
         let temp_dir = TempDir::new("ip-cache-test").unwrap();
         let cache_dir = temp_dir.path().join("cache");