From ed113d5436882f8570ca7ba4bf264ef773c416ca Mon Sep 17 00:00:00 2001
From: Cody Kesting <ckesting@google.com>
Date: Tue, 11 Feb 2020 14:16:41 -0800
Subject: [PATCH] Restrict VPN Diagnostics callbacks to underlying networks.

ConnectivityDiagnosticsCallbacks should only be invoked for the
underlying networks declared by active VPNs. This encourages VPN apps to
declare their underlying networks.

The previous permission model for VPNs allowed active VPNs to receive
callbacks on any network.

Bug: 148903617
Test: atest FrameworksNetTests
Change-Id: Ic08cdd2e2532580fda0fd3034e2bdff27e0ff84b
---
 .../android/server/ConnectivityService.java   |  9 ++++--
 .../server/ConnectivityServiceTest.java       | 28 +++++++++++++++++--
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 5f0a53cb23725..f060368db4b94 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -7890,10 +7890,13 @@ public class ConnectivityService extends IConnectivityManager.Stub
             return false;
         }
 
+        final Network[] underlyingNetworks;
         synchronized (mVpns) {
-            if (getVpnIfOwner(callbackUid) != null) {
-                return true;
-            }
+            final Vpn vpn = getVpnIfOwner(callbackUid);
+            underlyingNetworks = (vpn == null) ? null : vpn.getUnderlyingNetworks();
+        }
+        if (underlyingNetworks != null) {
+            if (Arrays.asList(underlyingNetworks).contains(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 6985415a6a37b..5149d410ae1ab 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -307,6 +307,8 @@ public class ConnectivityServiceTest {
 
     private static final long TIMESTAMP = 1234L;
 
+    private static final int NET_ID = 110;
+
     private static final String CLAT_PREFIX = "v4-";
     private static final String MOBILE_IFNAME = "test_rmnet_data0";
     private static final String WIFI_IFNAME = "test_wlan0";
@@ -1016,6 +1018,7 @@ public class ConnectivityServiceTest {
         private int mVpnType = VpnManager.TYPE_VPN_SERVICE;
 
         private VpnInfo mVpnInfo;
+        private Network[] mUnderlyingNetworks;
 
         public MockVpn(int userId) {
             super(startHandlerThreadAndReturnLooper(), mServiceContext, mNetworkManagementService,
@@ -1105,9 +1108,21 @@ public class ConnectivityServiceTest {
             return super.getVpnInfo();
         }
 
-        private void setVpnInfo(VpnInfo vpnInfo) {
+        private synchronized void setVpnInfo(VpnInfo vpnInfo) {
             mVpnInfo = vpnInfo;
         }
+
+        @Override
+        public synchronized Network[] getUnderlyingNetworks() {
+            if (mUnderlyingNetworks != null) return mUnderlyingNetworks;
+
+            return super.getUnderlyingNetworks();
+        }
+
+        /** Don't override behavior for {@link Vpn#setUnderlyingNetworks}. */
+        private synchronized void overrideUnderlyingNetworks(Network[] underlyingNetworks) {
+            mUnderlyingNetworks = underlyingNetworks;
+        }
     }
 
     private void mockVpn(int uid) {
@@ -6774,9 +6789,10 @@ public class ConnectivityServiceTest {
 
     @Test
     public void testCheckConnectivityDiagnosticsPermissionsActiveVpn() throws Exception {
+        final Network network = new Network(NET_ID);
         final NetworkAgentInfo naiWithoutUid =
                 new NetworkAgentInfo(
-                        null, null, null, null, null, new NetworkCapabilities(), 0,
+                        null, null, network, null, null, new NetworkCapabilities(), 0,
                         mServiceContext, null, null, mService, null, null, null, 0);
 
         setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION,
@@ -6789,11 +6805,19 @@ public class ConnectivityServiceTest {
         info.ownerUid = Process.myUid();
         info.vpnIface = "interface";
         mMockVpn.setVpnInfo(info);
+        mMockVpn.overrideUnderlyingNetworks(new Network[] {network});
         assertTrue(
                 "Active VPN permission not applied",
                 mService.checkConnectivityDiagnosticsPermissions(
                         Process.myPid(), Process.myUid(), naiWithoutUid,
                         mContext.getOpPackageName()));
+
+        mMockVpn.overrideUnderlyingNetworks(null);
+        assertFalse(
+                "VPN shouldn't receive callback on non-underlying network",
+                mService.checkConnectivityDiagnosticsPermissions(
+                        Process.myPid(), Process.myUid(), naiWithoutUid,
+                        mContext.getOpPackageName()));
     }
 
     @Test