3 files changed, 54 insertions, 5 deletions

diff --git a/src/android/net/ip/IpClient.java b/src/android/net/ip/IpClient.java
index 4860ff3..018d6ab 100644
--- a/src/android/net/ip/IpClient.java
+++ b/src/android/net/ip/IpClient.java
@@ -64,7 +64,6 @@ import android.text.TextUtils;
 import android.util.LocalLog;
 import android.util.Log;
 import android.util.Pair;
-import android.util.Patterns;
 import android.util.SparseArray;
 
 import androidx.annotation.NonNull;
@@ -86,6 +85,8 @@ import com.android.server.NetworkStackService.NetworkStackServiceManager;
 import java.io.FileDescriptor;
 import java.io.PrintWriter;
 import java.net.InetAddress;
+import java.net.MalformedURLException;
+import java.net.URL;
 import java.nio.BufferUnderflowException;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
@@ -1263,9 +1264,9 @@ public class IpClient extends StateMachine {
             }
 
             final String capportUrl = mDhcpResults.captivePortalApiUrl;
-            // Uri.parse does no syntax check; do a simple regex check to eliminate garbage.
+            // Uri.parse does no syntax check; do a simple check to eliminate garbage.
             // If the URL is still incorrect data fetching will fail later, which is fine.
-            if (capportUrl != null && Patterns.WEB_URL.matcher(capportUrl).matches()) {
+            if (isParseableUrl(capportUrl)) {
                 NetworkInformationShimImpl.newInstance()
                         .setCaptivePortalApiUrl(newLp, Uri.parse(capportUrl));
             }
@@ -1303,6 +1304,19 @@ public class IpClient extends StateMachine {
         return newLp;
     }
 
+    private static boolean isParseableUrl(String url) {
+        // Verify that a URL has a reasonable format that can be parsed as per the URL constructor.
+        // This does not use Patterns.WEB_URL as that pattern excludes URLs without TLDs, such as on
+        // localhost.
+        if (url == null) return false;
+        try {
+            new URL(url);
+            return true;
+        } catch (MalformedURLException e) {
+            return false;
+        }
+    }
+
     private static void addAllReachableDnsServers(
             LinkProperties lp, Iterable<InetAddress> dnses) {
         // TODO: Investigate deleting this reachability check.  We should be
diff --git a/src/com/android/server/connectivity/NetworkMonitor.java b/src/com/android/server/connectivity/NetworkMonitor.java
index 8a1b0c7..fc7f8c9 100755
--- a/src/com/android/server/connectivity/NetworkMonitor.java
+++ b/src/com/android/server/connectivity/NetworkMonitor.java
@@ -2548,7 +2548,12 @@ public class NetworkMonitor extends StateMachine {
             final String apiContent;
             try {
                 final URL url = new URL(mCaptivePortalApiUrl.toString());
-                if (!"https".equals(url.getProtocol())) {
+                // Protocol must be HTTPS
+                // (as per https://www.ietf.org/id/draft-ietf-capport-api-07.txt, #4).
+                // Only allow HTTP on localhost, for testing.
+                final boolean isLocalhostHttp =
+                        "localhost".equals(url.getHost()) && "http".equals(url.getProtocol());
+                if (!"https".equals(url.getProtocol()) && !isLocalhostHttp) {
                     validationLog("Invalid captive portal API protocol: " + url.getProtocol());
                     return null;
                 }
diff --git a/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java b/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
index f0a7bfb..4b249ce 100644
--- a/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
+++ b/tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java
@@ -1253,7 +1253,7 @@ public class NetworkMonitorTest {
     @Test
     public void testIsCaptivePortal_CapportApiInvalidContent() throws Exception {
         assumeTrue(CaptivePortalDataShimImpl.isSupported());
-        setStatus(mHttpsConnection, 204);
+        setSslException(mHttpsConnection);
         setPortal302(mHttpConnection);
         setApiContent(mCapportApiConnection, "{SomeInvalidText");
         runNetworkTest(makeCapportLPs(), CELL_METERED_CAPABILITIES,
@@ -1264,6 +1264,36 @@ public class NetworkMonitorTest {
         verify(mHttpConnection).getResponseCode();
     }
 
+    private void runCapportApiInvalidUrlTest(String url) throws Exception {
+        assumeTrue(CaptivePortalDataShimImpl.isSupported());
+        setSslException(mHttpsConnection);
+        setPortal302(mHttpConnection);
+        final LinkProperties lp = new LinkProperties(TEST_LINK_PROPERTIES);
+        lp.setCaptivePortalApiUrl(Uri.parse(url));
+        runNetworkTest(makeCapportLPs(), CELL_METERED_CAPABILITIES,
+                VALIDATION_RESULT_PORTAL, 0 /* probesSucceeded */,
+                TEST_LOGIN_URL);
+
+        verify(mCallbacks, never()).notifyCaptivePortalDataChanged(any());
+        verify(mCapportApiConnection, never()).getInputStream();
+        verify(mHttpConnection).getResponseCode();
+    }
+
+    @Test
+    public void testIsCaptivePortal_HttpIsInvalidCapportApiScheme() throws Exception {
+        runCapportApiInvalidUrlTest("http://capport.example.com");
+    }
+
+    @Test
+    public void testIsCaptivePortal_FileIsInvalidCapportApiScheme() throws Exception {
+        runCapportApiInvalidUrlTest("file://localhost/myfile");
+    }
+
+    @Test
+    public void testIsCaptivePortal_InvalidUrlFormat() throws Exception {
+        runCapportApiInvalidUrlTest("ThisIsNotAValidUrl");
+    }
+
     @Test @IgnoreUpTo(Build.VERSION_CODES.Q)
     public void testIsCaptivePortal_CapportApiNotSupported() throws Exception {
         // Test that on a R+ device, if NetworkStack was compiled without CaptivePortalData support