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 09f0f7dff0ac65cfe8277c2f6377ee7bab1a9a13)

3 files changed, 37 insertions, 18 deletions

diff --git a/packages/Connectivity/framework/src/android/net/ConnectivityDiagnosticsManager.java b/packages/Connectivity/framework/src/android/net/ConnectivityDiagnosticsManager.java
index 3598ebc70118..dcc8a5eacd13 100644
--- a/packages/Connectivity/framework/src/android/net/ConnectivityDiagnosticsManager.java
+++ b/packages/Connectivity/framework/src/android/net/ConnectivityDiagnosticsManager.java
@@ -713,7 +713,9 @@ public class ConnectivityDiagnosticsManager {
      * <p>Callbacks registered by apps not meeting the above criteria will not be invoked.
      *
      * <p>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).
      *
      * <p>Each register() call <b>MUST</b> use a ConnectivityDiagnosticsCallback instance that is
      * not currently registered. If a ConnectivityDiagnosticsCallback instance is registered with
diff --git a/packages/Connectivity/service/src/com/android/server/ConnectivityService.java b/packages/Connectivity/service/src/com/android/server/ConnectivityService.java
index 6027a996838f..061b5bd5f6bd 100644
--- a/packages/Connectivity/service/src/com/android/server/ConnectivityService.java
+++ b/packages/Connectivity/service/src/com/android/server/ConnectivityService.java
@@ -9158,36 +9158,49 @@ public class ConnectivityService extends IConnectivityManager.Stub
         return results;
     }
 
-    @VisibleForTesting
-    boolean checkConnectivityDiagnosticsPermissions(
-            int callbackPid, int callbackUid, NetworkAgentInfo nai, String callbackPackageName) {
-        if (checkNetworkStackPermission(callbackPid, callbackUid)) {
-            return true;
-        }
-
+    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(
-                    callbackPackageName, null /* featureId */, callbackUid, null /* message */)) {
+                        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() == callbackUid
-                    && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, nai.network)) {
+                    && virtual.networkCapabilities.getOwnerUid() == uid
+                    && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, network)) {
                 return true;
             }
         }
 
+        return false;
+    }
+
+    @VisibleForTesting
+    boolean checkConnectivityDiagnosticsPermissions(
+            int callbackPid, int callbackUid, NetworkAgentInfo nai, String callbackPackageName) {
+        if (checkNetworkStackPermission(callbackPid, callbackUid)) {
+            return true;
+        }
+
         // Administrator UIDs also contains the Owner UID
         final int[] administratorUids = nai.networkCapabilities.getAdministratorUids();
-        return CollectionUtils.contains(administratorUids, callbackUid);
+        if (!CollectionUtils.contains(administratorUids, callbackUid)
+                && !ownsVpnRunningOverNetwork(callbackUid, nai.network)) {
+            return false;
+        }
+
+        return hasLocationPermission(callbackPackageName, callbackUid);
     }
 
     @Override
diff --git a/packages/Connectivity/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/packages/Connectivity/tests/unit/java/com/android/server/ConnectivityServiceTest.java
index 29a411e2c935..466138550aae 100644
--- a/packages/Connectivity/tests/unit/java/com/android/server/ConnectivityServiceTest.java
+++ b/packages/Connectivity/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