From 54e7806acf22cfa50ff5a7bc3f8afb400d49cf27 Mon Sep 17 00:00:00 2001 From: Chiachang Wang Date: Fri, 21 May 2021 04:54:39 +0000 Subject: [PATCH 1/3] Add test for NetworkCapabilities.Builder.withoutDefaultCapabilities Bug: 186061922 Test: atest CtsNetTestCases:android.net.NetworkCapabilitiesTest Merged-In: I369e71dd6ae85da78e114ea8377967ab0bde787b Change-Id: I369e71dd6ae85da78e114ea8377967ab0bde787b (cherry picked from commit 0f3913d99836c61a3daf66f1631b268ba27c4b96) --- .../java/android/net/NetworkCapabilitiesTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/common/java/android/net/NetworkCapabilitiesTest.java b/tests/common/java/android/net/NetworkCapabilitiesTest.java index 9efdde4da0..9537786055 100644 --- a/tests/common/java/android/net/NetworkCapabilitiesTest.java +++ b/tests/common/java/android/net/NetworkCapabilitiesTest.java @@ -33,6 +33,7 @@ import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_VPN; import static android.net.NetworkCapabilities.NET_CAPABILITY_OEM_PAID; import static android.net.NetworkCapabilities.NET_CAPABILITY_OEM_PRIVATE; import static android.net.NetworkCapabilities.NET_CAPABILITY_PARTIAL_CONNECTIVITY; +import static android.net.NetworkCapabilities.NET_CAPABILITY_TRUSTED; import static android.net.NetworkCapabilities.NET_CAPABILITY_VALIDATED; import static android.net.NetworkCapabilities.NET_CAPABILITY_WIFI_P2P; import static android.net.NetworkCapabilities.REDACT_FOR_ACCESS_FINE_LOCATION; @@ -1155,4 +1156,15 @@ public class NetworkCapabilitiesTest { assertEquals(Set.of(TEST_SUBID1), nc2.getSubscriptionIds()); } } + + @Test @IgnoreUpTo(Build.VERSION_CODES.R) + public void testBuilderWithoutDefaultCap() { + final NetworkCapabilities nc = + NetworkCapabilities.Builder.withoutDefaultCapabilities().build(); + assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_RESTRICTED)); + assertFalse(nc.hasCapability(NET_CAPABILITY_TRUSTED)); + assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_VPN)); + // Ensure test case fails if new net cap is added into default cap but no update here. + assertEquals(0, nc.getCapabilities().length); + } } From 043787dd373ebc91d0f0437a62f512432ed626f4 Mon Sep 17 00:00:00 2001 From: Treehugger Robot Date: Fri, 21 May 2021 02:42:59 +0000 Subject: [PATCH 2/3] Use CS identity to update setting while performing factory reset When apps try to call factoryReset to do networking reset, it will result in updating the setting in SettingsProvider. ContentProvider will verify if the package name of the caller that initiated the request being processed on the current thread. The package should belong to the calling UID. The setting update started from the ConnectivityService context, so the package will be android but the calling UID will be the calling app. It will cause a SecurityException. The behavior is fine previously as its known caller(Settings) shares system UID. But it will be a problem for other callers, such as CTS. Thus, clear the identity since the necessary permission check should be examined at the top of the method. The following actions should be fine to be proceed from the system itself. Also replace the user restriction check via hasUserRestrictionForUser with the UserHandle created from the calling uid to ensure it's verified with correct user. Bug: 186061922 Test: Factory reset from Settings Merged-In: If2dd69f702a1eafff331f9e71f6b92aeadfb715d Change-Id: If2dd69f702a1eafff331f9e71f6b92aeadfb715d (cherry picked from commit 235dd87ecf2b228ccb3117aec242da8680164d98) --- .../android/server/ConnectivityService.java | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 6027a99683..29a4856806 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -8655,28 +8655,32 @@ public class ConnectivityService extends IConnectivityManager.Stub public void factoryReset() { enforceSettingsPermission(); - if (mUserManager.hasUserRestriction(UserManager.DISALLOW_NETWORK_RESET)) { - return; - } - + final int uid = mDeps.getCallingUid(); final long token = Binder.clearCallingIdentity(); try { + if (mUserManager.hasUserRestrictionForUser(UserManager.DISALLOW_NETWORK_RESET, + UserHandle.getUserHandleForUid(uid))) { + return; + } + final IpMemoryStore ipMemoryStore = IpMemoryStore.getMemoryStore(mContext); ipMemoryStore.factoryReset(); + + // Turn airplane mode off + setAirplaneMode(false); + + // restore private DNS settings to default mode (opportunistic) + if (!mUserManager.hasUserRestrictionForUser(UserManager.DISALLOW_CONFIG_PRIVATE_DNS, + UserHandle.getUserHandleForUid(uid))) { + ConnectivitySettingsManager.setPrivateDnsMode(mContext, + PRIVATE_DNS_MODE_OPPORTUNISTIC); + } + + Settings.Global.putString(mContext.getContentResolver(), + ConnectivitySettingsManager.NETWORK_AVOID_BAD_WIFI, null); } finally { Binder.restoreCallingIdentity(token); } - - // Turn airplane mode off - setAirplaneMode(false); - - // restore private DNS settings to default mode (opportunistic) - if (!mUserManager.hasUserRestriction(UserManager.DISALLOW_CONFIG_PRIVATE_DNS)) { - ConnectivitySettingsManager.setPrivateDnsMode(mContext, PRIVATE_DNS_MODE_OPPORTUNISTIC); - } - - Settings.Global.putString(mContext.getContentResolver(), - ConnectivitySettingsManager.NETWORK_AVOID_BAD_WIFI, null); } @Override From 9f4273a9a5efa56b9c540df8a944a520b1edd65b Mon Sep 17 00:00:00 2001 From: Cody Kesting Date: Thu, 20 May 2021 22:57:07 +0000 Subject: [PATCH 3/3] Check location permission for ConnDiags last. This CL updates ConnectivityService to check location permissions for ConnectivityDiagnostics callbacks last in the permission check process. This minimizes misattribution of location access for networks that an app is not administering. This CL also updates ConnectivityDiagnosticsManager documentation to clearly state that location permissions are required in order to receive callbacks. Bug: 187310575 Test: atest ConnectivityDiagnosticsManagerTest Test: atest ConnectivityServiceTest Change-Id: I2dbeddac6273e2392ccaeae51a1c7776d6d3da75 Merged-In: I2dbeddac6273e2392ccaeae51a1c7776d6d3da75 (cherry picked from commit 23e37e6c76204fea5a000657ac00821b15ebca45) --- .../net/ConnectivityDiagnosticsManager.java | 4 +- .../android/server/ConnectivityService.java | 53 ++++++++++++------- .../server/ConnectivityServiceTest.java | 16 +++--- 3 files changed, 46 insertions(+), 27 deletions(-) diff --git a/framework/src/android/net/ConnectivityDiagnosticsManager.java b/framework/src/android/net/ConnectivityDiagnosticsManager.java index 3598ebc701..dcc8a5eacd 100644 --- a/framework/src/android/net/ConnectivityDiagnosticsManager.java +++ b/framework/src/android/net/ConnectivityDiagnosticsManager.java @@ -713,7 +713,9 @@ public class ConnectivityDiagnosticsManager { *

Callbacks registered by apps not meeting the above criteria will not be invoked. * *

If a registering app loses its relevant permissions, any callbacks it registered will - * silently stop receiving callbacks. + * silently stop receiving callbacks. Note that registering apps must also have location + * permissions to receive callbacks as some Networks may be location-bound (such as WiFi + * networks). * *

Each register() call MUST use a ConnectivityDiagnosticsCallback instance that is * not currently registered. If a ConnectivityDiagnosticsCallback instance is registered with diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 6027a99683..061b5bd5f6 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -9158,6 +9158,34 @@ public class ConnectivityService extends IConnectivityManager.Stub return results; } + private boolean hasLocationPermission(String packageName, int uid) { + // LocationPermissionChecker#checkLocationPermission can throw SecurityException if the uid + // and package name don't match. Throwing on the CS thread is not acceptable, so wrap the + // call in a try-catch. + try { + if (!mLocationPermissionChecker.checkLocationPermission( + packageName, null /* featureId */, uid, null /* message */)) { + return false; + } + } catch (SecurityException e) { + return false; + } + + return true; + } + + private boolean ownsVpnRunningOverNetwork(int uid, Network network) { + for (NetworkAgentInfo virtual : mNetworkAgentInfos) { + if (virtual.supportsUnderlyingNetworks() + && virtual.networkCapabilities.getOwnerUid() == uid + && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, network)) { + return true; + } + } + + return false; + } + @VisibleForTesting boolean checkConnectivityDiagnosticsPermissions( int callbackPid, int callbackUid, NetworkAgentInfo nai, String callbackPackageName) { @@ -9165,29 +9193,14 @@ public class ConnectivityService extends IConnectivityManager.Stub return true; } - // LocationPermissionChecker#checkLocationPermission can throw SecurityException if the uid - // and package name don't match. Throwing on the CS thread is not acceptable, so wrap the - // call in a try-catch. - try { - if (!mLocationPermissionChecker.checkLocationPermission( - callbackPackageName, null /* featureId */, callbackUid, null /* message */)) { - return false; - } - } catch (SecurityException e) { + // Administrator UIDs also contains the Owner UID + final int[] administratorUids = nai.networkCapabilities.getAdministratorUids(); + if (!CollectionUtils.contains(administratorUids, callbackUid) + && !ownsVpnRunningOverNetwork(callbackUid, nai.network)) { return false; } - for (NetworkAgentInfo virtual : mNetworkAgentInfos) { - if (virtual.supportsUnderlyingNetworks() - && virtual.networkCapabilities.getOwnerUid() == callbackUid - && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, nai.network)) { - return true; - } - } - - // Administrator UIDs also contains the Owner UID - final int[] administratorUids = nai.networkCapabilities.getAdministratorUids(); - return CollectionUtils.contains(administratorUids, callbackUid); + return hasLocationPermission(callbackPackageName, callbackUid); } @Override diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 29a411e2c9..466138550a 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -9940,28 +9940,32 @@ public class ConnectivityServiceTest { @Test public void testCheckConnectivityDiagnosticsPermissionsWrongUidPackageName() throws Exception { - final NetworkAgentInfo naiWithoutUid = fakeMobileNai(new NetworkCapabilities()); + final int wrongUid = Process.myUid() + 1; + + final NetworkCapabilities nc = new NetworkCapabilities(); + nc.setAdministratorUids(new int[] {wrongUid}); + final NetworkAgentInfo naiWithUid = fakeMobileNai(nc); mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED); assertFalse( "Mismatched uid/package name should not pass the location permission check", mService.checkConnectivityDiagnosticsPermissions( - Process.myPid() + 1, Process.myUid() + 1, naiWithoutUid, - mContext.getOpPackageName())); + Process.myPid() + 1, wrongUid, naiWithUid, mContext.getOpPackageName())); } @Test public void testCheckConnectivityDiagnosticsPermissionsNoLocationPermission() throws Exception { - final NetworkAgentInfo naiWithoutUid = fakeMobileNai(new NetworkCapabilities()); + final NetworkCapabilities nc = new NetworkCapabilities(); + nc.setAdministratorUids(new int[] {Process.myUid()}); + final NetworkAgentInfo naiWithUid = fakeMobileNai(nc); mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED); assertFalse( "ACCESS_FINE_LOCATION permission necessary for Connectivity Diagnostics", mService.checkConnectivityDiagnosticsPermissions( - Process.myPid(), Process.myUid(), naiWithoutUid, - mContext.getOpPackageName())); + Process.myPid(), Process.myUid(), naiWithUid, mContext.getOpPackageName())); } @Test