From 0b4be0203d35f52f97500d1882915218cbcb5ce7 Mon Sep 17 00:00:00 2001 From: Cody Kesting Date: Thu, 20 May 2021 22:57:07 +0000 Subject: [PATCH] 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 f3d0fc49dbb5f82433a37b978618ac370841df1c) --- .../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