From 515efcf47df513731ae46d0ccfb340604acb53f9 Mon Sep 17 00:00:00 2001 From: Paul Hu Date: Tue, 15 Apr 2025 09:54:50 +0000 Subject: [PATCH] Update link properties early for VPN networks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When LinkProperties are updated after a private DNS configuration is set in updateNetworkInfo(), particularly when private DNS mode is set to opportunistic and the VPN server uses its own DNS servers, this can cause the private DNS to use the underlying network for TLS handshaking, potentially leading to a leak. To fix this, adjusting the order of operations—setting the private DNS configuration before updating LinkProperties should resolve the issue. This ensures that private DNS validation occurs after the VPN's network configuration is fully applied. Bug: 274867806 Test: atest ConnectivityCoverageTests Flag: EXEMPT legacy flag early_link_properties_update_for_vpn Change-Id: If684b51e6522a8284d5baee8db0d2f767405d359 --- .../android/server/ConnectivityService.java | 14 ++- .../connectivity/ConnectivityFlags.java | 9 ++ .../server/ConnectivityServiceTest.java | 1 + .../connectivityservice/CSNetworkAgentTest.kt | 119 ++++++++++++++++++ .../server/connectivityservice/base/CSTest.kt | 5 +- .../connectivityservice/base/CSTestHelpers.kt | 5 +- 6 files changed, 148 insertions(+), 5 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 529c51d6bd..cac31017a3 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -549,6 +549,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // Flag to delay callbacks for frozen apps, suppressing duplicate and stale callbacks. private final boolean mQueueCallbacksForFrozenApps; + // Flag for early link properties update + private final boolean mSupportEarlyLinkPropertiesUpdateForVPN; /** * Uids ConnectivityService tracks blocked status of to send blocked status callbacks. @@ -1973,6 +1975,8 @@ public class ConnectivityService extends IConnectivityManager.Stub mQueueNetworkAgentEventsInSystemServer = mDeps.isAtLeastB() && mDeps.isFeatureNotChickenedOut(context, ConnectivityFlags.QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER); + mSupportEarlyLinkPropertiesUpdateForVPN = mDeps.isFeatureNotChickenedOut(context, + ConnectivityFlags.EARLY_LINK_PROPERTIES_UPDATE_FOR_VPN); // registerUidFrozenStateChangedCallback is only available on U+ mQueueCallbacksForFrozenApps = mDeps.isAtLeastU() && mDeps.isFeatureNotChickenedOut(context, QUEUE_CALLBACKS_FOR_FROZEN_APPS); @@ -5532,6 +5536,10 @@ public class ConnectivityService extends IConnectivityManager.Stub && !caps.hasTransport(TRANSPORT_THREAD)); } + private boolean shouldUpdateLinkPropertiesEarlyForVPNNetwork(@NonNull NetworkAgentInfo nai) { + return mSupportEarlyLinkPropertiesUpdateForVPN && nai.isVPN(); + } + private boolean shouldCreateNativeNetwork(@NonNull NetworkAgentInfo nai, @NonNull NetworkInfo.State state) { if (nai.isCreated()) return false; @@ -12218,7 +12226,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // interfaces and routing rules have been added, DNS servers programmed, etc. // For VPNs, this must be done before the capabilities are updated, because as soon as // that happens, UIDs are routed to the network. - if (shouldCreateNetworksImmediately(networkAgent.getCapsNoCopy())) { + if (shouldCreateNetworksImmediately(networkAgent.getCapsNoCopy()) + || shouldUpdateLinkPropertiesEarlyForVPNNetwork(networkAgent)) { applyInitialLinkProperties(networkAgent); } @@ -12243,7 +12252,8 @@ public class ConnectivityService extends IConnectivityManager.Stub networkAgent.getAndSetNetworkCapabilities(networkAgent.networkCapabilities); handlePerNetworkPrivateDnsConfig(networkAgent, mDnsManager.getPrivateDnsConfig()); - if (!shouldCreateNetworksImmediately(networkAgent.getCapsNoCopy())) { + if (!(shouldCreateNetworksImmediately(networkAgent.getCapsNoCopy()) + || shouldUpdateLinkPropertiesEarlyForVPNNetwork(networkAgent))) { applyInitialLinkProperties(networkAgent); } else { // The network was created when the agent registered, and the LinkProperties are diff --git a/service/src/com/android/server/connectivity/ConnectivityFlags.java b/service/src/com/android/server/connectivity/ConnectivityFlags.java index 1655328816..06f3d63291 100644 --- a/service/src/com/android/server/connectivity/ConnectivityFlags.java +++ b/service/src/com/android/server/connectivity/ConnectivityFlags.java @@ -70,6 +70,15 @@ public final class ConnectivityFlags { public static final String CONSTRAINED_DATA_SATELLITE_OPTIN = "constrained_data_satellite_optin"; + /** + * A feature flag to control whether the early link properties update for vpn should be enabled. + * + * Note: This feature is automatically enabled if the flag + * QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER is enabled. + */ + public static final String EARLY_LINK_PROPERTIES_UPDATE_FOR_VPN = + "early_link_properties_update_for_vpn"; + private boolean mNoRematchAllRequestsOnRegister; /** diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index ce04e94d7c..1a3a399024 100755 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -2209,6 +2209,7 @@ public class ConnectivityServiceTest { case ConnectivityFlags.QUEUE_CALLBACKS_FOR_FROZEN_APPS: case ConnectivityFlags.QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER: case ConnectivityFlags.CLOSE_QUIC_CONNECTION: + case ConnectivityFlags.EARLY_LINK_PROPERTIES_UPDATE_FOR_VPN: return true; default: throw new UnsupportedOperationException("Unknown flag " + name diff --git a/tests/unit/java/com/android/server/connectivityservice/CSNetworkAgentTest.kt b/tests/unit/java/com/android/server/connectivityservice/CSNetworkAgentTest.kt index 547a4cccf2..c07d0e2bc2 100644 --- a/tests/unit/java/com/android/server/connectivityservice/CSNetworkAgentTest.kt +++ b/tests/unit/java/com/android/server/connectivityservice/CSNetworkAgentTest.kt @@ -16,16 +16,27 @@ package com.android.server +import android.net.ConnectivitySettingsManager +import android.net.InetAddresses +import android.net.IpPrefix +import android.net.LinkAddress +import android.net.LinkProperties import android.net.NativeNetworkConfig +import android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET import android.net.NetworkCapabilities.NET_CAPABILITY_NOT_VPN +import android.net.NetworkCapabilities.NET_CAPABILITY_TRUSTED import android.net.NetworkCapabilities.TRANSPORT_VPN import android.net.NetworkCapabilities.TRANSPORT_WIFI +import android.net.ResolverParamsParcel +import android.net.RouteInfo import android.net.VpnManager import android.net.VpnTransportInfo import android.net.netd.aidl.NativeUidRangeConfig import android.os.Build import android.os.Process import android.util.Range +import com.android.server.connectivity.ConnectivityFlags.EARLY_LINK_PROPERTIES_UPDATE_FOR_VPN +import com.android.server.connectivity.ConnectivityFlags.QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER import com.android.testutils.ConnectivityModuleTest import com.android.testutils.DevSdkIgnoreRule import com.android.testutils.DevSdkIgnoreRunner @@ -33,13 +44,20 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentMatchers.anyInt import org.mockito.ArgumentMatchers.argThat +import org.mockito.ArgumentMatchers.eq import org.mockito.Mockito.inOrder import org.mockito.Mockito.never +import org.mockito.Mockito.times + +private const val DNS_ADDR = "8.8.8.8" +private const val IPV4_ADDR = "192.168.2.1" +private const val ROUTE_PREFIX = "0.0.0.0/0" @DevSdkIgnoreRunner.MonitorThreadLeak @RunWith(DevSdkIgnoreRunner::class) @DevSdkIgnoreRule.IgnoreUpTo(Build.VERSION_CODES.S) class CSNetworkAgentTest : CSTest() { + @Test fun testVpnUidAgent() = testUidAgent( TRANSPORT_VPN, expectAddUidRanges = true @@ -89,4 +107,105 @@ class CSNetworkAgentTest : CSTest() { // The old method should never be called in any case netdInOrder.verify(netd, never()).networkAddUidRanges(anyInt(), any()) } + + // Test the early link properties update for the VPN network when the flag + // QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER is disabled. That flag is enabled by default in + // the CSTest. + @ConnectivityModuleTest + @FeatureFlags(flags = [Flag(QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER, false)]) + @Test + fun testEarlyLinkPropertiesUpdateForVPN() = + testEarlyLinkPropertiesUpdate(TRANSPORT_VPN, expectEarlyLinkPropertiesUpdate = true) + + // Test the early link properties update for the non-VPN network when the flag + // QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER is disabled. That flag is enabled by default in + // the CSTest. + @ConnectivityModuleTest + @FeatureFlags(flags = [Flag(QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER, false)]) + @Test + fun testEarlyLinkPropertiesUpdateForNonVPN() = + testEarlyLinkPropertiesUpdate(TRANSPORT_WIFI, expectEarlyLinkPropertiesUpdate = false) + + // Test the early link properties update for the VPN network when both flags + // QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER and EARLY_LINK_PROPERTIES_UPDATE_FOR_VPN are + // disabled. These flags are enabled by default in the CSTest. + @ConnectivityModuleTest + @FeatureFlags( + flags = [Flag(QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER, false), + Flag(EARLY_LINK_PROPERTIES_UPDATE_FOR_VPN, false)] + ) + @Test + fun testEarlyLinkPropertiesUpdateDisabledForVPN() = + testEarlyLinkPropertiesUpdate(TRANSPORT_VPN, expectEarlyLinkPropertiesUpdate = false) + + private fun testEarlyLinkPropertiesUpdate( + transport: Int, + expectEarlyLinkPropertiesUpdate: Boolean + ) { + val netdInOrder = inOrder(netd) + val resolverInOrder = inOrder(dnsResolver) + + val nc = defaultNc() + .addTransportType(transport) + .addCapability(NET_CAPABILITY_TRUSTED) + .addCapability(NET_CAPABILITY_INTERNET) + if (TRANSPORT_VPN == transport) { + nc.removeCapability(NET_CAPABILITY_NOT_VPN) + nc.setTransportInfo( + VpnTransportInfo( + VpnManager.TYPE_VPN_SERVICE, + "MySession12345", + true /* bypassable */, + false /* longLivedTcpConnectionsExpensive */ + ) + ) + } + val linkAddress = LinkAddress(InetAddresses.parseNumericAddress(IPV4_ADDR), 32) + val lp = LinkProperties().apply { + addLinkAddress(linkAddress) + addRoute(RouteInfo(IpPrefix(ROUTE_PREFIX), null, null)) + addDnsServer(InetAddresses.parseNumericAddress(DNS_ADDR)) + } + ConnectivitySettingsManager.setPrivateDnsMode( + context, + ConnectivitySettingsManager.PRIVATE_DNS_MODE_OPPORTUNISTIC + ) + val agent = Agent(nc = nc, lp = lp) + agent.connect() + + netdInOrder.verify(netd).networkCreate(argThat { it.netId == agent.network.netId }) + netdInOrder.verify(netd).networkAddRouteParcel( + eq(agent.network.netId), + argThat { it.destination == ROUTE_PREFIX } + ) + netdInOrder.verify(netd).networkAddRouteParcel( + eq(agent.network.netId), + argThat { it.destination == linkAddress.toString() } + ) + + if (expectEarlyLinkPropertiesUpdate) { + // Update the DNS without the TLS servers because the private DNS hasn't been set. + resolverInOrder.verify(dnsResolver).setResolverConfiguration( + argThat { it: ResolverParamsParcel -> + it.netId == agent.network.netId && + it.servers.any { server -> server == DNS_ADDR } && + it.tlsServers.isEmpty() } + ) + // The private DNS has been set. Update the DNS again with the TLS servers. + resolverInOrder.verify(dnsResolver).setResolverConfiguration( + argThat { + it.netId == agent.network.netId && + it.servers.any { server -> server == DNS_ADDR } && + it.tlsServers.any { server -> server == DNS_ADDR }} + ) + } else { + // Update the DNS with the TLS servers because the private DNS has been set. + resolverInOrder.verify(dnsResolver, times(2)).setResolverConfiguration( + argThat { + it.netId == agent.network.netId && + it.servers.any { server -> server == DNS_ADDR } && + it.tlsServers.any { server -> server == DNS_ADDR }} + ) + } + } } diff --git a/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt b/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt index 3b8b27a57d..afa8d13ac1 100644 --- a/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt +++ b/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt @@ -28,6 +28,7 @@ import android.content.pm.PackageManager.PERMISSION_GRANTED import android.content.pm.UserInfo import android.content.res.Resources import android.net.ConnectivityManager +import android.net.IDnsResolver import android.net.INetd import android.net.INetd.PERMISSION_INTERNET import android.net.InetAddresses @@ -177,6 +178,7 @@ open class CSTest { it[ConnectivityFlags.QUEUE_CALLBACKS_FOR_FROZEN_APPS] = true it[ConnectivityFlags.QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER] = true it[ConnectivityFlags.CLOSE_QUIC_CONNECTION] = true + it[ConnectivityFlags.EARLY_LINK_PROPERTIES_UPDATE_FOR_VPN] = true } fun setFeatureEnabled(flag: String, enabled: Boolean) = enabledFeatures.set(flag, enabled) @@ -227,6 +229,7 @@ open class CSTest { val satelliteAccessController = mock() val quicConnectionCloser = mock() val destroySocketsWrapper = mock() + val dnsResolver = mock() val deps = CSDeps() val permDeps = PermDeps() @@ -266,7 +269,7 @@ open class CSTest { alarmHandlerThread = HandlerThread("TestAlarmManager").also { it.start() } alarmManager = makeMockAlarmManager(alarmHandlerThread) - service = makeConnectivityService(context, netd, deps, permDeps).also { + service = makeConnectivityService(context, netd, deps, permDeps, dnsResolver).also { it.systemReadyInternal() } cm = ConnectivityManager(context, service) diff --git a/tests/unit/java/com/android/server/connectivityservice/base/CSTestHelpers.kt b/tests/unit/java/com/android/server/connectivityservice/base/CSTestHelpers.kt index 827d73771f..69ff193594 100644 --- a/tests/unit/java/com/android/server/connectivityservice/base/CSTestHelpers.kt +++ b/tests/unit/java/com/android/server/connectivityservice/base/CSTestHelpers.kt @@ -223,11 +223,12 @@ internal fun makeConnectivityService( context: Context, netd: INetd, deps: Dependencies, - mPermDeps: PermissionMonitor.Dependencies + mPermDeps: PermissionMonitor.Dependencies, + resolver: IDnsResolver ) = ConnectivityService( context, - mock(), + resolver, mock(), netd, deps,