From 06d81d2c624e7221b62adb8cefc288544585cfd0 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 30 May 2019 15:40:11 +0900 Subject: [PATCH] Always give VPN the INTERNET capability. Split-tunnel VPN (which are the only ones affected by this change) always fall through to the default network for routes they don't handle, and even if the underlying network(s) don't provide access this may be a pinhole that can actually reach the broader network. In practice this behaves like the original release of P and is the safest thing to do for Q. In R we should evaluate giving the VPN app the ability to simply tell the network stack whether it does provide Internet access or not. Bug: 119216095 Test: FrameworksNetTests NetworkStackTests Change-Id: I262ca41fe0225660551c9a421562405366b6acac --- .../com/android/server/connectivity/Vpn.java | 27 ++---- .../android/server/connectivity/VpnTest.java | 90 ------------------- 2 files changed, 9 insertions(+), 108 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index f5de2ad886b87..6cac49d2a90b3 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -953,30 +953,21 @@ public class Vpn { return false; } - LinkProperties lp = makeLinkProperties(); - final boolean hadInternetCapability = mNetworkCapabilities.hasCapability( - NetworkCapabilities.NET_CAPABILITY_INTERNET); - final boolean willHaveInternetCapability = providesRoutesToMostDestinations(lp); - if (hadInternetCapability != willHaveInternetCapability) { - // A seamless handover would have led to a change to INTERNET capability, which - // is supposed to be immutable for a given network. In this case bail out and do not - // perform handover. - Log.i(TAG, "Handover not possible due to changes to INTERNET capability"); - return false; - } - - agent.sendLinkProperties(lp); + agent.sendLinkProperties(makeLinkProperties()); return true; } private void agentConnect() { LinkProperties lp = makeLinkProperties(); - if (providesRoutesToMostDestinations(lp)) { - mNetworkCapabilities.addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET); - } else { - mNetworkCapabilities.removeCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET); - } + // VPN either provide a default route (IPv4 or IPv6 or both), or they are a split tunnel + // that falls back to the default network, which by definition provides INTERNET (unless + // there is no default network, in which case none of this matters in any sense). + // Also, it guarantees that when a VPN applies to an app, the VPN will always be reported + // as the network by getDefaultNetwork and registerDefaultNetworkCallback. This in turn + // protects the invariant that apps calling CM#bindProcessToNetwork(getDefaultNetwork()) + // the same as if they use the default network. + mNetworkCapabilities.addCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET); mNetworkInfo.setDetailedState(DetailedState.CONNECTING, null, null); diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index 2cae2509026cb..ce50bef53d756 100644 --- a/tests/net/java/com/android/server/connectivity/VpnTest.java +++ b/tests/net/java/com/android/server/connectivity/VpnTest.java @@ -727,94 +727,4 @@ public class VpnTest { "::/1", "8000::/2", "c000::/3", "e000::/4", "f000::/5", "f800::/6", "fe00::/8", "2605:ef80:e:af1d::/64"); } - - @Test - public void testProvidesRoutesToMostDestinations() { - final LinkProperties lp = new LinkProperties(); - - // Default route provides routes to all IPv4 destinations. - lp.addRoute(new RouteInfo(new IpPrefix("0.0.0.0/0"))); - assertTrue(Vpn.providesRoutesToMostDestinations(lp)); - - // Empty LP provides routes to no destination - lp.clear(); - assertFalse(Vpn.providesRoutesToMostDestinations(lp)); - - // All IPv4 routes except for local networks. This is the case most relevant - // to this function. It provides routes to almost the entire space. - // (clone the stream so that we can reuse it later) - publicIpV4Routes().forEach(s -> lp.addRoute(new RouteInfo(new IpPrefix(s)))); - assertTrue(Vpn.providesRoutesToMostDestinations(lp)); - - // Removing a 16-bit prefix, which is 65536 addresses. This is still enough to - // provide routes to "most" destinations. - lp.removeRoute(new RouteInfo(new IpPrefix("192.169.0.0/16"))); - assertTrue(Vpn.providesRoutesToMostDestinations(lp)); - - // Remove the /2 route, which represent a quarter of the available routing space. - // This LP does not provides routes to "most" destinations any more. - lp.removeRoute(new RouteInfo(new IpPrefix("64.0.0.0/2"))); - assertFalse(Vpn.providesRoutesToMostDestinations(lp)); - - lp.clear(); - publicIpV6Routes().forEach(s -> lp.addRoute(new RouteInfo(new IpPrefix(s)))); - assertTrue(Vpn.providesRoutesToMostDestinations(lp)); - - lp.removeRoute(new RouteInfo(new IpPrefix("::/1"))); - assertFalse(Vpn.providesRoutesToMostDestinations(lp)); - - // V6 does not provide sufficient coverage but v4 does - publicIpV4Routes().forEach(s -> lp.addRoute(new RouteInfo(new IpPrefix(s)))); - assertTrue(Vpn.providesRoutesToMostDestinations(lp)); - - // V4 still does - lp.removeRoute(new RouteInfo(new IpPrefix("192.169.0.0/16"))); - assertTrue(Vpn.providesRoutesToMostDestinations(lp)); - - // V4 does not any more - lp.removeRoute(new RouteInfo(new IpPrefix("64.0.0.0/2"))); - assertFalse(Vpn.providesRoutesToMostDestinations(lp)); - - // V4 does not, but V6 has sufficient coverage again - lp.addRoute(new RouteInfo(new IpPrefix("::/1"))); - assertTrue(Vpn.providesRoutesToMostDestinations(lp)); - - lp.clear(); - // V4-unreachable route should not be treated as sufficient coverage - lp.addRoute(new RouteInfo(new IpPrefix(Inet4Address.ANY, 0), RTN_UNREACHABLE)); - assertFalse(Vpn.providesRoutesToMostDestinations(lp)); - - lp.clear(); - // V6-unreachable route should not be treated as sufficient coverage - lp.addRoute(new RouteInfo(new IpPrefix(Inet6Address.ANY, 0), RTN_UNREACHABLE)); - assertFalse(Vpn.providesRoutesToMostDestinations(lp)); - } - - @Test - public void testDoesNotLockUpWithTooManyRoutes() { - final LinkProperties lp = new LinkProperties(); - final byte[] ad = new byte[4]; - // Actually evaluating this many routes under 1500ms is impossible on - // current hardware and for some time, as the algorithm is O(n²). - // Make sure the system has a safeguard against this and does not - // lock up. - final int MAX_ROUTES = 4000; - final long MAX_ALLOWED_TIME_MS = 1500; - for (int i = 0; i < MAX_ROUTES; ++i) { - ad[0] = (byte)((i >> 24) & 0xFF); - ad[1] = (byte)((i >> 16) & 0xFF); - ad[2] = (byte)((i >> 8) & 0xFF); - ad[3] = (byte)(i & 0xFF); - try { - lp.addRoute(new RouteInfo(new IpPrefix(Inet4Address.getByAddress(ad), 32))); - } catch (UnknownHostException e) { - // UnknownHostException is only thrown for an address of illegal length, - // which can't happen in the case above. - } - } - final long start = SystemClock.currentThreadTimeMillis(); - assertTrue(Vpn.providesRoutesToMostDestinations(lp)); - final long end = SystemClock.currentThreadTimeMillis(); - assertTrue(end - start < MAX_ALLOWED_TIME_MS); - } }