Merge "Do not detect portals when DNS returns private IPs" into rvc-dev

6 files changed, 131 insertions, 6 deletions

diff --git a/common/captiveportal/src/android/net/captiveportal/CaptivePortalProbeResult.java b/common/captiveportal/src/android/net/captiveportal/CaptivePortalProbeResult.java
index 48cd48b..deee91a 100644..100755
--- a/common/captiveportal/src/android/net/captiveportal/CaptivePortalProbeResult.java
+++ b/common/captiveportal/src/android/net/captiveportal/CaptivePortalProbeResult.java
@@ -36,6 +36,12 @@ public final class CaptivePortalProbeResult {
      */
     public static final int PARTIAL_CODE = -1;
 
+    // DNS response with private IP on the probe URL means the network, especially Wi-Fi network is
+    // not connected to the Internet. This code represents the result of a single probe, for correct
+    // logging of the probe results. The result of the whole evaluation would typically be FAILED if
+    // one of the probes returns this status.
+    public static final int DNS_PRIVATE_IP_RESPONSE_CODE = -2;
+
     @NonNull
     public static final CaptivePortalProbeResult FAILED = new CaptivePortalProbeResult(FAILED_CODE);
     @NonNull
@@ -43,6 +49,8 @@ public final class CaptivePortalProbeResult {
             new CaptivePortalProbeResult(SUCCESS_CODE);
     public static final CaptivePortalProbeResult PARTIAL =
             new CaptivePortalProbeResult(PARTIAL_CODE);
+    public static final CaptivePortalProbeResult PRIVATE_IP =
+            new CaptivePortalProbeResult(DNS_PRIVATE_IP_RESPONSE_CODE);
 
     private final int mHttpResponseCode;  // HTTP response code returned from Internet probe.
     @Nullable
@@ -85,4 +93,8 @@ public final class CaptivePortalProbeResult {
     public boolean isPartialConnectivity() {
         return mHttpResponseCode == PARTIAL_CODE;
     }
+
+    public boolean isDnsPrivateIpResponse() {
+        return mHttpResponseCode == DNS_PRIVATE_IP_RESPONSE_CODE;
+    }
 }
diff --git a/res/values/config.xml b/res/values/config.xml
index a2f3959..8bd8eff 100644
--- a/res/values/config.xml
+++ b/res/values/config.xml
@@ -59,4 +59,12 @@
     <integer name="config_nud_steadystate_solicit_interval">750</integer>
     <integer name="config_nud_postroaming_solicit_num">5</integer>
     <integer name="config_nud_postroaming_solicit_interval">750</integer>
+
+    <!-- Whether to force considering DNS probes returning private IP addresses as failed
+         when attempting to detect captive portals.
+         The impact of this feature will be evaluated separately through experiments.
+         Force-enabling the feature regardless of the experiment results is not advised, as it
+         could result in valid captive portals being incorrectly classified as having no
+         connectivity.-->
+    <bool name="config_force_dns_probe_private_ip_no_internet">false</bool>
 </resources>
diff --git a/res/values/overlayable.xml b/res/values/overlayable.xml
index 052266d..ed86814 100644
--- a/res/values/overlayable.xml
+++ b/res/values/overlayable.xml
@@ -44,6 +44,14 @@
             <item type="integer" name="config_nud_steadystate_solicit_interval"/>
             <item type="integer" name="config_nud_postroaming_solicit_num"/>
             <item type="integer" name="config_nud_postroaming_solicit_interval"/>
+
+            <!-- Whether to force considering DNS probes returning private IP addresses as failed
+            when attempting to detect captive portals.
+            The impact of this feature will be evaluated separately through experiments.
+            Force-enabling the feature regardless of the experiment results is not advised, as it
+            could result in valid captive portals being incorrectly classified as having no
+            connectivity.-->
+            <item type="bool" name="config_force_dns_probe_private_ip_no_internet"/>
         </policy>
     </overlayable>
 </resources>
diff --git a/src/android/net/util/NetworkStackUtils.java b/src/android/net/util/NetworkStackUtils.java
index bc41549..2de18de 100644..100755
--- a/src/android/net/util/NetworkStackUtils.java
+++ b/src/android/net/util/NetworkStackUtils.java
@@ -172,6 +172,15 @@ public class NetworkStackUtils {
     public static final String DISMISS_PORTAL_IN_VALIDATED_NETWORK =
             "dismiss_portal_in_validated_network";
 
+    /**
+     * Experiment flag to enable considering DNS probes returning private IP addresses as failed
+     * when attempting to detect captive portals.
+     *
+     * This flag is enabled if !=0 and less than the module APK version.
+     */
+    public static final String DNS_PROBE_PRIVATE_IP_NO_INTERNET_VERSION =
+            "dns_probe_private_ip_no_internet";
+
     static {
         System.loadLibrary("networkstackutilsjni");
     }
diff --git a/src/com/android/server/connectivity/NetworkMonitor.java b/src/com/android/server/connectivity/NetworkMonitor.java
index 7ea61fa..0540258 100644..100755
--- a/src/com/android/server/connectivity/NetworkMonitor.java
+++ b/src/com/android/server/connectivity/NetworkMonitor.java
@@ -70,6 +70,7 @@ import static android.net.util.NetworkStackUtils.CAPTIVE_PORTAL_USE_HTTPS;
 import static android.net.util.NetworkStackUtils.DEFAULT_CAPTIVE_PORTAL_HTTPS_URLS;
 import static android.net.util.NetworkStackUtils.DEFAULT_CAPTIVE_PORTAL_HTTP_URLS;
 import static android.net.util.NetworkStackUtils.DISMISS_PORTAL_IN_VALIDATED_NETWORK;
+import static android.net.util.NetworkStackUtils.DNS_PROBE_PRIVATE_IP_NO_INTERNET_VERSION;
 import static android.net.util.NetworkStackUtils.isEmpty;
 import static android.provider.DeviceConfig.NAMESPACE_CONNECTIVITY;
 
@@ -423,6 +424,8 @@ public class NetworkMonitor extends StateMachine {
     private boolean mAcceptPartialConnectivity = false;
     private final EvaluationState mEvaluationState = new EvaluationState();
 
+    private final boolean mPrivateIpNotPortalEnabled;
+
     private int getCallbackVersion(INetworkMonitorCallbacks cb) {
         int version;
         try {
@@ -484,6 +487,7 @@ public class NetworkMonitor extends StateMachine {
         // CHECKSTYLE:ON IndentationCheck
 
         mIsCaptivePortalCheckEnabled = getIsCaptivePortalCheckEnabled();
+        mPrivateIpNotPortalEnabled = getIsPrivateIpNotPortalEnabled();
         mUseHttps = getUseHttpsValidation();
         mCaptivePortalUserAgent = getCaptivePortalUserAgent();
         mCaptivePortalHttpsUrls = makeCaptivePortalHttpsUrls();
@@ -1434,6 +1438,12 @@ public class NetworkMonitor extends StateMachine {
         return mode != CAPTIVE_PORTAL_MODE_IGNORE;
     }
 
+    private boolean getIsPrivateIpNotPortalEnabled() {
+        return mDependencies.isFeatureEnabled(mContext, DNS_PROBE_PRIVATE_IP_NO_INTERNET_VERSION)
+                || mContext.getResources().getBoolean(
+                        R.bool.config_force_dns_probe_private_ip_no_internet);
+    }
+
     private boolean getUseHttpsValidation() {
         return mDependencies.getDeviceConfigPropertyInt(NAMESPACE_CONNECTIVITY,
                 CAPTIVE_PORTAL_USE_HTTPS, 1) == 1;
@@ -1877,7 +1887,13 @@ public class NetworkMonitor extends StateMachine {
         // state machine thread. Reporting results here would cause races and potentially send
         // information to callers that does not make sense because the state machine has already
         // changed state.
-        sendDnsProbe(host);
+        final InetAddress[] resolvedAddr = sendDnsProbe(host);
+        // The private IP logic only applies to the HTTP probe, not the HTTPS probe (which would
+        // fail anyway) or the PAC probe.
+        if (mPrivateIpNotPortalEnabled && probeType == ValidationProbeEvent.PROBE_HTTP
+                && (proxy == null) && hasPrivateIpAddress(resolvedAddr)) {
+            return CaptivePortalProbeResult.PRIVATE_IP;
+        }
         return sendHttpProbe(url, probeType, null);
     }
 
@@ -1891,23 +1907,41 @@ public class NetworkMonitor extends StateMachine {
     }
 
     /** Do a DNS resolution of the given server. */
-    private void sendDnsProbe(String host) {
+    private InetAddress[] sendDnsProbe(String host) {
         if (TextUtils.isEmpty(host)) {
-            return;
+            return null;
         }
 
-        final String name = ValidationProbeEvent.getProbeName(ValidationProbeEvent.PROBE_DNS);
         final Stopwatch watch = new Stopwatch().start();
         int result;
-        String connectInfo;
+        InetAddress[] addresses;
         try {
-            InetAddress[] addresses = sendDnsProbeWithTimeout(host, getDnsProbeTimeout());
+            addresses = sendDnsProbeWithTimeout(host, getDnsProbeTimeout());
             result = ValidationProbeEvent.DNS_SUCCESS;
         } catch (UnknownHostException e) {
+            addresses = null;
             result = ValidationProbeEvent.DNS_FAILURE;
         }
         final long latency = watch.stop();
         logValidationProbe(latency, ValidationProbeEvent.PROBE_DNS, result);
+        return addresses;
+    }
+
+    /**
+     * Check if any of the provided IP addresses include a private IP, as defined by
+     * {@link com.android.server.util.NetworkStackConstants#PRIVATE_IPV4_RANGES}.
+     * @return true if an IP address is private.
+     */
+    private static boolean hasPrivateIpAddress(@Nullable InetAddress[] addresses) {
+        if (addresses == null) {
+            return false;
+        }
+        for (InetAddress address : addresses) {
+            if (address.isLinkLocalAddress() || address.isSiteLocalAddress()) {
+                return true;
+            }
+        }
+        return false;
     }
 
     /**
@@ -2230,6 +2264,15 @@ public class NetworkMonitor extends StateMachine {
             reportHttpProbeResult(NETWORK_VALIDATION_PROBE_HTTPS, httpsResult);
             return httpsResult;
         }
+        // Consider a DNS response with a private IP address on the HTTP probe as an indication that
+        // the network is not connected to the Internet, and have the whole evaluation fail in that
+        // case.
+        // This only applies if the DNS probe completed within PROBE_TIMEOUT_MS, as the fallback
+        // probe should not be delayed by this check.
+        if (mPrivateIpNotPortalEnabled && (httpResult.isDnsPrivateIpResponse())) {
+            validationLog("DNS response to the URL is private IP");
+            return CaptivePortalProbeResult.FAILED;
+        }
         // If a fallback method exists, use it to retry portal detection.
         // If we have new-style probe specs, use those. Otherwise, use the fallback URLs.
         final CaptivePortalProbeSpec probeSpec = nextFallbackSpec();
@@ -2449,6 +2492,16 @@ public class NetworkMonitor extends StateMachine {
         }
 
         /**
+         * Check whether or not one experimental feature in the connectivity namespace is
+         * enabled.
+         * @param name Flag name of the experiment in the connectivity namespace.
+         * @see NetworkStackUtils#isFeatureEnabled(Context, String, String)
+         */
+        public boolean isFeatureEnabled(@NonNull Context context, @NonNull String name) {
+            return NetworkStackUtils.isFeatureEnabled(context, NAMESPACE_CONNECTIVITY, name);
+        }
+
+        /**
          * Send a broadcast indicating network conditions.
          */
         public void sendNetworkConditionsBroadcast(@NonNull Context context,
diff --git a/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java b/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
index ab1ebd6..b0efa33 100644
--- a/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
+++ b/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
@@ -38,6 +38,7 @@ import static android.net.util.NetworkStackUtils.CAPTIVE_PORTAL_FALLBACK_PROBE_S
 import static android.net.util.NetworkStackUtils.CAPTIVE_PORTAL_OTHER_FALLBACK_URLS;
 import static android.net.util.NetworkStackUtils.CAPTIVE_PORTAL_USE_HTTPS;
 import static android.net.util.NetworkStackUtils.DISMISS_PORTAL_IN_VALIDATED_NETWORK;
+import static android.net.util.NetworkStackUtils.DNS_PROBE_PRIVATE_IP_NO_INTERNET_VERSION;
 
 import static com.android.networkstack.apishim.ConstantsShim.DETECTION_METHOD_DNS_EVENTS;
 import static com.android.networkstack.apishim.ConstantsShim.DETECTION_METHOD_TCP_METRICS;
@@ -93,6 +94,7 @@ import android.net.ConnectivityManager;
 import android.net.DnsResolver;
 import android.net.INetd;
 import android.net.INetworkMonitorCallbacks;
+import android.net.InetAddresses;
 import android.net.LinkProperties;
 import android.net.Network;
 import android.net.NetworkCapabilities;
@@ -152,6 +154,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.lang.reflect.Constructor;
 import java.net.HttpURLConnection;
+import java.net.Inet6Address;
 import java.net.InetAddress;
 import java.net.URL;
 import java.net.UnknownHostException;
@@ -747,6 +750,38 @@ public class NetworkMonitorTest {
         runPortalNetworkTest(VALIDATION_RESULT_PORTAL);
     }
 
+    private void setupPrivateIpResponse(String privateAddr) throws Exception {
+        setSslException(mHttpsConnection);
+        setPortal302(mHttpConnection);
+        final String httpHost = new URL(TEST_HTTP_URL).getHost();
+        mFakeDns.setAnswer(httpHost, new String[] { "2001:db8::123" }, TYPE_AAAA);
+        final InetAddress parsedPrivateAddr = InetAddresses.parseNumericAddress(privateAddr);
+        mFakeDns.setAnswer(httpHost, new String[] { privateAddr },
+                (parsedPrivateAddr instanceof Inet6Address) ? TYPE_AAAA : TYPE_A);
+    }
+
+    @Test
+    public void testIsCaptivePortal_PrivateIpNotPortal_Enabled_IPv4() throws Exception {
+        when(mDependencies.isFeatureEnabled(any(), eq(DNS_PROBE_PRIVATE_IP_NO_INTERNET_VERSION)))
+                .thenReturn(true);
+        setupPrivateIpResponse("192.168.1.1");
+        runFailedNetworkTest();
+    }
+
+    @Test
+    public void testIsCaptivePortal_PrivateIpNotPortal_Enabled_IPv6() throws Exception {
+        when(mDependencies.isFeatureEnabled(any(), eq(DNS_PROBE_PRIVATE_IP_NO_INTERNET_VERSION)))
+                .thenReturn(true);
+        setupPrivateIpResponse("fec0:1234::1");
+        runFailedNetworkTest();
+    }
+
+    @Test
+    public void testIsCaptivePortal_PrivateIpNotPortal_Disabled() throws Exception {
+        setupPrivateIpResponse("192.168.1.1");
+        runPortalNetworkTest(VALIDATION_RESULT_PORTAL);
+    }
+
     @Test
     public void testIsCaptivePortal_HttpsProbeIsNotPortal() throws IOException {
         setStatus(mHttpsConnection, 204);