From 2057b41c90b8d584df36e514041e700d4460f8f0 Mon Sep 17 00:00:00 2001 From: chiachangwang Date: Wed, 25 May 2022 11:22:33 +0000 Subject: [PATCH 1/3] Update VPN app exclusion list when packages are added or removed This commit passes through package added/removed broadcasts, triggering the VPN to update the allowed/excluded UIDs. This is required because the VPN app exclusion list is stored in the keystore. The UID range for the VPN network agent should be updated when the package status is updated. As an example, consider the following sequence: - Package A is installed, and in the exclusion list. - A is uninstalled - VPN starts, A is not installed, so no UID A to exclude - A is re-installed after VPN is started - The VPN would include the app A, potentially breaking it. Bug: 231373589 Test: atest FrameworksNetTests Change-Id: I7ee32103ffdfc97e4c95007f35671cefeb9b22d7 (cherry picked from commit 5fda65f94ce902021070ffc24159fb8f201883b3) Merged-In: I7ee32103ffdfc97e4c95007f35671cefeb9b22d7 --- .../com/android/server/VpnManagerService.java | 44 ++++++++++++++++--- .../com/android/server/connectivity/Vpn.java | 16 ++++++- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/VpnManagerService.java b/services/core/java/com/android/server/VpnManagerService.java index d3ef6bed46a09..07b6843b2febd 100644 --- a/services/core/java/com/android/server/VpnManagerService.java +++ b/services/core/java/com/android/server/VpnManagerService.java @@ -45,6 +45,7 @@ import android.os.Build; import android.os.Handler; import android.os.HandlerThread; import android.os.INetworkManagementService; +import android.os.Looper; import android.os.ParcelFileDescriptor; import android.os.Process; import android.os.ServiceManager; @@ -131,6 +132,12 @@ public class VpnManagerService extends IVpnManager.Stub { return INetworkManagementService.Stub.asInterface( ServiceManager.getService(Context.NETWORKMANAGEMENT_SERVICE)); } + + /** Create a VPN. */ + public Vpn createVpn(Looper looper, Context context, INetworkManagementService nms, + INetd netd, int userId) { + return new Vpn(looper, context, nms, netd, userId, new VpnProfileStore()); + } } public VpnManagerService(Context context, Dependencies deps) { @@ -688,6 +695,7 @@ public class VpnManagerService extends IVpnManager.Stub { // Listen to package add and removal events for all users. intentFilter = new IntentFilter(); + intentFilter.addAction(Intent.ACTION_PACKAGE_ADDED); intentFilter.addAction(Intent.ACTION_PACKAGE_REPLACED); intentFilter.addAction(Intent.ACTION_PACKAGE_REMOVED); intentFilter.addDataScheme("package"); @@ -738,6 +746,10 @@ public class VpnManagerService extends IVpnManager.Stub { final boolean isReplacing = intent.getBooleanExtra( Intent.EXTRA_REPLACING, false); onPackageRemoved(packageName, uid, isReplacing); + } else if (Intent.ACTION_PACKAGE_ADDED.equals(action)) { + final boolean isReplacing = intent.getBooleanExtra( + Intent.EXTRA_REPLACING, false); + onPackageAdded(packageName, uid, isReplacing); } else { Log.wtf(TAG, "received unexpected intent: " + action); } @@ -757,15 +769,15 @@ public class VpnManagerService extends IVpnManager.Stub { } }; - private void onUserStarted(int userId) { + @VisibleForTesting + void onUserStarted(int userId) { synchronized (mVpns) { Vpn userVpn = mVpns.get(userId); if (userVpn != null) { loge("Starting user already has a VPN"); return; } - userVpn = new Vpn(mHandler.getLooper(), mContext, mNMS, mNetd, userId, - new VpnProfileStore()); + userVpn = mDeps.createVpn(mHandler.getLooper(), mContext, mNMS, mNetd, userId); mVpns.put(userId, userVpn); if (mUserManager.getUserInfo(userId).isPrimary() && isLockdownVpnEnabled()) { updateLockdownVpn(); @@ -842,7 +854,8 @@ public class VpnManagerService extends IVpnManager.Stub { } } - private void onPackageRemoved(String packageName, int uid, boolean isReplacing) { + @VisibleForTesting + void onPackageRemoved(String packageName, int uid, boolean isReplacing) { if (TextUtils.isEmpty(packageName) || uid < 0) { Log.wtf(TAG, "Invalid package in onPackageRemoved: " + packageName + " | " + uid); return; @@ -851,15 +864,34 @@ public class VpnManagerService extends IVpnManager.Stub { final int userId = UserHandle.getUserId(uid); synchronized (mVpns) { final Vpn vpn = mVpns.get(userId); - if (vpn == null) { + if (vpn == null || isReplacing) { return; } // Legacy always-on VPN won't be affected since the package name is not set. - if (TextUtils.equals(vpn.getAlwaysOnPackage(), packageName) && !isReplacing) { + if (TextUtils.equals(vpn.getAlwaysOnPackage(), packageName)) { log("Removing always-on VPN package " + packageName + " for user " + userId); vpn.setAlwaysOnPackage(null, false, null); } + + vpn.refreshPlatformVpnAppExclusionList(); + } + } + + @VisibleForTesting + void onPackageAdded(String packageName, int uid, boolean isReplacing) { + if (TextUtils.isEmpty(packageName) || uid < 0) { + Log.wtf(TAG, "Invalid package in onPackageAdded: " + packageName + " | " + uid); + return; + } + + final int userId = UserHandle.getUserId(uid); + synchronized (mVpns) { + final Vpn vpn = mVpns.get(userId); + + if (vpn != null && !isReplacing) { + vpn.refreshPlatformVpnAppExclusionList(); + } } } diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index af15735e92366..40f3c02f33778 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -4132,6 +4132,20 @@ public class Vpn { @NonNull List excludedApps) { enforceNotRestrictedUser(); if (!storeAppExclusionList(packageName, excludedApps)) return false; + + updateAppExclusionList(excludedApps); + + return true; + } + + /** + * Triggers an update of the VPN network's excluded UIDs if a VPN is running. + */ + public synchronized void refreshPlatformVpnAppExclusionList() { + updateAppExclusionList(getAppExclusionList(mPackage)); + } + + private synchronized void updateAppExclusionList(@NonNull List excludedApps) { // Re-build and update NetworkCapabilities via NetworkAgent. if (mNetworkAgent != null) { // Only update the platform VPN @@ -4144,8 +4158,6 @@ public class Vpn { mNetworkAgent.sendNetworkCapabilities(mNetworkCapabilities); } } - - return true; } /** From 1526a8b5d13b1602b89c1642e162a9f587b1cd2a Mon Sep 17 00:00:00 2001 From: Yan Yan Date: Thu, 28 Jul 2022 00:30:45 +0000 Subject: [PATCH 2/3] Make Vpn more testable with a wrapper class This commit create a wrapper class for NetworkAgent to make the Vpn code more testable. Bug: 192077544 Test: atest VpnTest Change-Id: Ia599fed8c874ea9622a1f56f686de293bcbfddb8 (cherry picked from commit 62fd3bb4546b7e2ad06cbec922e73f756c07bb77) Merged-In: Ia599fed8c874ea9622a1f56f686de293bcbfddb8 --- .../com/android/server/connectivity/Vpn.java | 112 +++++++++++++++--- 1 file changed, 95 insertions(+), 17 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 40f3c02f33778..e1b555fc996ed 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -514,12 +514,8 @@ public class Vpn { @NonNull NetworkScore score, @NonNull NetworkAgentConfig config, @Nullable NetworkProvider provider) { - return new NetworkAgent(context, looper, logTag, nc, lp, score, config, provider) { - @Override - public void onNetworkUnwanted() { - // We are user controlled, not driven by NetworkRequest. - } - }; + return new VpnNetworkAgentWrapper( + context, looper, logTag, nc, lp, score, config, provider); } } @@ -1824,7 +1820,7 @@ public class Vpn { Log.wtf(TAG, "Failed to add restricted user to owner", e); } if (mNetworkAgent != null) { - mNetworkAgent.sendNetworkCapabilities(mNetworkCapabilities); + doSendNetworkCapabilities(mNetworkAgent, mNetworkCapabilities); } } setVpnForcedLocked(mLockdown); @@ -1854,7 +1850,7 @@ public class Vpn { Log.wtf(TAG, "Failed to remove restricted user to owner", e); } if (mNetworkAgent != null) { - mNetworkAgent.sendNetworkCapabilities(mNetworkCapabilities); + doSendNetworkCapabilities(mNetworkAgent, mNetworkCapabilities); } } setVpnForcedLocked(mLockdown); @@ -2084,7 +2080,7 @@ public class Vpn { return false; } boolean success = jniAddAddress(mInterface, address, prefixLength); - mNetworkAgent.sendLinkProperties(makeLinkProperties()); + doSendLinkProperties(mNetworkAgent, makeLinkProperties()); return success; } @@ -2093,7 +2089,7 @@ public class Vpn { return false; } boolean success = jniDelAddress(mInterface, address, prefixLength); - mNetworkAgent.sendLinkProperties(makeLinkProperties()); + doSendLinkProperties(mNetworkAgent, makeLinkProperties()); return success; } @@ -2107,8 +2103,11 @@ public class Vpn { // Make defensive copy since the content of array might be altered by the caller. mConfig.underlyingNetworks = (networks != null) ? Arrays.copyOf(networks, networks.length) : null; - mNetworkAgent.setUnderlyingNetworks((mConfig.underlyingNetworks != null) - ? Arrays.asList(mConfig.underlyingNetworks) : null); + doSetUnderlyingNetworks( + mNetworkAgent, + (mConfig.underlyingNetworks != null) + ? Arrays.asList(mConfig.underlyingNetworks) + : null); return true; } @@ -2917,7 +2916,7 @@ public class Vpn { return; // Link properties are already sent. } else { // Underlying networks also set in agentConnect() - networkAgent.setUnderlyingNetworks(Collections.singletonList(network)); + doSetUnderlyingNetworks(networkAgent, Collections.singletonList(network)); mNetworkCapabilities = new NetworkCapabilities.Builder(mNetworkCapabilities) .setUnderlyingNetworks(Collections.singletonList(network)) @@ -2927,7 +2926,7 @@ public class Vpn { lp = makeLinkProperties(); // Accesses VPN instance fields; must be locked } - networkAgent.sendLinkProperties(lp); + doSendLinkProperties(networkAgent, lp); } catch (Exception e) { Log.d(TAG, "Error in ChildOpened for token " + token, e); onSessionLost(token, e); @@ -2994,7 +2993,7 @@ public class Vpn { new NetworkCapabilities.Builder(mNetworkCapabilities) .setUnderlyingNetworks(Collections.singletonList(network)) .build(); - mNetworkAgent.setUnderlyingNetworks(Collections.singletonList(network)); + doSetUnderlyingNetworks(mNetworkAgent, Collections.singletonList(network)); } mTunnelIface.setUnderlyingNetwork(network); @@ -3438,7 +3437,7 @@ public class Vpn { null /*gateway*/, null /*iface*/, RTN_UNREACHABLE)); } if (mNetworkAgent != null) { - mNetworkAgent.sendLinkProperties(makeLinkProperties()); + doSendLinkProperties(mNetworkAgent, makeLinkProperties()); } } } @@ -4155,7 +4154,7 @@ public class Vpn { .setUids(createUserAndRestrictedProfilesRanges( mUserId, null /* allowedApplications */, excludedApps)) .build(); - mNetworkAgent.sendNetworkCapabilities(mNetworkCapabilities); + doSendNetworkCapabilities(mNetworkAgent, mNetworkCapabilities); } } } @@ -4232,6 +4231,85 @@ public class Vpn { return isCurrentIkev2VpnLocked(packageName) ? makeVpnProfileStateLocked() : null; } + /** Proxy to allow different testing setups */ + // TODO: b/240492694 Remove VpnNetworkAgentWrapper and this method when + // NetworkAgent#sendLinkProperties can be un-finalized. + private static void doSendLinkProperties( + @NonNull NetworkAgent agent, @NonNull LinkProperties lp) { + if (agent instanceof VpnNetworkAgentWrapper) { + ((VpnNetworkAgentWrapper) agent).doSendLinkProperties(lp); + } else { + agent.sendLinkProperties(lp); + } + } + + /** Proxy to allow different testing setups */ + // TODO: b/240492694 Remove VpnNetworkAgentWrapper and this method when + // NetworkAgent#sendNetworkCapabilities can be un-finalized. + private static void doSendNetworkCapabilities( + @NonNull NetworkAgent agent, @NonNull NetworkCapabilities nc) { + if (agent instanceof VpnNetworkAgentWrapper) { + ((VpnNetworkAgentWrapper) agent).doSendNetworkCapabilities(nc); + } else { + agent.sendNetworkCapabilities(nc); + } + } + + /** Proxy to allow different testing setups */ + // TODO: b/240492694 Remove VpnNetworkAgentWrapper and this method when + // NetworkAgent#setUnderlyingNetworks can be un-finalized. + private static void doSetUnderlyingNetworks( + @NonNull NetworkAgent agent, @NonNull List networks) { + if (agent instanceof VpnNetworkAgentWrapper) { + ((VpnNetworkAgentWrapper) agent).doSetUnderlyingNetworks(networks); + } else { + agent.setUnderlyingNetworks(networks); + } + } + + /** + * Proxy to allow testing + * + * @hide + */ + // TODO: b/240492694 Remove VpnNetworkAgentWrapper when NetworkAgent's methods can be + // un-finalized. + @VisibleForTesting + public static class VpnNetworkAgentWrapper extends NetworkAgent { + /** Create an VpnNetworkAgentWrapper */ + public VpnNetworkAgentWrapper( + @NonNull Context context, + @NonNull Looper looper, + @NonNull String logTag, + @NonNull NetworkCapabilities nc, + @NonNull LinkProperties lp, + @NonNull NetworkScore score, + @NonNull NetworkAgentConfig config, + @Nullable NetworkProvider provider) { + super(context, looper, logTag, nc, lp, score, config, provider); + } + + /** Update the LinkProperties */ + public void doSendLinkProperties(@NonNull LinkProperties lp) { + sendLinkProperties(lp); + } + + /** Update the NetworkCapabilities */ + public void doSendNetworkCapabilities(@NonNull NetworkCapabilities nc) { + sendNetworkCapabilities(nc); + } + + /** Set the underlying networks */ + public void doSetUnderlyingNetworks(@NonNull List networks) { + setUnderlyingNetworks(networks); + } + + @Override + public void onNetworkUnwanted() { + // We are user controlled, not driven by NetworkRequest. + } + } + /** * Proxy to allow testing * From 3b6806554716eef81fe541aadc823c188da58ffb Mon Sep 17 00:00:00 2001 From: chiachangwang Date: Thu, 30 Jun 2022 01:48:45 +0000 Subject: [PATCH 3/3] Update the prefix in keystore for app exclusion Update the string without the underscore to prevent conflict with the existing VPN prefix. Settings app may get exception when the vpn exclusion list was set since the logic is used in the Settings relying on the prefix match. The current app exclusion prefix is also started with "VPN_", so it will be mis-used by Settings as a VPN profile. Bug: 237345836 Test: atest FrameworksNetTests Change-Id: I01e773cc15eb1ae5ffaa12aef124bbbf390cc004 (cherry picked from commit 89ca8b306368ddc5afbe0d3cf0dff8b8b3918732) Merged-In: I01e773cc15eb1ae5ffaa12aef124bbbf390cc004 --- services/core/java/com/android/server/connectivity/Vpn.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index e1b555fc996ed..9691b20066c7d 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -186,7 +186,7 @@ public class Vpn { private static final boolean LOGD = true; private static final String ANDROID_KEYSTORE_PROVIDER = "AndroidKeyStore"; /** Key containing prefix of vpn app excluded list */ - @VisibleForTesting static final String VPN_APP_EXCLUDED = "VPN_APP_EXCLUDED_"; + @VisibleForTesting static final String VPN_APP_EXCLUDED = "VPNAPPEXCLUDED_"; // Length of time (in milliseconds) that an app hosting an always-on VPN is placed on // the device idle allowlist during service launch and VPN bootstrap.