From 5d50ce8181a947ffbdd0322d20e2d440a65989f6 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Mon, 20 Jan 2020 22:14:59 -0800 Subject: [PATCH 1/2] Enforce restricted user, getConnectionOwnerUid checks This CL adds checks to ensure restricted users cannot change or start/stop platform VPNs. In addition, this also adds checks to the ConnectivityManager#getConnectionOwnerUid() to ensure that only VpnService based VPNs can identify connections Bug: 148040659 Test: FrameworksNetTests run Change-Id: Id47ada5766036bfc84f3ba47f66f2d2683af916d --- .../java/android/net/ConnectivityManager.java | 18 +-- .../android/server/ConnectivityService.java | 7 ++ .../com/android/server/connectivity/Vpn.java | 42 ++++++- .../server/ConnectivityServiceTest.java | 119 ++++++++++++++++-- .../android/server/connectivity/VpnTest.java | 60 ++++++++- 5 files changed, 223 insertions(+), 23 deletions(-) diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index cb3140487f35a..5405ad37ffda5 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/core/java/android/net/ConnectivityManager.java @@ -4716,19 +4716,19 @@ public class ConnectivityManager { /** * Returns the {@code uid} of the owner of a network connection. * - * @param protocol The protocol of the connection. Only {@code IPPROTO_TCP} and - * {@code IPPROTO_UDP} currently supported. + * @param protocol The protocol of the connection. Only {@code IPPROTO_TCP} and {@code + * IPPROTO_UDP} currently supported. * @param local The local {@link InetSocketAddress} of a connection. * @param remote The remote {@link InetSocketAddress} of a connection. - * * @return {@code uid} if the connection is found and the app has permission to observe it - * (e.g., if it is associated with the calling VPN app's tunnel) or - * {@link android.os.Process#INVALID_UID} if the connection is not found. - * Throws {@link SecurityException} if the caller is not the active VPN for the current user. - * Throws {@link IllegalArgumentException} if an unsupported protocol is requested. + * (e.g., if it is associated with the calling VPN app's VpnService tunnel) or {@link + * android.os.Process#INVALID_UID} if the connection is not found. + * @throws {@link SecurityException} if the caller is not the active VpnService for the current + * user. + * @throws {@link IllegalArgumentException} if an unsupported protocol is requested. */ - public int getConnectionOwnerUid(int protocol, @NonNull InetSocketAddress local, - @NonNull InetSocketAddress remote) { + public int getConnectionOwnerUid( + int protocol, @NonNull InetSocketAddress local, @NonNull InetSocketAddress remote) { ConnectionInfo connectionInfo = new ConnectionInfo(protocol, local, remote); try { return mService.getConnectionOwnerUid(connectionInfo); diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 1b43fc0fa10d2..6351f3499ea33 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -7486,6 +7486,13 @@ public class ConnectivityService extends IConnectivityManager.Stub */ public int getConnectionOwnerUid(ConnectionInfo connectionInfo) { final Vpn vpn = enforceActiveVpnOrNetworkStackPermission(); + + // Only VpnService based VPNs should be able to get this information. + if (vpn != null && vpn.getActiveAppVpnType() != VpnManager.TYPE_VPN_SERVICE) { + throw new SecurityException( + "getConnectionOwnerUid() not allowed for non-VpnService VPNs"); + } + if (connectionInfo.protocol != IPPROTO_TCP && connectionInfo.protocol != IPPROTO_UDP) { throw new IllegalArgumentException("Unsupported protocol " + connectionInfo.protocol); } diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 7f6dc55a369d1..3e831d187edc7 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -1104,7 +1104,6 @@ public class Vpn { */ public synchronized ParcelFileDescriptor establish(VpnConfig config) { // Check if the caller is already prepared. - UserManager mgr = UserManager.get(mContext); if (Binder.getCallingUid() != mOwnerUID) { return null; } @@ -1118,10 +1117,7 @@ public class Vpn { long token = Binder.clearCallingIdentity(); try { // Restricted users are not allowed to create VPNs, they are tied to Owner - UserInfo user = mgr.getUserInfo(mUserHandle); - if (user.isRestricted()) { - throw new SecurityException("Restricted users cannot establish VPNs"); - } + enforceNotRestrictedUser(); ResolveInfo info = AppGlobals.getPackageManager().resolveService(intent, null, 0, mUserHandle); @@ -1672,6 +1668,25 @@ public class Vpn { return mNetworkCapabilities.appliesToUid(uid); } + /** + * Gets the currently running App-based VPN type + * + * @return the {@link VpnManager.VpnType}. {@link VpnManager.TYPE_VPN_NONE} if not running an + * app-based VPN. While VpnService-based VPNs are always app VPNs and LegacyVpn is always + * Settings-based, the Platform VPNs can be initiated by both apps and Settings. + */ + public synchronized int getActiveAppVpnType() { + if (VpnConfig.LEGACY_VPN.equals(mPackage)) { + return VpnManager.TYPE_VPN_NONE; + } + + if (mVpnRunner != null && mVpnRunner instanceof IkeV2VpnRunner) { + return VpnManager.TYPE_VPN_PLATFORM; + } else { + return VpnManager.TYPE_VPN_SERVICE; + } + } + /** * @param uid The target uid. * @@ -1800,6 +1815,17 @@ public class Vpn { throw new IllegalStateException("Unable to find IPv4 default gateway"); } + private void enforceNotRestrictedUser() { + Binder.withCleanCallingIdentity(() -> { + final UserManager mgr = UserManager.get(mContext); + final UserInfo user = mgr.getUserInfo(mUserHandle); + + if (user.isRestricted()) { + throw new SecurityException("Restricted users cannot configure VPNs"); + } + }); + } + /** * Start legacy VPN, controlling native daemons as needed. Creates a * secondary thread to perform connection work, returning quickly. @@ -2757,6 +2783,7 @@ public class Vpn { checkNotNull(keyStore, "KeyStore missing"); verifyCallingUidAndPackage(packageName); + enforceNotRestrictedUser(); final byte[] encodedProfile = profile.encode(); if (encodedProfile.length > MAX_VPN_PROFILE_SIZE_BYTES) { @@ -2792,6 +2819,7 @@ public class Vpn { checkNotNull(keyStore, "KeyStore missing"); verifyCallingUidAndPackage(packageName); + enforceNotRestrictedUser(); Binder.withCleanCallingIdentity( () -> { @@ -2834,6 +2862,8 @@ public class Vpn { checkNotNull(packageName, "No package name provided"); checkNotNull(keyStore, "KeyStore missing"); + enforceNotRestrictedUser(); + // Prepare VPN for startup if (!prepare(packageName, null /* newPackage */, VpnManager.TYPE_VPN_PLATFORM)) { throw new SecurityException("User consent not granted for package " + packageName); @@ -2899,6 +2929,8 @@ public class Vpn { public synchronized void stopVpnProfile(@NonNull String packageName) { checkNotNull(packageName, "No package name provided"); + enforceNotRestrictedUser(); + // To stop the VPN profile, the caller must be the current prepared package and must be // running an Ikev2VpnProfile. if (!isCurrentPreparedPackage(packageName) && mVpnRunner instanceof IkeV2VpnRunner) { diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index f40e57fe46aa8..e2336512a104e 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -78,6 +78,7 @@ import static android.net.NetworkPolicyManager.RULE_NONE; import static android.net.NetworkPolicyManager.RULE_REJECT_ALL; import static android.net.NetworkPolicyManager.RULE_REJECT_METERED; import static android.net.RouteInfo.RTN_UNREACHABLE; +import static android.system.OsConstants.IPPROTO_TCP; import static com.android.server.ConnectivityServiceTestUtilsKt.transportToLegacyType; import static com.android.testutils.ConcurrentUtilsKt.await; @@ -138,6 +139,7 @@ import android.content.pm.UserInfo; import android.content.res.Resources; import android.location.LocationManager; import android.net.CaptivePortalData; +import android.net.ConnectionInfo; import android.net.ConnectivityManager; import android.net.ConnectivityManager.NetworkCallback; import android.net.ConnectivityManager.PacketKeepalive; @@ -153,6 +155,7 @@ import android.net.INetworkMonitorCallbacks; import android.net.INetworkPolicyListener; import android.net.INetworkPolicyManager; import android.net.INetworkStatsService; +import android.net.InetAddresses; import android.net.InterfaceConfiguration; import android.net.IpPrefix; import android.net.IpSecManager; @@ -176,6 +179,7 @@ import android.net.RouteInfo; import android.net.SocketKeepalive; import android.net.UidRange; import android.net.Uri; +import android.net.VpnManager; import android.net.metrics.IpConnectivityLog; import android.net.shared.NetworkMonitorUtils; import android.net.shared.PrivateDnsConfig; @@ -272,6 +276,7 @@ import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Predicate; +import java.util.function.Supplier; import kotlin.reflect.KClass; @@ -445,15 +450,21 @@ public class ConnectivityServiceTest { return mPackageManager; } + private int checkMockedPermission(String permission, Supplier ifAbsent) { + final Integer granted = mMockedPermissions.get(permission); + return granted != null ? granted : ifAbsent.get(); + } + @Override public int checkPermission(String permission, int pid, int uid) { - final Integer granted = mMockedPermissions.get(permission); - if (granted == null) { - // All non-mocked permissions should be held by the test or unnecessary: check as - // normal to make sure the code does not rely on unexpected permissions. - return super.checkPermission(permission, pid, uid); - } - return granted; + return checkMockedPermission( + permission, () -> super.checkPermission(permission, pid, uid)); + } + + @Override + public int checkCallingOrSelfPermission(String permission) { + return checkMockedPermission( + permission, () -> super.checkCallingOrSelfPermission(permission)); } @Override @@ -994,6 +1005,7 @@ public class ConnectivityServiceTest { // Careful ! This is different from mNetworkAgent, because MockNetworkAgent does // not inherit from NetworkAgent. private TestNetworkAgentWrapper mMockNetworkAgent; + private int mVpnType = VpnManager.TYPE_VPN_SERVICE; private VpnInfo mVpnInfo; @@ -1014,6 +1026,10 @@ public class ConnectivityServiceTest { updateCapabilities(null /* defaultNetwork */); } + public void setVpnType(int vpnType) { + mVpnType = vpnType; + } + @Override public int getNetId() { if (mMockNetworkAgent == null) { @@ -1032,6 +1048,11 @@ public class ConnectivityServiceTest { return mConnected; // Similar trickery } + @Override + public int getActiveAppVpnType() { + return mVpnType; + } + private void connect(boolean isAlwaysMetered) { mNetworkCapabilities.set(mMockNetworkAgent.getNetworkCapabilities()); mConnected = true; @@ -6382,6 +6403,90 @@ public class ConnectivityServiceTest { assertEquals(Process.INVALID_UID, newNc.getOwnerUid()); } + private void setupConnectionOwnerUid(int vpnOwnerUid, @VpnManager.VpnType int vpnType) + throws Exception { + final Set vpnRange = Collections.singleton(UidRange.createForUser(VPN_USER)); + establishVpn(new LinkProperties(), vpnOwnerUid, vpnRange); + mMockVpn.setVpnType(vpnType); + + final VpnInfo vpnInfo = new VpnInfo(); + vpnInfo.ownerUid = vpnOwnerUid; + mMockVpn.setVpnInfo(vpnInfo); + } + + private void setupConnectionOwnerUidAsVpnApp(int vpnOwnerUid, @VpnManager.VpnType int vpnType) + throws Exception { + setupConnectionOwnerUid(vpnOwnerUid, vpnType); + + // Test as VPN app + mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED); + mServiceContext.setPermission( + NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK, PERMISSION_DENIED); + } + + private ConnectionInfo getTestConnectionInfo() throws Exception { + return new ConnectionInfo( + IPPROTO_TCP, + new InetSocketAddress(InetAddresses.parseNumericAddress("1.2.3.4"), 1234), + new InetSocketAddress(InetAddresses.parseNumericAddress("2.3.4.5"), 2345)); + } + + @Test + public void testGetConnectionOwnerUidPlatformVpn() throws Exception { + final int myUid = Process.myUid(); + setupConnectionOwnerUidAsVpnApp(myUid, VpnManager.TYPE_VPN_PLATFORM); + + try { + mService.getConnectionOwnerUid(getTestConnectionInfo()); + fail("Expected SecurityException for non-VpnService app"); + } catch (SecurityException expected) { + } + } + + @Test + public void testGetConnectionOwnerUidVpnServiceWrongUser() throws Exception { + final int myUid = Process.myUid(); + setupConnectionOwnerUidAsVpnApp(myUid + 1, VpnManager.TYPE_VPN_SERVICE); + + try { + mService.getConnectionOwnerUid(getTestConnectionInfo()); + fail("Expected SecurityException for non-VpnService app"); + } catch (SecurityException expected) { + } + } + + @Test + public void testGetConnectionOwnerUidVpnServiceDoesNotThrow() throws Exception { + final int myUid = Process.myUid(); + setupConnectionOwnerUidAsVpnApp(myUid, VpnManager.TYPE_VPN_SERVICE); + + // TODO: Test the returned UID + mService.getConnectionOwnerUid(getTestConnectionInfo()); + } + + @Test + public void testGetConnectionOwnerUidVpnServiceNetworkStackDoesNotThrow() throws Exception { + final int myUid = Process.myUid(); + setupConnectionOwnerUid(myUid, VpnManager.TYPE_VPN_SERVICE); + mServiceContext.setPermission( + android.Manifest.permission.NETWORK_STACK, PERMISSION_GRANTED); + + // TODO: Test the returned UID + mService.getConnectionOwnerUid(getTestConnectionInfo()); + } + + @Test + public void testGetConnectionOwnerUidVpnServiceMainlineNetworkStackDoesNotThrow() + throws Exception { + final int myUid = Process.myUid(); + setupConnectionOwnerUid(myUid, VpnManager.TYPE_VPN_SERVICE); + mServiceContext.setPermission( + NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK, PERMISSION_GRANTED); + + // TODO: Test the returned UID + mService.getConnectionOwnerUid(getTestConnectionInfo()); + } + private TestNetworkAgentWrapper establishVpn( LinkProperties lp, int ownerUid, Set vpnRange) throws Exception { final TestNetworkAgentWrapper diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index eb78529e87158..ac1c51837e933 100644 --- a/tests/net/java/com/android/server/connectivity/VpnTest.java +++ b/tests/net/java/com/android/server/connectivity/VpnTest.java @@ -656,8 +656,12 @@ public class VpnTest { } private Vpn createVpnAndSetupUidChecks(int... grantedOps) throws Exception { - final Vpn vpn = createVpn(primaryUser.id); - setMockedUsers(primaryUser); + return createVpnAndSetupUidChecks(primaryUser, grantedOps); + } + + private Vpn createVpnAndSetupUidChecks(UserInfo user, int... grantedOps) throws Exception { + final Vpn vpn = createVpn(user.id); + setMockedUsers(user); when(mPackageManager.getPackageUidAsUser(eq(TEST_VPN_PKG), anyInt())) .thenReturn(Process.myUid()); @@ -725,6 +729,19 @@ public class VpnTest { } } + @Test + public void testProvisionVpnProfileRestrictedUser() throws Exception { + final Vpn vpn = + createVpnAndSetupUidChecks( + restrictedProfileA, AppOpsManager.OP_ACTIVATE_PLATFORM_VPN); + + try { + vpn.provisionVpnProfile(TEST_VPN_PKG, mVpnProfile, mKeyStore); + fail("Expected SecurityException due to restricted user"); + } catch (SecurityException expected) { + } + } + @Test public void testDeleteVpnProfile() throws Exception { final Vpn vpn = createVpnAndSetupUidChecks(); @@ -735,6 +752,19 @@ public class VpnTest { .delete(eq(vpn.getProfileNameForPackage(TEST_VPN_PKG)), eq(Process.SYSTEM_UID)); } + @Test + public void testDeleteVpnProfileRestrictedUser() throws Exception { + final Vpn vpn = + createVpnAndSetupUidChecks( + restrictedProfileA, AppOpsManager.OP_ACTIVATE_PLATFORM_VPN); + + try { + vpn.deleteVpnProfile(TEST_VPN_PKG, mKeyStore); + fail("Expected SecurityException due to restricted user"); + } catch (SecurityException expected) { + } + } + @Test public void testGetVpnProfilePrivileged() throws Exception { final Vpn vpn = createVpnAndSetupUidChecks(); @@ -819,6 +849,32 @@ public class VpnTest { eq(TEST_VPN_PKG)); } + @Test + public void testStartVpnProfileRestrictedUser() throws Exception { + final Vpn vpn = + createVpnAndSetupUidChecks( + restrictedProfileA, AppOpsManager.OP_ACTIVATE_PLATFORM_VPN); + + try { + vpn.startVpnProfile(TEST_VPN_PKG, mKeyStore); + fail("Expected SecurityException due to restricted user"); + } catch (SecurityException expected) { + } + } + + @Test + public void testStopVpnProfileRestrictedUser() throws Exception { + final Vpn vpn = + createVpnAndSetupUidChecks( + restrictedProfileA, AppOpsManager.OP_ACTIVATE_PLATFORM_VPN); + + try { + vpn.stopVpnProfile(TEST_VPN_PKG); + fail("Expected SecurityException due to restricted user"); + } catch (SecurityException expected) { + } + } + @Test public void testSetPackageAuthorizationVpnService() throws Exception { final Vpn vpn = createVpnAndSetupUidChecks(); From df50bb850ea0cadfe9244c5fcc2f2f139d5cdb25 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Fri, 24 Jan 2020 21:15:20 -0800 Subject: [PATCH 2/2] Ensure all VPN runners clean up state when exiting This CL tweaks the cleanup flow to ensure that VPN runners exit properly and clean up state. Previously, if a VPN exited before an interface was created and the Interface Observer started watching the virtual interface, some state (eg mConfig) might not get cleaned up. Also as a result of this change, the LegacyVpn no longer implicitly relies on the NetworkManagementEventObserver's watching for interface removed to cleanup mConfig, mStatusIntent, mNetworkCapabilities, and the VPN runner itself, but rather clears the state immediately. Bug: 144246767 Test: FrameworksNetTests passing Change-Id: Ide9daebca9a3fba025e7da5e3fe1d20d7bfdca02 --- .../com/android/server/connectivity/Vpn.java | 65 +++++++++++-------- 1 file changed, 37 insertions(+), 28 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 3e831d187edc7..8692436f92e4b 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -794,10 +794,10 @@ public class Vpn { // ignore } mContext.unbindService(mConnection); - mConnection = null; + cleanupVpnStateLocked(); } else if (mVpnRunner != null) { + // cleanupVpnStateLocked() is called from mVpnRunner.exit() mVpnRunner.exit(); - mVpnRunner = null; } try { @@ -1539,24 +1539,30 @@ public class Vpn { public void interfaceRemoved(String interfaze) { synchronized (Vpn.this) { if (interfaze.equals(mInterface) && jniCheck(interfaze) == 0) { - mStatusIntent = null; - mNetworkCapabilities.setUids(null); - mConfig = null; - mInterface = null; if (mConnection != null) { mContext.unbindService(mConnection); - mConnection = null; - agentDisconnect(); + cleanupVpnStateLocked(); } else if (mVpnRunner != null) { - // agentDisconnect must be called from mVpnRunner.exit() + // cleanupVpnStateLocked() is called from mVpnRunner.exit() mVpnRunner.exit(); - mVpnRunner = null; } } } } }; + private void cleanupVpnStateLocked() { + mStatusIntent = null; + mNetworkCapabilities.setUids(null); + mConfig = null; + mInterface = null; + + // Unconditionally clear both VpnService and VpnRunner fields. + mVpnRunner = null; + mConnection = null; + agentDisconnect(); + } + private void enforceControlPermission() { mContext.enforceCallingPermission(Manifest.permission.CONTROL_VPN, "Unauthorized Caller"); } @@ -2046,7 +2052,25 @@ public class Vpn { public abstract void run(); - protected abstract void exit(); + /** + * Disconnects the NetworkAgent and cleans up all state related to the VpnRunner. + * + *

All outer Vpn instance state is cleaned up in cleanupVpnStateLocked() + */ + protected abstract void exitVpnRunner(); + + /** + * Triggers the cleanup of the VpnRunner, and additionally cleans up Vpn instance-wide state + * + *

This method ensures that simple calls to exit() will always clean up global state + * properly. + */ + protected final void exit() { + synchronized (Vpn.this) { + exitVpnRunner(); + cleanupVpnStateLocked(); + } + } } interface IkeV2VpnRunnerCallback { @@ -2375,17 +2399,6 @@ public class Vpn { } } - /** - * Triggers cleanup of outer class' state - * - *

Can be called from any thread, as it does not mutate state in the Ikev2VpnRunner. - */ - private void cleanupVpnState() { - synchronized (Vpn.this) { - agentDisconnect(); - } - } - /** * Cleans up all Ikev2VpnRunner internal state * @@ -2405,10 +2418,7 @@ public class Vpn { } @Override - public void exit() { - // Cleanup outer class' state immediately, otherwise race conditions may ensue. - cleanupVpnState(); - + public void exitVpnRunner() { mExecutor.execute(() -> { shutdownVpnRunner(); }); @@ -2507,10 +2517,9 @@ public class Vpn { /** Tears down this LegacyVpn connection */ @Override - public void exit() { + public void exitVpnRunner() { // We assume that everything is reset after stopping the daemons. interrupt(); - agentDisconnect(); try { mContext.unregisterReceiver(mBroadcastReceiver); } catch (IllegalArgumentException e) {}