From b291ab643505ffacd07b0a897db3f2db639baae8 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 7 Jan 2021 23:33:18 +0900 Subject: [PATCH 1/3] Test for bugs with suspended VPN underlying networks. Test: atest --rerun-until-failure 100 ConnectivityServiceTest#testVpnSwitchFromSuspendedToNonSuspended Change-Id: Ia52f9cafef3f49ae70ad135d017e207eb57fddfe --- .../server/ConnectivityServiceTest.java | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 5a375d2301..c652261b58 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5894,6 +5894,121 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(callback); } + private void assertGetNetworkInfoOfGetActiveNetworkIsConnected(boolean expectedConnectivity) { + // What Chromium used to do before https://chromium-review.googlesource.com/2605304 + assertEquals("Unexpected result for getActiveNetworkInfo(getActiveNetwork())", + expectedConnectivity, mCm.getNetworkInfo(mCm.getActiveNetwork()).isConnected()); + } + + @Test + public void testVpnUnderlyingNetworkSuspended() throws Exception { + final TestNetworkCallback callback = new TestNetworkCallback(); + mCm.registerDefaultNetworkCallback(callback); + + // Connect a VPN. + mMockVpn.establishForMyUid(false, true, false); + callback.expectAvailableCallbacksUnvalidated(mMockVpn); + + // Connect cellular data. + mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); + mCellNetworkAgent.connect(false /* validated */); + callback.expectCapabilitiesThat(mMockVpn, + nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + callback.assertNoCallback(); + + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertGetNetworkInfoOfGetActiveNetworkIsConnected(true); + + // Suspend the cellular network and expect the VPN to be suspended. + mCellNetworkAgent.suspend(); + callback.expectCapabilitiesThat(mMockVpn, + nc -> !nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + callback.expectCallback(CallbackEntry.SUSPENDED, mMockVpn); + callback.assertNoCallback(); + + assertFalse(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); + assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); + // VPN's main underlying network is suspended, so no connectivity. + assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + + // Switch to another network. The VPN should no longer be suspended. + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); + mWiFiNetworkAgent.connect(false /* validated */); + callback.expectCapabilitiesThat(mMockVpn, + nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_WIFI)); + + // BUG: the VPN is no longer suspended, so a RESUMED callback should have been sent. + // callback.expectCallback(CallbackEntry.RESUMED, mMockVpn); + callback.assertNoCallback(); + + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertNetworkInfo(TYPE_MOBILE, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); // BUG: VPN caps have NOT_SUSPENDED. + assertActiveNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); + // BUG: the device has connectivity, so this should return true. + assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + + // Unsuspend cellular and then switch back to it. + // The same bug happens in the opposite direction: the VPN's capabilities correctly have + // NOT_SUSPENDED, but the VPN's NetworkInfo is in state SUSPENDED. + mCellNetworkAgent.resume(); + mWiFiNetworkAgent.disconnect(); + callback.expectCapabilitiesThat(mMockVpn, + nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + // Spurious double callback? + callback.expectCapabilitiesThat(mMockVpn, + nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + callback.assertNoCallback(); + + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); // BUG: VPN caps have NOT_SUSPENDED. + assertNull(mCm.getActiveNetworkInfo()); // ??? + // BUG: the device has connectivity, so this should return true. + assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + + // Suspending and resuming reveals other bugs. + mCellNetworkAgent.suspend(); + callback.assertNoCallback(); // BUG: should get callback that VPN is suspended. + + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); // BUG: VPN should be SUSPENDED. + assertNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); + assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); + assertNull(mCm.getActiveNetworkInfo()); // ??? + assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + + mCellNetworkAgent.resume(); + callback.assertNoCallback(); // BUG: should get callback that VPN is no longer suspended. + + assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); + assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); + assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); + assertNull(mCm.getActiveNetworkInfo()); // ??? + assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + } + @Test public void testVpnNetworkActive() throws Exception { final int uid = Process.myUid(); From 24c152ce87e9b9047c1d444d353c65522b91009c Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 12 Jan 2021 00:34:44 +0900 Subject: [PATCH 2/3] Fix propagating underlying caps when a network disconnects. aosp/1513052, which generalized support for underlying networks, broke default network switching when the network underlying a VPN disconnects. This is because it calls propagateUnderlyingNetworkCapabilities in the middle of the bookkeeping operations needed when a network is disconnected (specifically, after all satisified requests are removed from the disconnecting network, but before mDefaultNetworkNai is updated). This is completely incorrect because propagateUnderlyingNetworkCapabilities can trigger a network rematch, and running a rematch when the request data structures are inconsistent is obviously wrong. See the test changes in this CL for an example of the damage. Fix this by moving propagateUnderlyingNetworkCapabilities to before the bookeeping operations begin. It must be before mDefaultNetworkNai is updated, because otherwise it will not know that the default network is disconnecting, and it will not be able to propagate capabilities to VPNs that set underlying networks to null (i.e., to the default network). It must be after the nai is removed from mNetworkForNetId because otherwise it will think that the underlying network is still connected. Bug: 173331190 Test: accompanying unit test shows lots of bugs removed Change-Id: Ibf376a6fa4b34d1c96f8506fa8abbb7595a8c272 --- .../android/server/ConnectivityService.java | 2 +- .../server/ConnectivityServiceTest.java | 28 ++++++++++++------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 90a17e7dff..9012fbc670 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3466,6 +3466,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // available until we've told netd to delete it below. mNetworkForNetId.remove(nai.network.getNetId()); } + propagateUnderlyingNetworkCapabilities(nai.network); // Remove all previously satisfied requests. for (int i = 0; i < nai.numNetworkRequests(); i++) { NetworkRequest request = nai.requestAt(i); @@ -3478,7 +3479,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } } nai.clearLingerState(); - propagateUnderlyingNetworkCapabilities(nai.network); if (nai.isSatisfyingRequest(mDefaultRequest.requestId)) { mDefaultNetworkNai = null; updateDataActivityTracking(null /* newNetwork */, nai); diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index c652261b58..bca498688f 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5981,32 +5981,40 @@ public class ConnectivityServiceTest { assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); // BUG: VPN caps have NOT_SUSPENDED. - assertNull(mCm.getActiveNetworkInfo()); // ??? + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); // BUG: the device has connectivity, so this should return true. assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); - // Suspending and resuming reveals other bugs. + // Re-suspending the current network fixes the problem. mCellNetworkAgent.suspend(); - callback.assertNoCallback(); // BUG: should get callback that VPN is suspended. + callback.expectCapabilitiesThat(mMockVpn, + nc -> !nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + callback.expectCallback(CallbackEntry.SUSPENDED, mMockVpn); + callback.assertNoCallback(); - assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) - .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); // BUG: VPN should be SUSPENDED. + assertFalse(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) + .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); assertNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); - assertNull(mCm.getActiveNetworkInfo()); // ??? + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.SUSPENDED); assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); mCellNetworkAgent.resume(); - callback.assertNoCallback(); // BUG: should get callback that VPN is no longer suspended. + callback.expectCapabilitiesThat(mMockVpn, + nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) + && nc.hasTransport(TRANSPORT_CELLULAR)); + callback.expectCallback(CallbackEntry.RESUMED, mMockVpn); + callback.assertNoCallback(); assertTrue(mCm.getNetworkCapabilities(mMockVpn.getNetwork()) .hasCapability(NET_CAPABILITY_NOT_SUSPENDED)); assertNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); assertNetworkInfo(TYPE_WIFI, DetailedState.DISCONNECTED); - assertNetworkInfo(TYPE_VPN, DetailedState.SUSPENDED); - assertNull(mCm.getActiveNetworkInfo()); // ??? - assertGetNetworkInfoOfGetActiveNetworkIsConnected(false); + assertNetworkInfo(TYPE_VPN, DetailedState.CONNECTED); + assertActiveNetworkInfo(TYPE_MOBILE, DetailedState.CONNECTED); + assertGetNetworkInfoOfGetActiveNetworkIsConnected(true); } @Test From c5ba160582930612708294289d9edeaf7738e28e Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 19 Jan 2021 10:24:25 +0900 Subject: [PATCH 3/3] Address comments on aosp/1539753, aosp/1542487 and aosp/1547496. Bug: 173331190 Test: test-only change Change-Id: I475502fde55d24e7ae3f7fe9f43c54740c57a9cf --- .../android/server/ConnectivityService.java | 5 ++++ .../server/ConnectivityServiceTest.java | 28 +++++++++++++------ 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 9012fbc670..7541833b15 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3479,6 +3479,9 @@ public class ConnectivityService extends IConnectivityManager.Stub } } nai.clearLingerState(); + // TODO: this loop, and the mLegacyTypeTracker.remove just below it, seem redundant given + // there's a full rematch right after. Currently, deleting it breaks tests that check for + // the default network disconnecting. Find out why, fix the rematch code, and delete this. if (nai.isSatisfyingRequest(mDefaultRequest.requestId)) { mDefaultNetworkNai = null; updateDataActivityTracking(null /* newNetwork */, nai); @@ -4995,6 +4998,8 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public boolean updateLockdownVpn() { + // Allow the system UID for the system server and for Settings. + // Also, for unit tests, allow the process that ConnectivityService is running in. if (mDeps.getCallingUid() != Process.SYSTEM_UID && Binder.getCallingPid() != Process.myPid()) { logw("Lockdown VPN only available to system process or AID_SYSTEM"); diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index bca498688f..37307a46b8 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -1389,9 +1389,6 @@ public class ConnectivityServiceTest { verify(mNetworkPolicyManager).registerListener(policyListenerCaptor.capture()); mPolicyListener = policyListenerCaptor.getValue(); - mServiceContext.setPermission( - Manifest.permission.CONTROL_VPN, PERMISSION_GRANTED); - // Create local CM before sending system ready so that we can answer // getSystemService() correctly. mCm = new WrappedConnectivityManager(InstrumentationRegistry.getContext(), mService); @@ -1588,6 +1585,7 @@ public class ConnectivityServiceTest { } public void expectNoBroadcast(int timeoutMs) throws Exception { + waitForIdle(); try { final Intent intent = get(timeoutMs, TimeUnit.MILLISECONDS); fail("Unexpected broadcast: " + intent.getAction() + " " + intent.getExtras()); @@ -5906,7 +5904,8 @@ public class ConnectivityServiceTest { mCm.registerDefaultNetworkCallback(callback); // Connect a VPN. - mMockVpn.establishForMyUid(false, true, false); + mMockVpn.establishForMyUid(false /* validated */, true /* hasInternet */, + false /* isStrictMode */); callback.expectAvailableCallbacksUnvalidated(mMockVpn); // Connect cellular data. @@ -5966,6 +5965,7 @@ public class ConnectivityServiceTest { // The same bug happens in the opposite direction: the VPN's capabilities correctly have // NOT_SUSPENDED, but the VPN's NetworkInfo is in state SUSPENDED. mCellNetworkAgent.resume(); + callback.assertNoCallback(); mWiFiNetworkAgent.disconnect(); callback.expectCapabilitiesThat(mMockVpn, nc -> nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED) @@ -6573,9 +6573,13 @@ public class ConnectivityServiceTest { @Test public void testLockdownVpnWithRestrictedProfiles() throws Exception { - // NETWORK_SETTINGS is necessary to see the UID ranges in NetworkCapabilities. + // For ConnectivityService#setAlwaysOnVpnPackage. mServiceContext.setPermission( Manifest.permission.CONTROL_ALWAYS_ON_VPN, PERMISSION_GRANTED); + // For call Vpn#setAlwaysOnPackage. + mServiceContext.setPermission( + Manifest.permission.CONTROL_VPN, PERMISSION_GRANTED); + // Necessary to see the UID ranges in NetworkCapabilities. mServiceContext.setPermission( Manifest.permission.NETWORK_SETTINGS, PERMISSION_GRANTED); @@ -7138,6 +7142,9 @@ public class ConnectivityServiceTest { @Test public void testLegacyLockdownVpn() throws Exception { + mServiceContext.setPermission( + Manifest.permission.CONTROL_VPN, PERMISSION_GRANTED); + final NetworkRequest request = new NetworkRequest.Builder().clearCapabilities().build(); final TestNetworkCallback callback = new TestNetworkCallback(); mCm.registerNetworkCallback(request, callback); @@ -7255,11 +7262,14 @@ public class ConnectivityServiceTest { mMockVpn.expectStopVpnRunnerPrivileged(); mMockVpn.expectStartLegacyVpnRunner(); - // TODO: why is wifi not blocked? Is this because something calls prepare()? + // TODO: why is wifi not blocked? Is it because when this callback is sent, the VPN is still + // connected, so the network is not considered blocked by the lockdown UID ranges? But the + // fact that a VPN is connected should only result in the VPN itself being unblocked, not + // any other network. Bug in isUidBlockedByVpn? callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); - callback.expectCapabilitiesThat(mMockVpn, (nc) -> nc.hasTransport(TRANSPORT_WIFI)); - defaultCallback.expectCapabilitiesThat(mMockVpn, (nc) -> nc.hasTransport(TRANSPORT_WIFI)); + callback.expectCapabilitiesThat(mMockVpn, nc -> nc.hasTransport(TRANSPORT_WIFI)); callback.expectCallback(CallbackEntry.LOST, mMockVpn); + defaultCallback.expectCapabilitiesThat(mMockVpn, nc -> nc.hasTransport(TRANSPORT_WIFI)); defaultCallback.expectCallback(CallbackEntry.LOST, mMockVpn); defaultCallback.expectAvailableCallbacksUnvalidatedAndBlocked(mWiFiNetworkAgent); @@ -7302,7 +7312,7 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent.disconnect(); callback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); b1.expectBroadcast(); - callback.expectCapabilitiesThat(mMockVpn, (nc) -> !nc.hasTransport(TRANSPORT_WIFI)); + callback.expectCapabilitiesThat(mMockVpn, nc -> !nc.hasTransport(TRANSPORT_WIFI)); b2 = expectConnectivityAction(TYPE_VPN, DetailedState.DISCONNECTED); mMockVpn.expectStopVpnRunnerPrivileged(); callback.expectCallback(CallbackEntry.LOST, mMockVpn);