From a73199168b4fb3fbbd65c1f5f252f3b66c5c2fb0 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Wed, 6 Nov 2019 00:20:15 -0800 Subject: [PATCH] Add separate user consent for Platform VPNs This change adds a new VPN user consent flow (using the same text) for granting the lesser OP_ACTIVATE_PLATFORM_VPN. A new PlatformVpnConfirmDialog is created as a subclass to preserve all logic, but ensure the right appop is granted for the relevant dialog. Intent extras were considered, but are inherently unsafe, since the caller may add any extras that they would want. Bug: 144246835 Test: FrameworksNetTests passing Change-Id: Ia6f36207d43c3748f938430c2780dcf29e5623f3 Merged-In: Ia6f36207d43c3748f938430c2780dcf29e5623f3 --- .../android/net/IConnectivityManager.aidl | 2 +- .../android/server/ConnectivityService.java | 27 ++++++---- .../net/java/android/net/VpnManagerTest.java | 11 +++- .../android/server/connectivity/VpnTest.java | 50 ++++++++++++++++++- 4 files changed, 75 insertions(+), 15 deletions(-) diff --git a/core/java/android/net/IConnectivityManager.aidl b/core/java/android/net/IConnectivityManager.aidl index b050e479d2..06b18a6467 100644 --- a/core/java/android/net/IConnectivityManager.aidl +++ b/core/java/android/net/IConnectivityManager.aidl @@ -116,7 +116,7 @@ interface IConnectivityManager boolean prepareVpn(String oldPackage, String newPackage, int userId); - void setVpnPackageAuthorization(String packageName, int userId, boolean authorized); + void setVpnPackageAuthorization(String packageName, int userId, int vpnType); ParcelFileDescriptor establishVpn(in VpnConfig config); diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index caf185145f..44932fc01e 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -112,6 +112,7 @@ import android.net.SocketKeepalive; import android.net.TetheringManager; import android.net.UidRange; import android.net.Uri; +import android.net.VpnManager; import android.net.VpnService; import android.net.metrics.IpConnectivityLog; import android.net.metrics.NetworkEvent; @@ -4293,7 +4294,7 @@ public class ConnectivityService extends IConnectivityManager.Stub throwIfLockdownEnabled(); Vpn vpn = mVpns.get(userId); if (vpn != null) { - return vpn.prepare(oldPackage, newPackage, false); + return vpn.prepare(oldPackage, newPackage, VpnManager.TYPE_VPN_SERVICE); } else { return false; } @@ -4301,26 +4302,29 @@ public class ConnectivityService extends IConnectivityManager.Stub } /** - * Set whether the VPN package has the ability to launch VPNs without user intervention. - * This method is used by system-privileged apps. - * VPN permissions are checked in the {@link Vpn} class. If the caller is not {@code userId}, - * {@link android.Manifest.permission.INTERACT_ACROSS_USERS_FULL} permission is required. + * Set whether the VPN package has the ability to launch VPNs without user intervention. This + * method is used by system-privileged apps. VPN permissions are checked in the {@link Vpn} + * class. If the caller is not {@code userId}, {@link + * android.Manifest.permission.INTERACT_ACROSS_USERS_FULL} permission is required. * * @param packageName The package for which authorization state should change. * @param userId User for whom {@code packageName} is installed. * @param authorized {@code true} if this app should be able to start a VPN connection without - * explicit user approval, {@code false} if not. - * + * explicit user approval, {@code false} if not. + * @param vpnType The {@link VpnManager.VpnType} constant representing what class of VPN + * permissions should be granted. When unauthorizing an app, {@link + * VpnManager.TYPE_VPN_NONE} should be used. * @hide */ @Override - public void setVpnPackageAuthorization(String packageName, int userId, boolean authorized) { + public void setVpnPackageAuthorization( + String packageName, int userId, @VpnManager.VpnType int vpnType) { enforceCrossUserPermission(userId); synchronized (mVpns) { Vpn vpn = mVpns.get(userId); if (vpn != null) { - vpn.setPackageAuthorization(packageName, authorized); + vpn.setPackageAuthorization(packageName, vpnType); } } } @@ -7193,7 +7197,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final String alwaysOnPackage = getAlwaysOnVpnPackage(userId); if (alwaysOnPackage != null) { setAlwaysOnVpnPackage(userId, null, false, null); - setVpnPackageAuthorization(alwaysOnPackage, userId, false); + setVpnPackageAuthorization(alwaysOnPackage, userId, VpnManager.TYPE_VPN_NONE); } // Turn Always-on VPN off @@ -7216,7 +7220,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } else { // Prevent this app (packagename = vpnConfig.user) from initiating // VPN connections in the future without user intervention. - setVpnPackageAuthorization(vpnConfig.user, userId, false); + setVpnPackageAuthorization( + vpnConfig.user, userId, VpnManager.TYPE_VPN_NONE); prepareVpn(null, VpnConfig.LEGACY_VPN, userId); } diff --git a/tests/net/java/android/net/VpnManagerTest.java b/tests/net/java/android/net/VpnManagerTest.java index 97551c94e2..95a794235a 100644 --- a/tests/net/java/android/net/VpnManagerTest.java +++ b/tests/net/java/android/net/VpnManagerTest.java @@ -16,6 +16,7 @@ package android.net; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.mockito.Matchers.any; @@ -24,6 +25,8 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import android.content.ComponentName; +import android.content.Intent; import android.test.mock.MockContext; import androidx.test.filters.SmallTest; @@ -78,7 +81,13 @@ public class VpnManagerTest { when(mMockCs.provisionVpnProfile(any(VpnProfile.class), eq(PKG_NAME))).thenReturn(false); // Expect intent to be returned, as consent has not already been granted. - assertNotNull(mVpnManager.provisionVpnProfile(profile)); + final Intent intent = mVpnManager.provisionVpnProfile(profile); + assertNotNull(intent); + + final ComponentName expectedComponentName = + ComponentName.unflattenFromString( + "com.android.vpndialogs/com.android.vpndialogs.PlatformVpnConfirmDialog"); + assertEquals(expectedComponentName, intent.getComponent()); verify(mMockCs).provisionVpnProfile(eq(profile.toVpnProfile()), eq(PKG_NAME)); } diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index 084ec73302..155c61f3f8 100644 --- a/tests/net/java/com/android/server/connectivity/VpnTest.java +++ b/tests/net/java/com/android/server/connectivity/VpnTest.java @@ -63,6 +63,7 @@ import android.net.Network; import android.net.NetworkCapabilities; import android.net.NetworkInfo.DetailedState; import android.net.UidRange; +import android.net.VpnManager; import android.net.VpnService; import android.os.Build.VERSION_CODES; import android.os.Bundle; @@ -471,12 +472,12 @@ public class VpnTest { order.verify(mNetService).setAllowOnlyVpnForUids(eq(true), aryEq(entireUser)); // When a new VPN package is set the rules should change to cover that package. - vpn.prepare(null, PKGS[0], false /* isPlatformVpn */); + vpn.prepare(null, PKGS[0], VpnManager.TYPE_VPN_SERVICE); order.verify(mNetService).setAllowOnlyVpnForUids(eq(false), aryEq(entireUser)); order.verify(mNetService).setAllowOnlyVpnForUids(eq(true), aryEq(exceptPkg0)); // When that VPN package is unset, everything should be undone again in reverse. - vpn.prepare(null, VpnConfig.LEGACY_VPN, false /* isPlatformVpn */); + vpn.prepare(null, VpnConfig.LEGACY_VPN, VpnManager.TYPE_VPN_SERVICE); order.verify(mNetService).setAllowOnlyVpnForUids(eq(false), aryEq(exceptPkg0)); order.verify(mNetService).setAllowOnlyVpnForUids(eq(true), aryEq(entireUser)); } @@ -817,6 +818,51 @@ public class VpnTest { eq(TEST_VPN_PKG)); } + @Test + public void testSetPackageAuthorizationVpnService() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(); + + assertTrue(vpn.setPackageAuthorization(TEST_VPN_PKG, VpnManager.TYPE_VPN_SERVICE)); + verify(mAppOps) + .setMode( + eq(AppOpsManager.OP_ACTIVATE_VPN), + eq(Process.myUid()), + eq(TEST_VPN_PKG), + eq(AppOpsManager.MODE_ALLOWED)); + } + + @Test + public void testSetPackageAuthorizationPlatformVpn() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(); + + assertTrue(vpn.setPackageAuthorization(TEST_VPN_PKG, VpnManager.TYPE_VPN_PLATFORM)); + verify(mAppOps) + .setMode( + eq(AppOpsManager.OP_ACTIVATE_PLATFORM_VPN), + eq(Process.myUid()), + eq(TEST_VPN_PKG), + eq(AppOpsManager.MODE_ALLOWED)); + } + + @Test + public void testSetPackageAuthorizationRevokeAuthorization() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(); + + assertTrue(vpn.setPackageAuthorization(TEST_VPN_PKG, VpnManager.TYPE_VPN_NONE)); + verify(mAppOps) + .setMode( + eq(AppOpsManager.OP_ACTIVATE_VPN), + eq(Process.myUid()), + eq(TEST_VPN_PKG), + eq(AppOpsManager.MODE_IGNORED)); + verify(mAppOps) + .setMode( + eq(AppOpsManager.OP_ACTIVATE_PLATFORM_VPN), + eq(Process.myUid()), + eq(TEST_VPN_PKG), + eq(AppOpsManager.MODE_IGNORED)); + } + /** * Mock some methods of vpn object. */