From b00152243319ebd18cb61682bfbac0e207599554 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Mon, 15 Jun 2020 17:54:29 +0000 Subject: [PATCH 1/2] Set correct owner UID for VPN agentConnect() This commit changes agentConnect to set the owner UID as the mOwnerUid field instead of the Binder.getCallingUid(). Binder.getCallingUid() can return incorrect results for platform VPNs, as agentConnect() is called under a clean calling UID. Additionally, this relaxes the ownerUid sanitization check to allow a VPN network's owner to see it's own ownership information. Vpn.mOwnerUid is guaranteed to be correct, as all VPNs MUST have called prepareInternal() at some previous point, which sets mOwnerUid as the package's UID (or SYSTEM_UID if this is legacy VPN). Bug: 150135470 Test: CTS tests showing ownership information Merged-In: Ic979dad73983d722365849fbfb0becfd432b894c Change-Id: Ic979dad73983d722365849fbfb0becfd432b894c (cherry picked from commit e29bf99a7fc1067c546d7fa6cbcb9001fb110d16) --- .../java/android/net/NetworkCapabilities.java | 27 +++++++++++++++---- .../android/server/ConnectivityService.java | 6 +++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/core/java/android/net/NetworkCapabilities.java b/core/java/android/net/NetworkCapabilities.java index a3fd60e9d3..004f84422b 100644 --- a/core/java/android/net/NetworkCapabilities.java +++ b/core/java/android/net/NetworkCapabilities.java @@ -900,9 +900,17 @@ public final class NetworkCapabilities implements Parcelable { *

For NetworkCapability instances being sent from ConnectivityService, this value MUST be * reset to Process.INVALID_UID unless all the following conditions are met: * + *

The caller is the network owner, AND one of the following sets of requirements is met: + * *

    - *
  1. The destination app is the network owner - *
  2. The destination app has the ACCESS_FINE_LOCATION permission granted + *
  3. The described Network is a VPN + *
+ * + *

OR: + * + *

    + *
  1. The calling app is the network owner + *
  2. The calling app has the ACCESS_FINE_LOCATION permission granted *
  3. The user's location toggle is on *
* @@ -928,7 +936,16 @@ public final class NetworkCapabilities implements Parcelable { /** * Retrieves the UID of the app that owns this network. * - *

For user privacy reasons, this field will only be populated if: + *

For user privacy reasons, this field will only be populated if the following conditions + * are met: + * + *

The caller is the network owner, AND one of the following sets of requirements is met: + * + *

    + *
  1. The described Network is a VPN + *
+ * + *

OR: * *

    *
  1. The calling app is the network owner @@ -936,8 +953,8 @@ public final class NetworkCapabilities implements Parcelable { *
  2. The user's location toggle is on *
* - * Instances of NetworkCapabilities sent to apps without the appropriate permissions will - * have this field cleared out. + * Instances of NetworkCapabilities sent to apps without the appropriate permissions will have + * this field cleared out. */ public int getOwnerUid() { return mOwnerUid; diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 2958fd2ae6..36ba610085 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -1698,6 +1698,12 @@ public class ConnectivityService extends IConnectivityManager.Stub return newNc; } + // Allow VPNs to see ownership of their own VPN networks - not location sensitive. + if (nc.hasTransport(TRANSPORT_VPN)) { + // Owner UIDs already checked above. No need to re-check. + return newNc; + } + Binder.withCleanCallingIdentity( () -> { if (!mLocationPermissionChecker.checkLocationPermission( From 58897cc491bf0fb24b8f3e49d1d80f64138f361a Mon Sep 17 00:00:00 2001 From: Treehugger Robot Date: Wed, 17 Jun 2020 13:21:06 +0000 Subject: [PATCH 2/2] Treat RouteInfo with different interfaces as different routes On Android different interfaces usually use different routing tables. As a result, a change in interface should not be treated as route update, but rather a remove and an add. This change fixes a bug in VPN seamless handover where routes failed to be updated when a new tunnel interface replaces the existing one within the same network. Bug: 158696878 Test: atest com.android.cts.net.HostsideVpnTests Test: atest NetworkStackTests Test: atest CtsNetTestCases Test: atest FrameworksNetTests Original-Change: https://android-review.googlesource.com/1331916 Merged-In: I57987233d42a0253eaee2e1ca5f28728c2354620 Change-Id: I57987233d42a0253eaee2e1ca5f28728c2354620 --- core/java/android/net/RouteInfo.java | 43 ++++++++++++++++--- .../java/android/net/LinkPropertiesTest.java | 18 ++++++++ .../java/android/net/RouteInfoTest.java | 42 +++++++++++++++++- 3 files changed, 95 insertions(+), 8 deletions(-) diff --git a/core/java/android/net/RouteInfo.java b/core/java/android/net/RouteInfo.java index e550f85e6b..9876076173 100644 --- a/core/java/android/net/RouteInfo.java +++ b/core/java/android/net/RouteInfo.java @@ -26,7 +26,6 @@ import android.net.util.NetUtils; import android.os.Build; import android.os.Parcel; import android.os.Parcelable; -import android.util.Pair; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -554,15 +553,45 @@ public final class RouteInfo implements Parcelable { } /** - * A helper class that contains the destination and the gateway in a {@code RouteInfo}, - * used by {@link ConnectivityService#updateRoutes} or + * A helper class that contains the destination, the gateway and the interface in a + * {@code RouteInfo}, used by {@link ConnectivityService#updateRoutes} or * {@link LinkProperties#addRoute} to calculate the list to be updated. + * {@code RouteInfo} objects with different interfaces are treated as different routes because + * *usually* on Android different interfaces use different routing tables, and moving a route + * to a new routing table never constitutes an update, but is always a remove and an add. * * @hide */ - public static class RouteKey extends Pair { - RouteKey(@NonNull IpPrefix destination, @Nullable InetAddress gateway) { - super(destination, gateway); + public static class RouteKey { + @NonNull private final IpPrefix mDestination; + @Nullable private final InetAddress mGateway; + @Nullable private final String mInterface; + + RouteKey(@NonNull IpPrefix destination, @Nullable InetAddress gateway, + @Nullable String iface) { + mDestination = destination; + mGateway = gateway; + mInterface = iface; + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof RouteKey)) { + return false; + } + RouteKey p = (RouteKey) o; + // No need to do anything special for scoped addresses. Inet6Address#equals does not + // consider the scope ID, but the netd route IPCs (e.g., INetd#networkAddRouteParcel) + // and the kernel ignore scoped addresses both in the prefix and in the nexthop and only + // look at RTA_OIF. + return Objects.equals(p.mDestination, mDestination) + && Objects.equals(p.mGateway, mGateway) + && Objects.equals(p.mInterface, mInterface); + } + + @Override + public int hashCode() { + return Objects.hash(mDestination, mGateway, mInterface); } } @@ -574,7 +603,7 @@ public final class RouteInfo implements Parcelable { */ @NonNull public RouteKey getRouteKey() { - return new RouteKey(mDestination, mGateway); + return new RouteKey(mDestination, mGateway, mInterface); } /** diff --git a/tests/net/common/java/android/net/LinkPropertiesTest.java b/tests/net/common/java/android/net/LinkPropertiesTest.java index 0fc9be32f4..6eba62e637 100644 --- a/tests/net/common/java/android/net/LinkPropertiesTest.java +++ b/tests/net/common/java/android/net/LinkPropertiesTest.java @@ -16,6 +16,8 @@ package android.net; +import static android.net.RouteInfo.RTN_THROW; +import static android.net.RouteInfo.RTN_UNICAST; import static android.net.RouteInfo.RTN_UNREACHABLE; import static com.android.testutils.ParcelUtilsKt.assertParcelSane; @@ -1282,4 +1284,20 @@ public class LinkPropertiesTest { assertTrue(lp.hasIpv6UnreachableDefaultRoute()); assertFalse(lp.hasIpv4UnreachableDefaultRoute()); } + + @Test @IgnoreUpTo(Build.VERSION_CODES.Q) + public void testRouteAddWithSameKey() throws Exception { + LinkProperties lp = new LinkProperties(); + lp.setInterfaceName("wlan0"); + final IpPrefix v6 = new IpPrefix("64:ff9b::/96"); + lp.addRoute(new RouteInfo(v6, address("fe80::1"), "wlan0", RTN_UNICAST, 1280)); + assertEquals(1, lp.getRoutes().size()); + lp.addRoute(new RouteInfo(v6, address("fe80::1"), "wlan0", RTN_UNICAST, 1500)); + assertEquals(1, lp.getRoutes().size()); + final IpPrefix v4 = new IpPrefix("192.0.2.128/25"); + lp.addRoute(new RouteInfo(v4, address("192.0.2.1"), "wlan0", RTN_UNICAST, 1460)); + assertEquals(2, lp.getRoutes().size()); + lp.addRoute(new RouteInfo(v4, address("192.0.2.1"), "wlan0", RTN_THROW, 1460)); + assertEquals(2, lp.getRoutes().size()); + } } diff --git a/tests/net/common/java/android/net/RouteInfoTest.java b/tests/net/common/java/android/net/RouteInfoTest.java index 8204b494bb..60cac0b6b0 100644 --- a/tests/net/common/java/android/net/RouteInfoTest.java +++ b/tests/net/common/java/android/net/RouteInfoTest.java @@ -25,6 +25,7 @@ import static com.android.testutils.ParcelUtilsKt.assertParcelingIsLossless; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -56,7 +57,7 @@ public class RouteInfoTest { private static final int INVALID_ROUTE_TYPE = -1; private InetAddress Address(String addr) { - return InetAddress.parseNumericAddress(addr); + return InetAddresses.parseNumericAddress(addr); } private IpPrefix Prefix(String prefix) { @@ -391,4 +392,43 @@ public class RouteInfoTest { r = new RouteInfo(Prefix("0.0.0.0/0"), Address("0.0.0.0"), "wlan0"); assertEquals(0, r.getMtu()); } + + @Test @IgnoreUpTo(Build.VERSION_CODES.Q) + public void testRouteKey() { + RouteInfo.RouteKey k1, k2; + // Only prefix, null gateway and null interface + k1 = new RouteInfo(Prefix("2001:db8::/128"), null).getRouteKey(); + k2 = new RouteInfo(Prefix("2001:db8::/128"), null).getRouteKey(); + assertEquals(k1, k2); + assertEquals(k1.hashCode(), k2.hashCode()); + + // With prefix, gateway and interface. Type and MTU does not affect RouteKey equality + k1 = new RouteInfo(Prefix("192.0.2.0/24"), Address("192.0.2.1"), "wlan0", + RTN_UNREACHABLE, 1450).getRouteKey(); + k2 = new RouteInfo(Prefix("192.0.2.0/24"), Address("192.0.2.1"), "wlan0", + RouteInfo.RTN_UNICAST, 1400).getRouteKey(); + assertEquals(k1, k2); + assertEquals(k1.hashCode(), k2.hashCode()); + + // Different scope IDs are ignored by the kernel, so we consider them equal here too. + k1 = new RouteInfo(Prefix("2001:db8::/64"), Address("fe80::1%1"), "wlan0").getRouteKey(); + k2 = new RouteInfo(Prefix("2001:db8::/64"), Address("fe80::1%2"), "wlan0").getRouteKey(); + assertEquals(k1, k2); + assertEquals(k1.hashCode(), k2.hashCode()); + + // Different prefix + k1 = new RouteInfo(Prefix("192.0.2.0/24"), null).getRouteKey(); + k2 = new RouteInfo(Prefix("192.0.3.0/24"), null).getRouteKey(); + assertNotEquals(k1, k2); + + // Different gateway + k1 = new RouteInfo(Prefix("ff02::1/128"), Address("2001:db8::1"), null).getRouteKey(); + k2 = new RouteInfo(Prefix("ff02::1/128"), Address("2001:db8::2"), null).getRouteKey(); + assertNotEquals(k1, k2); + + // Different interface + k1 = new RouteInfo(Prefix("ff02::1/128"), null, "tun0").getRouteKey(); + k2 = new RouteInfo(Prefix("ff02::1/128"), null, "tun1").getRouteKey(); + assertNotEquals(k1, k2); + } }