From 053b2a2737a8611eec64b004996b9ff1598403e8 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 18 Mar 2021 20:57:11 +0900 Subject: [PATCH 1/2] Test a bug with NETWORK_SETTINGS+registerDefaultNetworkCallback. Currently, if a process with NETWORK_SETTINGS registers a default network callback, its uid will be ignored and replaced with an empty list of UIDs. This means it will incorrectly match VPNs with any UID range. Add a test for this bug to make it easier to review the upcoming change that fixes it. Bug: 165835257 Test: test-only change Change-Id: If58524b01fdd60045fb7236d17dedf31fb563f99 --- .../android/server/ConnectivityServiceTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index cc1bee5a51..2ec8707000 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -7487,6 +7487,9 @@ public class ConnectivityServiceTest { final NetworkRequest vpnUidRequest = new NetworkRequest.Builder().build(); registerNetworkCallbackAsUid(vpnUidRequest, vpnUidCallback, VPN_UID); + final TestNetworkCallback vpnUidDefaultCallback = new TestNetworkCallback(); + registerDefaultNetworkCallbackAsUid(vpnUidDefaultCallback, VPN_UID); + final int uid = Process.myUid(); final int userId = UserHandle.getUserId(uid); final ArrayList allowList = new ArrayList<>(); @@ -7505,6 +7508,7 @@ public class ConnectivityServiceTest { callback.expectAvailableCallbacksUnvalidatedAndBlocked(mWiFiNetworkAgent); defaultCallback.expectAvailableCallbacksUnvalidatedAndBlocked(mWiFiNetworkAgent); vpnUidCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + vpnUidDefaultCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetworkForUid(VPN_UID)); assertNull(mCm.getActiveNetwork()); assertActiveNetworkInfo(TYPE_WIFI, DetailedState.BLOCKED); @@ -7517,6 +7521,7 @@ public class ConnectivityServiceTest { callback.expectBlockedStatusCallback(false, mWiFiNetworkAgent); defaultCallback.expectBlockedStatusCallback(false, mWiFiNetworkAgent); vpnUidCallback.assertNoCallback(); + vpnUidDefaultCallback.assertNoCallback(); expectNetworkRejectNonSecureVpn(inOrder, false, firstHalf, secondHalf); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetworkForUid(VPN_UID)); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); @@ -7531,6 +7536,7 @@ public class ConnectivityServiceTest { callback.assertNoCallback(); defaultCallback.assertNoCallback(); vpnUidCallback.assertNoCallback(); + vpnUidDefaultCallback.assertNoCallback(); // The following requires that the UID of this test package is greater than VPN_UID. This // is always true in practice because a plain AOSP build with no apps installed has almost @@ -7551,6 +7557,7 @@ public class ConnectivityServiceTest { callback.expectAvailableCallbacksUnvalidated(mCellNetworkAgent); defaultCallback.assertNoCallback(); vpnUidCallback.expectAvailableCallbacksUnvalidated(mCellNetworkAgent); + vpnUidDefaultCallback.assertNoCallback(); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetworkForUid(VPN_UID)); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); assertActiveNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); @@ -7571,6 +7578,7 @@ public class ConnectivityServiceTest { defaultCallback.expectBlockedStatusCallback(true, mWiFiNetworkAgent); assertBlockedCallbackInAnyOrder(callback, true, mWiFiNetworkAgent, mCellNetworkAgent); vpnUidCallback.assertNoCallback(); + vpnUidDefaultCallback.assertNoCallback(); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetworkForUid(VPN_UID)); assertNull(mCm.getActiveNetwork()); assertActiveNetworkInfo(TYPE_WIFI, DetailedState.BLOCKED); @@ -7582,6 +7590,7 @@ public class ConnectivityServiceTest { defaultCallback.expectBlockedStatusCallback(false, mWiFiNetworkAgent); assertBlockedCallbackInAnyOrder(callback, false, mWiFiNetworkAgent, mCellNetworkAgent); vpnUidCallback.assertNoCallback(); + vpnUidDefaultCallback.assertNoCallback(); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetworkForUid(VPN_UID)); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); assertActiveNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); @@ -7596,6 +7605,7 @@ public class ConnectivityServiceTest { callback.assertNoCallback(); defaultCallback.assertNoCallback(); vpnUidCallback.assertNoCallback(); + vpnUidDefaultCallback.assertNoCallback(); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetworkForUid(VPN_UID)); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); assertActiveNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); @@ -7607,6 +7617,7 @@ public class ConnectivityServiceTest { callback.assertNoCallback(); defaultCallback.assertNoCallback(); vpnUidCallback.assertNoCallback(); + vpnUidDefaultCallback.assertNoCallback(); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetworkForUid(VPN_UID)); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); assertActiveNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); @@ -7619,6 +7630,7 @@ public class ConnectivityServiceTest { defaultCallback.expectBlockedStatusCallback(true, mWiFiNetworkAgent); assertBlockedCallbackInAnyOrder(callback, true, mWiFiNetworkAgent, mCellNetworkAgent); vpnUidCallback.assertNoCallback(); + vpnUidDefaultCallback.assertNoCallback(); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetworkForUid(VPN_UID)); assertNull(mCm.getActiveNetwork()); assertActiveNetworkInfo(TYPE_WIFI, DetailedState.BLOCKED); @@ -7629,6 +7641,8 @@ public class ConnectivityServiceTest { assertUidRangesUpdatedForMyUid(true); defaultCallback.expectAvailableThenValidatedCallbacks(mMockVpn); vpnUidCallback.assertNoCallback(); // vpnUidCallback has NOT_VPN capability. + // TODO: this is a bug. The VPN does not apply to VPN_UID. + vpnUidDefaultCallback.expectAvailableThenValidatedCallbacks(mMockVpn); assertEquals(mMockVpn.getNetwork(), mCm.getActiveNetwork()); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetworkForUid(VPN_UID)); assertActiveNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); @@ -7639,11 +7653,14 @@ public class ConnectivityServiceTest { mMockVpn.disconnect(); defaultCallback.expectCallback(CallbackEntry.LOST, mMockVpn); defaultCallback.expectAvailableCallbacksUnvalidatedAndBlocked(mWiFiNetworkAgent); + vpnUidCallback.assertNoCallback(); + vpnUidDefaultCallback.expectCallback(CallbackEntry.LOST, mMockVpn); // BUG! assertNull(mCm.getActiveNetwork()); mCm.unregisterNetworkCallback(callback); mCm.unregisterNetworkCallback(defaultCallback); mCm.unregisterNetworkCallback(vpnUidCallback); + mCm.unregisterNetworkCallback(vpnUidDefaultCallback); } private void setupLegacyLockdownVpn() { From c62368fd7ae1004acd60bc1de06118d8f5d879c9 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 22 Mar 2021 02:12:04 +0900 Subject: [PATCH 2/2] Fix privileged apps calling registerDefaultNetworkCallback. When registerDefaultNetworkCallback is called by an app that has NETWORK_SETTINGS, the UID of the app is forgotten and the request that is filed has an empty UID set. This results in that request matching networks that have UID ranges that do not include it, e.g., VPNs. Fix this by ensuring that the UID ranges are properly set. Bug: 165835257 Test: updated specific tests for this bug Change-Id: I90bf79573342c144d1cfbc2f61a3155fdd5b1fa7 --- .../core/java/com/android/server/ConnectivityService.java | 7 ++++++- .../java/com/android/server/ConnectivityServiceTest.java | 5 ++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 3923063096..6fcb3f74c7 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6116,10 +6116,15 @@ public class ConnectivityService extends IConnectivityManager.Stub private NetworkCapabilities copyDefaultNetworkCapabilitiesForUid( @NonNull final NetworkCapabilities netCapToCopy, @NonNull final int requestorUid, @NonNull final String requestorPackageName) { + // These capabilities are for a TRACK_DEFAULT callback, so: + // 1. Remove NET_CAPABILITY_VPN, because it's (currently!) the only difference between + // mDefaultRequest and a per-UID default request. + // TODO: stop depending on the fact that these two unrelated things happen to be the same + // 2. Always set the UIDs to mAsUid. restrictRequestUidsForCallerAndSetRequestorInfo will + // not do this in the case of a privileged application. final NetworkCapabilities netCap = new NetworkCapabilities(netCapToCopy); netCap.removeCapability(NET_CAPABILITY_NOT_VPN); netCap.setSingleUid(requestorUid); - netCap.setUids(new ArraySet<>()); restrictRequestUidsForCallerAndSetRequestorInfo( netCap, requestorUid, requestorPackageName); return netCap; diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 2ec8707000..ed9a44b614 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -7641,8 +7641,7 @@ public class ConnectivityServiceTest { assertUidRangesUpdatedForMyUid(true); defaultCallback.expectAvailableThenValidatedCallbacks(mMockVpn); vpnUidCallback.assertNoCallback(); // vpnUidCallback has NOT_VPN capability. - // TODO: this is a bug. The VPN does not apply to VPN_UID. - vpnUidDefaultCallback.expectAvailableThenValidatedCallbacks(mMockVpn); + vpnUidDefaultCallback.assertNoCallback(); // VPN does not apply to VPN_UID assertEquals(mMockVpn.getNetwork(), mCm.getActiveNetwork()); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetworkForUid(VPN_UID)); assertActiveNetworkInfo(TYPE_WIFI, DetailedState.CONNECTED); @@ -7654,7 +7653,7 @@ public class ConnectivityServiceTest { defaultCallback.expectCallback(CallbackEntry.LOST, mMockVpn); defaultCallback.expectAvailableCallbacksUnvalidatedAndBlocked(mWiFiNetworkAgent); vpnUidCallback.assertNoCallback(); - vpnUidDefaultCallback.expectCallback(CallbackEntry.LOST, mMockVpn); // BUG! + vpnUidDefaultCallback.assertNoCallback(); assertNull(mCm.getActiveNetwork()); mCm.unregisterNetworkCallback(callback);