Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 2f187305 authored by Remi NGUYEN VAN's avatar Remi NGUYEN VAN
Browse files

Only allow HTTP capport URLs on test networks 

HTTP URLs on localhost for the capport API should only be accepted on
networks with TRANSPORT_TEST.

This change also introduces the first changes to fix thread safety
issues in NetworkMonitor, where LinkProperties or NetworkCapabilities
are read from the evaluation thread, even though they are updated from
the StateMachine thread. The EvaluationThreadDeps class should be
augmented in later changes to hold thread-safe copies of what the
evaluation thread needs.

Bug: 156062304
Bug: 155455470
Test: atest NetworkMonitorTest

Change-Id: I65bb54c581965159b99d7ac8596304ceb6b5f2cb
parent 48bd1067

apishim/30/com/android/networkstack/apishim/api30/ConstantsShim.java

+5 −0
Original line number Original line Diff line number Diff line
@@ -37,4 +37,9 @@ public class ConstantsShim extends com.android.networkstack.apishim.api29.Consta 
            DataStallReport.DETECTION_METHOD_DNS_EVENTS;

            DataStallReport.DETECTION_METHOD_DNS_EVENTS;

    public static final int DETECTION_METHOD_TCP_METRICS =

    public static final int DETECTION_METHOD_TCP_METRICS =

            DataStallReport.DETECTION_METHOD_TCP_METRICS;

            DataStallReport.DETECTION_METHOD_TCP_METRICS;



    /**

     * @see android.net.NetworkCapabilities

     */

    public static final int TRANSPORT_TEST = 7;

}
 
}

apishim/31/com/android/networkstack/apishim/ConstantsShim.java

+3 −0
Original line number Original line Diff line number Diff line
@@ -30,4 +30,7 @@ public class ConstantsShim extends com.android.networkstack.apishim.api30.Consta 
     */

     */

    @VisibleForTesting

    @VisibleForTesting

    public static final int VERSION = 31;

    public static final int VERSION = 31;



    // TODO: add TRANSPORT_TEST to system API in API 31 (it is only a test API as of R)

    public static final int TRANSPORT_TEST = 7;

}
 
}

src/com/android/server/connectivity/NetworkMonitor.java

+51 −27
Original line number Original line Diff line number Diff line
@@ -83,6 +83,7 @@ import static android.provider.DeviceConfig.NAMESPACE_CONNECTIVITY; 




import static com.android.networkstack.apishim.ConstantsShim.DETECTION_METHOD_DNS_EVENTS;

import static com.android.networkstack.apishim.ConstantsShim.DETECTION_METHOD_DNS_EVENTS;

import static com.android.networkstack.apishim.ConstantsShim.DETECTION_METHOD_TCP_METRICS;

import static com.android.networkstack.apishim.ConstantsShim.DETECTION_METHOD_TCP_METRICS;

import static com.android.networkstack.apishim.ConstantsShim.TRANSPORT_TEST;

import static com.android.networkstack.util.DnsUtils.PRIVATE_DNS_PROBE_HOST_SUFFIX;

import static com.android.networkstack.util.DnsUtils.PRIVATE_DNS_PROBE_HOST_SUFFIX;

import static com.android.networkstack.util.DnsUtils.TYPE_ADDRCONFIG;

import static com.android.networkstack.util.DnsUtils.TYPE_ADDRCONFIG;
@@ -387,7 +388,8 @@ public class NetworkMonitor extends StateMachine { 
    private static final int CMD_BANDWIDTH_CHECK_TIMEOUT = 24;

    private static final int CMD_BANDWIDTH_CHECK_TIMEOUT = 24;





    // Start mReevaluateDelayMs at this value and double.

    // Start mReevaluateDelayMs at this value and double.

    private static final int INITIAL_REEVALUATE_DELAY_MS = 1000;

    @VisibleForTesting

    static final int INITIAL_REEVALUATE_DELAY_MS = 1000;

    private static final int MAX_REEVALUATE_DELAY_MS = 10 * 60 * 1000;

    private static final int MAX_REEVALUATE_DELAY_MS = 10 * 60 * 1000;

    // Default timeout of evaluating network bandwidth.

    // Default timeout of evaluating network bandwidth.

    private static final int DEFAULT_EVALUATING_BANDWIDTH_TIMEOUT_MS = 10_000;

    private static final int DEFAULT_EVALUATING_BANDWIDTH_TIMEOUT_MS = 10_000;
@@ -1434,8 +1436,9 @@ public class NetworkMonitor extends StateMachine { 
            }

            }





            final int token = ++mProbeToken;

            final int token = ++mProbeToken;

            final EvaluationThreadDeps deps = new EvaluationThreadDeps(mNetworkCapabilities);

            mThread = new Thread(() -> sendMessage(obtainMessage(CMD_PROBE_COMPLETE, token, 0,

            mThread = new Thread(() -> sendMessage(obtainMessage(CMD_PROBE_COMPLETE, token, 0,

                    isCaptivePortal())));

                    isCaptivePortal(deps))));

            mThread.start();

            mThread.start();

        }

        }
@@ -2147,8 +2150,25 @@ public class NetworkMonitor extends StateMachine { 
        return mCaptivePortalFallbackSpecs[idx];

        return mCaptivePortalFallbackSpecs[idx];

    }

    }





    @VisibleForTesting

    /**

    protected CaptivePortalProbeResult isCaptivePortal() {

     * Parameters that can be accessed by the evaluation thread in a thread-safe way.

     *

     * Parameters such as LinkProperties and NetworkCapabilities cannot be accessed by the

     * evaluation thread directly, as they are managed in the state machine thread and not

     * synchronized. This class provides a copy of the required data that is not modified and can be

     * used safely by the evaluation thread.

     */

    private static class EvaluationThreadDeps {

        // TODO: add parameters that are accessed in a non-thread-safe way from the evaluation

        // thread (read from LinkProperties, NetworkCapabilities, useHttps, validationStage)

        private final boolean mIsTestNetwork;



        EvaluationThreadDeps(NetworkCapabilities nc) {

            this.mIsTestNetwork = nc.hasTransport(TRANSPORT_TEST);

        }

    }



    private CaptivePortalProbeResult isCaptivePortal(EvaluationThreadDeps deps) {

        if (!mIsCaptivePortalCheckEnabled) {

        if (!mIsCaptivePortalCheckEnabled) {

            validationLog("Validation disabled.");

            validationLog("Validation disabled.");

            return CaptivePortalProbeResult.success(CaptivePortalProbeResult.PROBE_UNKNOWN);

            return CaptivePortalProbeResult.success(CaptivePortalProbeResult.PROBE_UNKNOWN);
@@ -2196,11 +2216,11 @@ public class NetworkMonitor extends StateMachine { 
            reportHttpProbeResult(NETWORK_VALIDATION_PROBE_HTTP, result);

            reportHttpProbeResult(NETWORK_VALIDATION_PROBE_HTTP, result);

        } else if (mUseHttps && httpsUrls.length == 1 && httpUrls.length == 1) {

        } else if (mUseHttps && httpsUrls.length == 1 && httpUrls.length == 1) {

            // Probe results are reported inside sendHttpAndHttpsParallelWithFallbackProbes.

            // Probe results are reported inside sendHttpAndHttpsParallelWithFallbackProbes.

            result = sendHttpAndHttpsParallelWithFallbackProbes(

            result = sendHttpAndHttpsParallelWithFallbackProbes(deps, proxyInfo,

                    proxyInfo, httpsUrls[0], httpUrls[0]);

                    httpsUrls[0], httpUrls[0]);

        } else if (mUseHttps) {

        } else if (mUseHttps) {

            // Support result aggregation from multiple Urls.

            // Support result aggregation from multiple Urls.

            result = sendMultiParallelHttpAndHttpsProbes(proxyInfo, httpsUrls, httpUrls);

            result = sendMultiParallelHttpAndHttpsProbes(deps, proxyInfo, httpsUrls, httpUrls);

        } else {

        } else {

            result = sendDnsAndHttpProbes(proxyInfo, httpUrls[0], ValidationProbeEvent.PROBE_HTTP);

            result = sendDnsAndHttpProbes(proxyInfo, httpUrls[0], ValidationProbeEvent.PROBE_HTTP);

            reportHttpProbeResult(NETWORK_VALIDATION_PROBE_HTTP, result);

            reportHttpProbeResult(NETWORK_VALIDATION_PROBE_HTTP, result);
@@ -2482,12 +2502,12 @@ public class NetworkMonitor extends StateMachine { 
        private final CountDownLatch mLatch;

        private final CountDownLatch mLatch;

        private final Probe mProbe;

        private final Probe mProbe;





        ProbeThread(CountDownLatch latch, ProxyInfo proxy, URL url, int probeType,

        ProbeThread(CountDownLatch latch, EvaluationThreadDeps deps, ProxyInfo proxy, URL url,

                Uri captivePortalApiUrl) {

                int probeType, Uri captivePortalApiUrl) {

            mLatch = latch;

            mLatch = latch;

            mProbe = (probeType == ValidationProbeEvent.PROBE_HTTPS)

            mProbe = (probeType == ValidationProbeEvent.PROBE_HTTPS)

                    ? new HttpsProbe(proxy, url, captivePortalApiUrl)

                    ? new HttpsProbe(deps, proxy, url, captivePortalApiUrl)

                    : new HttpProbe(proxy, url, captivePortalApiUrl);

                    : new HttpProbe(deps, proxy, url, captivePortalApiUrl);

            mResult = CaptivePortalProbeResult.failed(probeType);

            mResult = CaptivePortalProbeResult.failed(probeType);

        }

        }
@@ -2512,11 +2532,14 @@ public class NetworkMonitor extends StateMachine { 
    }

    }





    private abstract static class Probe {

    private abstract static class Probe {

        protected final EvaluationThreadDeps mDeps;

        protected final ProxyInfo mProxy;

        protected final ProxyInfo mProxy;

        protected final URL mUrl;

        protected final URL mUrl;

        protected final Uri mCaptivePortalApiUrl;

        protected final Uri mCaptivePortalApiUrl;





        protected Probe(ProxyInfo proxy, URL url, Uri captivePortalApiUrl) {

        protected Probe(EvaluationThreadDeps deps, ProxyInfo proxy, URL url,

                Uri captivePortalApiUrl) {

            mDeps = deps;

            mProxy = proxy;

            mProxy = proxy;

            mUrl = url;

            mUrl = url;

            mCaptivePortalApiUrl = captivePortalApiUrl;

            mCaptivePortalApiUrl = captivePortalApiUrl;
@@ -2526,8 +2549,8 @@ public class NetworkMonitor extends StateMachine { 
    }

    }





    final class HttpsProbe extends Probe {

    final class HttpsProbe extends Probe {

        HttpsProbe(ProxyInfo proxy, URL url, Uri captivePortalApiUrl) {

        HttpsProbe(EvaluationThreadDeps deps, ProxyInfo proxy, URL url, Uri captivePortalApiUrl) {

            super(proxy, url, captivePortalApiUrl);

            super(deps, proxy, url, captivePortalApiUrl);

        }

        }





        @Override

        @Override
@@ -2537,8 +2560,8 @@ public class NetworkMonitor extends StateMachine { 
    }

    }





    final class HttpProbe extends Probe {

    final class HttpProbe extends Probe {

        HttpProbe(ProxyInfo proxy, URL url, Uri captivePortalApiUrl) {

        HttpProbe(EvaluationThreadDeps deps, ProxyInfo proxy, URL url, Uri captivePortalApiUrl) {

            super(proxy, url, captivePortalApiUrl);

            super(deps, proxy, url, captivePortalApiUrl);

        }

        }





        private CaptivePortalDataShim tryCapportApiProbe() {

        private CaptivePortalDataShim tryCapportApiProbe() {
@@ -2551,9 +2574,9 @@ public class NetworkMonitor extends StateMachine { 
                // Protocol must be HTTPS

                // Protocol must be HTTPS

                // (as per https://www.ietf.org/id/draft-ietf-capport-api-07.txt, #4).

                // (as per https://www.ietf.org/id/draft-ietf-capport-api-07.txt, #4).

                // Only allow HTTP on localhost, for testing.

                // Only allow HTTP on localhost, for testing.

                final boolean isLocalhostHttp =

                final boolean isTestLocalhostHttp = mDeps.mIsTestNetwork

                        "localhost".equals(url.getHost()) && "http".equals(url.getProtocol());

                        && "localhost".equals(url.getHost()) && "http".equals(url.getProtocol());

                if (!"https".equals(url.getProtocol()) && !isLocalhostHttp) {

                if (!"https".equals(url.getProtocol()) && !isTestLocalhostHttp) {

                    validationLog("Invalid captive portal API protocol: " + url.getProtocol());

                    validationLog("Invalid captive portal API protocol: " + url.getProtocol());

                    return null;

                    return null;

                }

                }
@@ -2636,8 +2659,9 @@ public class NetworkMonitor extends StateMachine { 
                        && captivePortalApiUrl == null);

                        && captivePortalApiUrl == null);

    }

    }





    private CaptivePortalProbeResult sendMultiParallelHttpAndHttpsProbes(@NonNull ProxyInfo proxy,

    private CaptivePortalProbeResult sendMultiParallelHttpAndHttpsProbes(

            @NonNull URL[] httpsUrls, @NonNull URL[] httpUrls) {

            @NonNull EvaluationThreadDeps deps, @Nullable ProxyInfo proxy, @NonNull URL[] httpsUrls,

            @NonNull URL[] httpUrls) {

        // If multiple URLs are required to ensure the correctness of validation, send parallel

        // If multiple URLs are required to ensure the correctness of validation, send parallel

        // probes to explore the result in separate probe threads and aggregate those results into

        // probes to explore the result in separate probe threads and aggregate those results into

        // one as the final result for either HTTP or HTTPS.

        // one as the final result for either HTTP or HTTPS.
@@ -2662,13 +2686,13 @@ public class NetworkMonitor extends StateMachine { 
            // TODO: Have the capport probe as a different probe for cleanliness.

            // TODO: Have the capport probe as a different probe for cleanliness.

            final URL urlMaybeWithCapport = httpUrls[0];

            final URL urlMaybeWithCapport = httpUrls[0];

            for (final URL url : httpUrls) {

            for (final URL url : httpUrls) {

                futures.add(ecs.submit(() -> new HttpProbe(proxy, url,

                futures.add(ecs.submit(() -> new HttpProbe(deps, proxy, url,

                        url.equals(urlMaybeWithCapport) ? capportApiUrl : null).sendProbe()));

                        url.equals(urlMaybeWithCapport) ? capportApiUrl : null).sendProbe()));

            }

            }





            for (final URL url : httpsUrls) {

            for (final URL url : httpsUrls) {

                futures.add(

                futures.add(ecs.submit(() -> new HttpsProbe(deps, proxy, url, capportApiUrl)

                        ecs.submit(() -> new HttpsProbe(proxy, url, capportApiUrl).sendProbe()));

                        .sendProbe()));

            }

            }





            final ArrayList<CaptivePortalProbeResult> completedProbes = new ArrayList<>();

            final ArrayList<CaptivePortalProbeResult> completedProbes = new ArrayList<>();
@@ -2764,15 +2788,15 @@ public class NetworkMonitor extends StateMachine { 
    }

    }





    private CaptivePortalProbeResult sendHttpAndHttpsParallelWithFallbackProbes(

    private CaptivePortalProbeResult sendHttpAndHttpsParallelWithFallbackProbes(

            ProxyInfo proxy, URL httpsUrl, URL httpUrl) {

            EvaluationThreadDeps deps, ProxyInfo proxy, URL httpsUrl, URL httpUrl) {

        // Number of probes to wait for. If a probe completes with a conclusive answer

        // Number of probes to wait for. If a probe completes with a conclusive answer

        // it shortcuts the latch immediately by forcing the count to 0.

        // it shortcuts the latch immediately by forcing the count to 0.

        final CountDownLatch latch = new CountDownLatch(2);

        final CountDownLatch latch = new CountDownLatch(2);





        final Uri capportApiUrl = getCaptivePortalApiUrl(mLinkProperties);

        final Uri capportApiUrl = getCaptivePortalApiUrl(mLinkProperties);

        final ProbeThread httpsProbe = new ProbeThread(latch, proxy, httpsUrl,

        final ProbeThread httpsProbe = new ProbeThread(latch, deps, proxy, httpsUrl,

                ValidationProbeEvent.PROBE_HTTPS, capportApiUrl);

                ValidationProbeEvent.PROBE_HTTPS, capportApiUrl);

        final ProbeThread httpProbe = new ProbeThread(latch, proxy, httpUrl,

        final ProbeThread httpProbe = new ProbeThread(latch, deps, proxy, httpUrl,

                ValidationProbeEvent.PROBE_HTTP, capportApiUrl);

                ValidationProbeEvent.PROBE_HTTP, capportApiUrl);





        try {

        try {

tests/unit/src/com/android/server/connectivity/NetworkMonitorTest.java

+8 −4
Original line number Original line Diff line number Diff line
@@ -52,6 +52,7 @@ import static android.net.util.NetworkStackUtils.TEST_URL_EXPIRATION_TIME; 
import static android.provider.DeviceConfig.NAMESPACE_CONNECTIVITY;

import static android.provider.DeviceConfig.NAMESPACE_CONNECTIVITY;





import static com.android.networkstack.util.DnsUtils.PRIVATE_DNS_PROBE_HOST_SUFFIX;

import static com.android.networkstack.util.DnsUtils.PRIVATE_DNS_PROBE_HOST_SUFFIX;

import static com.android.server.connectivity.NetworkMonitor.INITIAL_REEVALUATE_DELAY_MS;

import static com.android.server.connectivity.NetworkMonitor.extractCharset;

import static com.android.server.connectivity.NetworkMonitor.extractCharset;





import static junit.framework.Assert.assertEquals;

import static junit.framework.Assert.assertEquals;
@@ -1113,10 +1114,13 @@ public class NetworkMonitorTest { 
        verify(mFallbackConnection, times(1)).getResponseCode();

        verify(mFallbackConnection, times(1)).getResponseCode();

        verify(mOtherFallbackConnection, never()).getResponseCode();

        verify(mOtherFallbackConnection, never()).getResponseCode();





        // Second check uses the URL chosen by Random

        // Second check should be triggered automatically after the reevaluate delay, and uses the

        final CaptivePortalProbeResult result = monitor.isCaptivePortal();

        // URL chosen by mRandom

        assertTrue(result.isPortal());

        // This test is appropriate to cover reevaluate behavior as long as the timeout is short

        verify(mOtherFallbackConnection, times(1)).getResponseCode();

        assertTrue(INITIAL_REEVALUATE_DELAY_MS < 2000);

        verify(mOtherFallbackConnection, timeout(INITIAL_REEVALUATE_DELAY_MS + HANDLER_TIMEOUT_MS))

                .getResponseCode();

        verifyNetworkTested(VALIDATION_RESULT_PORTAL, 0 /* probesSucceeded */, TEST_LOGIN_URL);

    }

    }





    @Test

    @Test