From f3072fcd46112bad7c5f6ddd4cc35d2c67f00d11 Mon Sep 17 00:00:00 2001
From: lucaslin <lucaslin@google.com>
Date: Thu, 29 Jul 2021 13:26:35 +0800
Subject: [PATCH] Make sure that only the owner can call stopVpnProfile()

In stopVpnProfile(), it doesn't check if the caller's package
name is the same as the given one, so any app has chance to stop
the VPN profile of other apps.

Bug: 191382886
Test: atest FrameworksNetTests CtsNetTestCases \
      CtsHostsideNetworkTests:HostsideVpnTests
Change-Id: Ib0a6e9ed191ff8c8bd55ce9902d894b6a339ace2
Merged-In: I254ffd1c08ec058d594b4ea55cbae5505f8497cc
---
 .../android/server/ConnectivityService.java   | 28 +++++++++++++++++--
 .../server/ConnectivityServiceTest.java       | 18 ++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index fcf73ac7ce15a..0eab71a3763dd 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -73,6 +73,7 @@ import android.content.Context;
 import android.content.Intent;
 import android.content.IntentFilter;
 import android.content.pm.PackageManager;
+import android.content.pm.PackageManager.NameNotFoundException;
 import android.content.res.Configuration;
 import android.database.ContentObserver;
 import android.net.CaptivePortal;
@@ -4607,6 +4608,25 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
     }
 
+    private int getAppUid(final String app, final int userId) {
+        final PackageManager pm = mContext.getPackageManager();
+        final long token = Binder.clearCallingIdentity();
+        try {
+            return pm.getPackageUidAsUser(app, userId);
+        } catch (NameNotFoundException e) {
+            return -1;
+        } finally {
+            Binder.restoreCallingIdentity(token);
+        }
+    }
+
+    private void verifyCallingUidAndPackage(String packageName, int callingUid) {
+        final int userId = UserHandle.getUserId(callingUid);
+        if (getAppUid(packageName, userId) != callingUid) {
+            throw new SecurityException(packageName + " does not belong to uid " + callingUid);
+        }
+    }
+
     /**
      * Starts the VPN based on the stored profile for the given package
      *
@@ -4618,7 +4638,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
      */
     @Override
     public void startVpnProfile(@NonNull String packageName) {
-        final int user = UserHandle.getUserId(Binder.getCallingUid());
+        final int callingUid = Binder.getCallingUid();
+        verifyCallingUidAndPackage(packageName, callingUid);
+        final int user = UserHandle.getUserId(callingUid);
         synchronized (mVpns) {
             throwIfLockdownEnabled();
             mVpns.get(user).startVpnProfile(packageName, mKeyStore);
@@ -4635,7 +4657,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
      */
     @Override
     public void stopVpnProfile(@NonNull String packageName) {
-        final int user = UserHandle.getUserId(Binder.getCallingUid());
+        final int callingUid = Binder.getCallingUid();
+        verifyCallingUidAndPackage(packageName, callingUid);
+        final int user = UserHandle.getUserId(callingUid);
         synchronized (mVpns) {
             mVpns.get(user).stopVpnProfile(packageName);
         }
diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java
index a5554c740e7f7..e9301d1dea9d5 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -1224,6 +1224,9 @@ public class ConnectivityServiceTest {
                 Arrays.asList(new UserInfo[] {
                         new UserInfo(VPN_USER, "", 0),
                 }));
+        final int userId = UserHandle.getCallingUserId();
+        final UserInfo primaryUser = new UserInfo(userId, "", UserInfo.FLAG_PRIMARY);
+        doReturn(primaryUser).when(mUserManager).getUserInfo(eq(userId));
         final ApplicationInfo applicationInfo = new ApplicationInfo();
         applicationInfo.targetSdkVersion = Build.VERSION_CODES.Q;
         when(mPackageManager.getApplicationInfoAsUser(anyString(), anyInt(), any()))
@@ -1368,6 +1371,9 @@ public class ConnectivityServiceTest {
                         buildPackageInfo(/* SYSTEM */ false, APP2_UID),
                         buildPackageInfo(/* SYSTEM */ false, VPN_UID)
                 }));
+        final int userId = UserHandle.getCallingUserId();
+        when(mPackageManager.getPackageUidAsUser(TEST_PACKAGE_NAME, userId))
+                .thenReturn(Process.myUid());
     }
 
     private void verifyActiveNetwork(int transport) {
@@ -7068,6 +7074,18 @@ public class ConnectivityServiceTest {
         assertContainsExactly(uidCaptor.getValue(), APP1_UID, APP2_UID);
     }
 
+    @Test
+    public void testStartVpnProfileFromDiffPackage() throws Exception {
+        final String notMyVpnPkg = "com.not.my.vpn";
+        assertThrows(SecurityException.class, () -> mService.startVpnProfile(notMyVpnPkg));
+    }
+
+    @Test
+    public void testStopVpnProfileFromDiffPackage() throws Exception {
+        final String notMyVpnPkg = "com.not.my.vpn";
+        assertThrows(SecurityException.class, () -> mService.stopVpnProfile(notMyVpnPkg));
+    }
+
     @Test
     public void testUidUpdateChangesInterfaceFilteringRule() throws Exception {
         LinkProperties lp = new LinkProperties();