Merge "Check location permission for ConnDiags last." into sc-dev

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 29a485680667..5c47f27ef58f 100644
--- a/packages/Connectivity/service/src/com/android/server/ConnectivityService.java
+++ b/packages/Connectivity/service/src/com/android/server/ConnectivityService.java
@@ -9162,36 +9162,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