From eeaf8557510e3c592aac72502d250685a3c9d254 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 3 Jul 2023 21:01:16 +0900 Subject: Fix: VPN is off but key is displayed in the status bar There seem to be a narrow race condition in which it's possible to turn the VPN off where it won't null out the config. It's possible to make this happen by trying to migrate to a network that won't carry the VPN packets (UDP 4500 or ESP, respectively) and turning off the VPN while the IKE session is trying to establish. Alternatively, this might be a visibility issue where mConfig is nulled out by a thread, but the binder thread reading it from #getVpnConfig returns the old object, not yet nulled out. To fix this, make mConfig guarded by "this", which it almost was already. Bug: 289187571 Test: manual Turning off underlying networks, VPN stays "connected" in settings Adding a settings VPN, making sure the status is correct Try and fail to cause the race condition (cherry picked from https://android-review.googlesource.com/q/commit:25181986301828f8be5101bb6b08a72dda99b0ad) Merged-In: I1c19eb33f0c1d0f1ccca6ac17a709f8bc1ad9f94 Change-Id: I1c19eb33f0c1d0f1ccca6ac17a709f8bc1ad9f94 --- .../java/com/android/server/connectivity/Vpn.java | 125 ++++++++++++--------- 1 file changed, 70 insertions(+), 55 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index e85eee817d29..c1df335dee47 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -380,6 +380,7 @@ public class Vpn { private final INetworkManagementService mNms; private final INetd mNetd; @VisibleForTesting + @GuardedBy("this") protected VpnConfig mConfig; private final NetworkProvider mNetworkProvider; @VisibleForTesting @@ -1598,6 +1599,8 @@ public class Vpn { return network; } + // TODO : this is not synchronized(this) but reads from mConfig, which is dangerous + // This file makes an effort to avoid partly initializing mConfig, but this is still not great private LinkProperties makeLinkProperties() { // The design of disabling IPv6 is only enabled for IKEv2 VPN because it needs additional // logic to handle IPv6 only VPN, and the IPv6 only VPN may be restarted when its MTU @@ -1679,6 +1682,7 @@ public class Vpn { * registering a new NetworkAgent. This is not always possible if the new VPN configuration * has certain changes, in which case this method would just return {@code false}. */ + // TODO : this method is not synchronized(this) but reads from mConfig private boolean updateLinkPropertiesInPlaceIfPossible(NetworkAgent agent, VpnConfig oldConfig) { // NetworkAgentConfig cannot be updated without registering a new NetworkAgent. // Strictly speaking, bypassability is affected by lockdown and therefore it's possible @@ -2269,7 +2273,12 @@ public class Vpn { */ public synchronized VpnConfig getVpnConfig() { enforceControlPermission(); - return mConfig; + // Constructor of VpnConfig cannot take a null parameter. Return null directly if mConfig is + // null + if (mConfig == null) return null; + // mConfig is guarded by "this" and can be modified by another thread as soon as + // this method returns, so this method must return a copy. + return new VpnConfig(mConfig); } @Deprecated @@ -2315,6 +2324,7 @@ public class Vpn { } }; + @GuardedBy("this") private void cleanupVpnStateLocked() { mStatusIntent = null; resetNetworkCapabilities(); @@ -2837,9 +2847,7 @@ public class Vpn { } final boolean isLegacyVpn = mVpnRunner instanceof LegacyVpnRunner; - mVpnRunner.exit(); - mVpnRunner = null; // LegacyVpn uses daemons that must be shut down before new ones are brought up. // The same limitation does not apply to Platform VPNs. @@ -3084,6 +3092,7 @@ public class Vpn { } }; + // GuardedBy("Vpn.this") (annotation can't be applied to constructor) IkeV2VpnRunner( @NonNull Ikev2VpnProfile profile, @NonNull ScheduledThreadPoolExecutor executor) { super(TAG); @@ -3710,11 +3719,14 @@ public class Vpn { } public void updateVpnTransportInfoAndNetCap(int keepaliveDelaySec) { - final VpnTransportInfo info = new VpnTransportInfo( - getActiveVpnType(), - mConfig.session, - mConfig.allowBypass && !mLockdown, - areLongLivedTcpConnectionsExpensive(keepaliveDelaySec)); + final VpnTransportInfo info; + synchronized (Vpn.this) { + info = new VpnTransportInfo( + getActiveVpnType(), + mConfig.session, + mConfig.allowBypass && !mLockdown, + areLongLivedTcpConnectionsExpensive(keepaliveDelaySec)); + } final boolean ncUpdateRequired = !info.equals(mNetworkCapabilities.getTransportInfo()); if (ncUpdateRequired) { mNetworkCapabilities = new NetworkCapabilities.Builder(mNetworkCapabilities) @@ -4220,7 +4232,7 @@ public class Vpn { * consistency of the Ikev2VpnRunner fields. */ private void disconnectVpnRunner() { - mEventChanges.log("[VPNRunner] Disconnect runner, underlying network" + mActiveNetwork); + mEventChanges.log("[VPNRunner] Disconnect runner, underlying net " + mActiveNetwork); mActiveNetwork = null; mUnderlyingNetworkCapabilities = null; mUnderlyingLinkProperties = null; @@ -4293,6 +4305,7 @@ public class Vpn { } }; + // GuardedBy("Vpn.this") (annotation can't be applied to constructor) LegacyVpnRunner(VpnConfig config, String[] racoon, String[] mtpd, VpnProfile profile) { super(TAG); if (racoon == null && mtpd == null) { @@ -4500,46 +4513,46 @@ public class Vpn { } // Set the interface and the addresses in the config. - mConfig.interfaze = parameters[0].trim(); + synchronized (Vpn.this) { + mConfig.interfaze = parameters[0].trim(); - mConfig.addLegacyAddresses(parameters[1]); - // Set the routes if they are not set in the config. - if (mConfig.routes == null || mConfig.routes.isEmpty()) { - mConfig.addLegacyRoutes(parameters[2]); - } + mConfig.addLegacyAddresses(parameters[1]); + // Set the routes if they are not set in the config. + if (mConfig.routes == null || mConfig.routes.isEmpty()) { + mConfig.addLegacyRoutes(parameters[2]); + } - // Set the DNS servers if they are not set in the config. - if (mConfig.dnsServers == null || mConfig.dnsServers.size() == 0) { - String dnsServers = parameters[3].trim(); - if (!dnsServers.isEmpty()) { - mConfig.dnsServers = Arrays.asList(dnsServers.split(" ")); + // Set the DNS servers if they are not set in the config. + if (mConfig.dnsServers == null || mConfig.dnsServers.size() == 0) { + String dnsServers = parameters[3].trim(); + if (!dnsServers.isEmpty()) { + mConfig.dnsServers = Arrays.asList(dnsServers.split(" ")); + } } - } - // Set the search domains if they are not set in the config. - if (mConfig.searchDomains == null || mConfig.searchDomains.size() == 0) { - String searchDomains = parameters[4].trim(); - if (!searchDomains.isEmpty()) { - mConfig.searchDomains = Arrays.asList(searchDomains.split(" ")); + // Set the search domains if they are not set in the config. + if (mConfig.searchDomains == null || mConfig.searchDomains.size() == 0) { + String searchDomains = parameters[4].trim(); + if (!searchDomains.isEmpty()) { + mConfig.searchDomains = Arrays.asList(searchDomains.split(" ")); + } } - } - // Add a throw route for the VPN server endpoint, if one was specified. - if (endpointAddress instanceof Inet4Address) { - mConfig.routes.add(new RouteInfo( - new IpPrefix(endpointAddress, 32), null /*gateway*/, - null /*iface*/, RTN_THROW)); - } else if (endpointAddress instanceof Inet6Address) { - mConfig.routes.add(new RouteInfo( - new IpPrefix(endpointAddress, 128), null /*gateway*/, - null /*iface*/, RTN_THROW)); - } else { - Log.e(TAG, "Unknown IP address family for VPN endpoint: " - + endpointAddress); - } + // Add a throw route for the VPN server endpoint, if one was specified. + if (endpointAddress instanceof Inet4Address) { + mConfig.routes.add(new RouteInfo( + new IpPrefix(endpointAddress, 32), null /*gateway*/, + null /*iface*/, RTN_THROW)); + } else if (endpointAddress instanceof Inet6Address) { + mConfig.routes.add(new RouteInfo( + new IpPrefix(endpointAddress, 128), null /*gateway*/, + null /*iface*/, RTN_THROW)); + } else { + Log.e(TAG, "Unknown IP address family for VPN endpoint: " + + endpointAddress); + } - // Here is the last step and it must be done synchronously. - synchronized (Vpn.this) { + // Here is the last step and it must be done synchronously. // Set the start time mConfig.startTime = SystemClock.elapsedRealtime(); @@ -4773,25 +4786,26 @@ public class Vpn { try { // Build basic config - mConfig = new VpnConfig(); + final VpnConfig config = new VpnConfig(); if (VpnConfig.LEGACY_VPN.equals(packageName)) { - mConfig.legacy = true; - mConfig.session = profile.name; - mConfig.user = profile.key; + config.legacy = true; + config.session = profile.name; + config.user = profile.key; // TODO: Add support for configuring meteredness via Settings. Until then, use a // safe default. - mConfig.isMetered = true; + config.isMetered = true; } else { - mConfig.user = packageName; - mConfig.isMetered = profile.isMetered; + config.user = packageName; + config.isMetered = profile.isMetered; } - mConfig.startTime = SystemClock.elapsedRealtime(); - mConfig.proxyInfo = profile.proxy; - mConfig.requiresInternetValidation = profile.requiresInternetValidation; - mConfig.excludeLocalRoutes = profile.excludeLocalRoutes; - mConfig.allowBypass = profile.isBypassable; - mConfig.disallowedApplications = getAppExclusionList(mPackage); + config.startTime = SystemClock.elapsedRealtime(); + config.proxyInfo = profile.proxy; + config.requiresInternetValidation = profile.requiresInternetValidation; + config.excludeLocalRoutes = profile.excludeLocalRoutes; + config.allowBypass = profile.isBypassable; + config.disallowedApplications = getAppExclusionList(mPackage); + mConfig = config; switch (profile.type) { case VpnProfile.TYPE_IKEV2_IPSEC_USER_PASS: @@ -4805,6 +4819,7 @@ public class Vpn { mVpnRunner.start(); break; default: + mConfig = null; updateState(DetailedState.FAILED, "Invalid platform VPN type"); Log.d(TAG, "Unknown VPN profile type: " + profile.type); break; -- cgit v1.2.3-59-g8ed1b