From 083b00b5fb228c358dfde93ba639aff748c9a073 Mon Sep 17 00:00:00 2001
From: Lorenzo Colitti <lorenzo@google.com>
Date: Mon, 16 Nov 2020 23:41:21 +0900
Subject: [PATCH] Stop accessing VPNs in
 checkConnectivityDiagnosticsPermissions.

Currently, checkConnectivityDiagnosticsPermissions takes the VPN
lock to examine the VPN's underlying networks. Use the underlying
network data that is available in ConnectivityService instead.

Bug: 173331190
Test: passes existing tests in ConnectivityServiceTest
Change-Id: Ia1c366c5e9974d4d2c4b38030e66c007d62020ff
---
 .../com/android/server/ConnectivityService.java     | 13 ++++++-------
 .../com/android/server/ConnectivityServiceTest.java | 13 ++++---------
 2 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 5420ee2f11..3361322908 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -8276,13 +8276,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
             return false;
         }
 
-        final Network[] underlyingNetworks;
-        synchronized (mVpns) {
-            final Vpn vpn = getVpnIfOwner(callbackUid);
-            underlyingNetworks = (vpn == null) ? null : vpn.getUnderlyingNetworks();
-        }
-        if (underlyingNetworks != null) {
-            if (Arrays.asList(underlyingNetworks).contains(nai.network)) return true;
+        for (NetworkAgentInfo virtual : mNetworkAgentInfos.values()) {
+            if (virtual.supportsUnderlyingNetworks()
+                    && virtual.networkCapabilities.getOwnerUid() == callbackUid
+                    && ArrayUtils.contains(virtual.declaredUnderlyingNetworks, nai.network)) {
+                return true;
+            }
         }
 
         // Administrator UIDs also contains the Owner UID
diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java
index 561c6bab9c..5037553d8a 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -7438,20 +7438,14 @@ public class ConnectivityServiceTest {
         setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION,
                 Manifest.permission.ACCESS_FINE_LOCATION);
 
-        // setUp() calls mockVpn() which adds a VPN with the Test Runner's uid. Configure it to be
-        // active
-        final VpnInfo info = new VpnInfo();
-        info.ownerUid = Process.myUid();
-        info.vpnIface = VPN_IFNAME;
-        mMockVpn.setVpnInfo(info);
-
         mMockVpn.establishForMyUid();
-        waitForIdle();
 
+        // Wait for networks to connect and broadcasts to be sent before removing permissions.
+        waitForIdle();
         mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED);
 
-
         assertTrue(mService.setUnderlyingNetworksForVpn(new Network[] {network}));
+        waitForIdle();
         assertTrue(
                 "Active VPN permission not applied",
                 mService.checkConnectivityDiagnosticsPermissions(
@@ -7459,6 +7453,7 @@ public class ConnectivityServiceTest {
                         mContext.getOpPackageName()));
 
         assertTrue(mService.setUnderlyingNetworksForVpn(null));
+        waitForIdle();
         assertFalse(
                 "VPN shouldn't receive callback on non-underlying network",
                 mService.checkConnectivityDiagnosticsPermissions(