From 46fd589d896fc189d089e21d21fb28ff49bfcf26 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Fri, 27 Nov 2020 14:46:42 +0900 Subject: [PATCH 1/3] Observe mOwnerUID in NetworkCapabilities#equals. Currently, NetworkCapabilities's equals and hashCode methods ignore mOwnerUID. This is confusing because it is inconsistent with pretty much every other member of this class. Bug: 175188445 Test: atest CtsNetTestCases:NetworkAgentTest \ CtsNetTestCases:Ikev2VpnTest \ CtsNetTestCases:VpnServiceTest HostsideVpnTests \ CtsNetTestCases:android.net.cts.ConnectivityDiagnosticsManagerTest \ ConnectivityServiceTest com.android.server.connectivity.VpnTest Change-Id: I2348b7a35f32a931687f2d3c2fa57620a12fe06f --- .../java/android/net/NetworkCapabilities.java | 26 ++++++++++++------- .../server/ConnectivityServiceTest.java | 10 +++---- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/core/java/android/net/NetworkCapabilities.java b/core/java/android/net/NetworkCapabilities.java index 8dad11ffa7..1a37fb9fb6 100644 --- a/core/java/android/net/NetworkCapabilities.java +++ b/core/java/android/net/NetworkCapabilities.java @@ -975,6 +975,10 @@ public final class NetworkCapabilities implements Parcelable { return mOwnerUid; } + private boolean equalsOwnerUid(@NonNull final NetworkCapabilities nc) { + return mOwnerUid == nc.mOwnerUid; + } + /** * UIDs of packages that are administrators of this network, or empty if none. * @@ -1684,6 +1688,7 @@ public final class NetworkCapabilities implements Parcelable { && equalsTransportInfo(that) && equalsUids(that) && equalsSSID(that) + && equalsOwnerUid(that) && equalsPrivateDnsBroken(that) && equalsRequestor(that) && equalsAdministratorUids(that); @@ -1697,17 +1702,18 @@ public final class NetworkCapabilities implements Parcelable { + ((int) (mUnwantedNetworkCapabilities >> 32) * 7) + ((int) (mTransportTypes & 0xFFFFFFFF) * 11) + ((int) (mTransportTypes >> 32) * 13) - + (mLinkUpBandwidthKbps * 17) - + (mLinkDownBandwidthKbps * 19) + + mLinkUpBandwidthKbps * 17 + + mLinkDownBandwidthKbps * 19 + Objects.hashCode(mNetworkSpecifier) * 23 - + (mSignalStrength * 29) - + Objects.hashCode(mUids) * 31 - + Objects.hashCode(mSSID) * 37 - + Objects.hashCode(mTransportInfo) * 41 - + Objects.hashCode(mPrivateDnsBroken) * 43 - + Objects.hashCode(mRequestorUid) * 47 - + Objects.hashCode(mRequestorPackageName) * 53 - + Arrays.hashCode(mAdministratorUids) * 59; + + mSignalStrength * 29 + + mOwnerUid * 31 + + Objects.hashCode(mUids) * 37 + + Objects.hashCode(mSSID) * 41 + + Objects.hashCode(mTransportInfo) * 43 + + Objects.hashCode(mPrivateDnsBroken) * 47 + + Objects.hashCode(mRequestorUid) * 53 + + Objects.hashCode(mRequestorPackageName) * 59 + + Arrays.hashCode(mAdministratorUids) * 61; } @Override diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 34b26789b7..2aade42476 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -1980,18 +1980,16 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent.setNetworkCapabilities(agentCapabilities, true); waitForIdle(); - // Check that the owner UID is not updated. + // Check that the owner UID is updated. + // The owner UID is -1 because it is visible only to the UID that owns the network. NetworkCapabilities nc = mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()); - assertEquals(originalOwnerUid, nc.getOwnerUid()); + assertEquals(-1, nc.getOwnerUid()); - // Make an unrelated change to the capabilities. + // Make an unrelated change to the capabilities and check it. The owner UID remains -1. assertFalse(agentCapabilities.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); agentCapabilities.addCapability(NET_CAPABILITY_NOT_CONGESTED); mWiFiNetworkAgent.setNetworkCapabilities(agentCapabilities, true); waitForIdle(); - - // Check that both the capability change and the owner UID have been modified. - // The owner UID is -1 because it is visible only to the UID that owns the network. nc = mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()); assertEquals(-1, nc.getOwnerUid()); assertTrue(nc.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); From 93401340261848ab0545fdf85dc79b16b39277da Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Wed, 9 Dec 2020 18:30:52 +0900 Subject: [PATCH 2/3] Disallow NetworkAgents from changing the owner UID. The current behaviour with regards to changing the owner UID is bizarre and arguably incorrect. A NetworkAgent can change the owner to whatever other app it wants, regardless of signatures, at any time. This includes, for example, transferring ownership to another UID and then recovering it. Fortunately no existing NetworkAgent appears to do this: - ClientModeImpl sets it to the UID of the app that created the configuration. It doesn't look like it can change while the network is connected. - Vpn sets it to the UID of the VPN owner. That also can't change. - Telephony does not appear to set it at all, it only sets the administrator UIDs (and updates them whenever it gets EVENT_CARRIER_PRIVILEGED_UIDS_CHANGED). Disallow this now before code is written that depends on it. Bug: 175188445 Test: modified tests in ConnectivityServiceTest Change-Id: I638e29fda2481ec3bf4fff562ea66a73322881df --- .../android/server/ConnectivityService.java | 5 ++++ .../server/ConnectivityServiceTest.java | 28 ++++--------------- 2 files changed, 10 insertions(+), 23 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 65903af492..6b3e8a9718 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6355,6 +6355,11 @@ public class ConnectivityService extends IConnectivityManager.Stub */ private void processCapabilitiesFromAgent(NetworkAgentInfo nai, NetworkCapabilities nc) { nai.declaredMetered = !nc.hasCapability(NET_CAPABILITY_NOT_METERED); + if (nai.networkCapabilities.getOwnerUid() != nc.getOwnerUid()) { + Log.e(TAG, nai.toShortString() + ": ignoring attempt to change owner from " + + nai.networkCapabilities.getOwnerUid() + " to " + nc.getOwnerUid()); + nc.setOwnerUid(nai.networkCapabilities.getOwnerUid()); + } } /** Modifies |caps| based on the capabilities of the specified underlying networks. */ diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 2aade42476..a4b6e31182 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -1958,7 +1958,7 @@ public class ConnectivityServiceTest { } @Test - public void testOwnerUidChangeBug() throws Exception { + public void testOwnerUidCannotChange() throws Exception { // Owner UIDs are not visible without location permission. setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION, Manifest.permission.ACCESS_FINE_LOCATION); @@ -1973,37 +1973,19 @@ public class ConnectivityServiceTest { waitForIdle(); // Send ConnectivityService an update to the mWiFiNetworkAgent's capabilities that changes - // its owner UID. + // the owner UID and an unrelated capability. NetworkCapabilities agentCapabilities = mWiFiNetworkAgent.getNetworkCapabilities(); assertEquals(originalOwnerUid, agentCapabilities.getOwnerUid()); agentCapabilities.setOwnerUid(42); - mWiFiNetworkAgent.setNetworkCapabilities(agentCapabilities, true); - waitForIdle(); - - // Check that the owner UID is updated. - // The owner UID is -1 because it is visible only to the UID that owns the network. - NetworkCapabilities nc = mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()); - assertEquals(-1, nc.getOwnerUid()); - - // Make an unrelated change to the capabilities and check it. The owner UID remains -1. assertFalse(agentCapabilities.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); agentCapabilities.addCapability(NET_CAPABILITY_NOT_CONGESTED); mWiFiNetworkAgent.setNetworkCapabilities(agentCapabilities, true); waitForIdle(); - nc = mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()); - assertEquals(-1, nc.getOwnerUid()); - assertTrue(nc.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); - // Set the owner back to originalOwnerUid, update the capabilities, and check that it is - // visible again. - // TODO: should this even be possible? - agentCapabilities.setOwnerUid(originalOwnerUid); - agentCapabilities.removeCapability(NET_CAPABILITY_NOT_CONGESTED); - mWiFiNetworkAgent.setNetworkCapabilities(agentCapabilities, true); - waitForIdle(); - - nc = mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()); + // Check that the capability change has been applied but the owner UID is not modified. + NetworkCapabilities nc = mCm.getNetworkCapabilities(mWiFiNetworkAgent.getNetwork()); assertEquals(originalOwnerUid, nc.getOwnerUid()); + assertTrue(nc.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); } @Test From e4d1e526e17aead8b2a02a06ea9582dd5d38636d Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 10 Dec 2020 00:32:04 +0900 Subject: [PATCH 3/3] Add a convenience method to update a network's capabilities. Almost all calls to ConnectivityService#updateCapabilities use all the current data in the network, and thus call the method like this: updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); Introduce a convenience method to simplify this frequent use case. Bug: 173331190 Test: passes existing ConnectivityService tests Change-Id: I6eb6d92bd159f2575d10a929bd59f6dd1b7a4b4e --- .../android/server/ConnectivityService.java | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 6b3e8a9718..16cff7bb7f 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -2832,7 +2832,7 @@ public class ConnectivityService extends IConnectivityManager.Stub log(nai.toShortString() + " changed underlying networks to " + Arrays.toString(nai.declaredUnderlyingNetworks)); } - updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); + updateCapabilitiesForNetwork(nai); notifyIfacesChangedForNetworkStats(); } } @@ -2856,8 +2856,7 @@ public class ConnectivityService extends IConnectivityManager.Stub if (probePrivateDnsCompleted) { if (nai.networkCapabilities.isPrivateDnsBroken() != privateDnsBroken) { nai.networkCapabilities.setPrivateDnsBroken(privateDnsBroken); - final int oldScore = nai.getCurrentScore(); - updateCapabilities(oldScore, nai, nai.networkCapabilities); + updateCapabilitiesForNetwork(nai); } // Only show the notification when the private DNS is broken and the // PRIVATE_DNS_BROKEN notification hasn't shown since last valid. @@ -2872,8 +2871,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // done yet. In either case, the networkCapabilities should be updated to // reflect the new status. nai.networkCapabilities.setPrivateDnsBroken(false); - final int oldScore = nai.getCurrentScore(); - updateCapabilities(oldScore, nai, nai.networkCapabilities); + updateCapabilitiesForNetwork(nai); nai.networkAgentConfig.hasShownBroken = false; } break; @@ -2894,7 +2892,6 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkAgentInfo nai = getNetworkAgentInfoForNetId(netId); // If captive portal status has changed, update capabilities or disconnect. if (nai != null && (visible != nai.lastCaptivePortalDetected)) { - final int oldScore = nai.getCurrentScore(); nai.lastCaptivePortalDetected = visible; nai.everCaptivePortalDetected |= visible; if (nai.lastCaptivePortalDetected && @@ -2905,7 +2902,7 @@ public class ConnectivityService extends IConnectivityManager.Stub teardownUnneededNetwork(nai); break; } - updateCapabilities(oldScore, nai, nai.networkCapabilities); + updateCapabilitiesForNetwork(nai); } if (!visible) { // Only clear SIGN_IN and NETWORK_SWITCH notifications here, or else other @@ -2989,7 +2986,7 @@ public class ConnectivityService extends IConnectivityManager.Stub nai.networkAgentConfig.hasShownBroken = false; } } else if (partialConnectivityChanged) { - updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); + updateCapabilitiesForNetwork(nai); } updateInetCondition(nai); // Let the NetworkAgent know the state of its network @@ -3657,7 +3654,7 @@ public class ConnectivityService extends IConnectivityManager.Stub nri.mSatisfier = null; if (!wasBackgroundNetwork && nai.isBackgroundNetwork()) { // Went from foreground to background. - updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); + updateCapabilitiesForNetwork(nai); } } @@ -4817,7 +4814,7 @@ public class ConnectivityService extends IConnectivityManager.Stub ensureRunningOnConnectivityServiceThread(); for (NetworkAgentInfo nai : mNetworkAgentInfos.values()) { if (nai.supportsUnderlyingNetworks()) { - updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); + updateCapabilitiesForNetwork(nai); } } } @@ -6573,6 +6570,11 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + /** Convenience method to update the capabilities for a given network. */ + private void updateCapabilitiesForNetwork(NetworkAgentInfo nai) { + updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); + } + /** * Returns whether VPN isolation (ingress interface filtering) should be applied on the given * network. @@ -6858,8 +6860,7 @@ public class ConnectivityService extends IConnectivityManager.Stub teardownUnneededNetwork(oldNetwork); } else { // Put the network in the background. - updateCapabilities(oldNetwork.getCurrentScore(), oldNetwork, - oldNetwork.networkCapabilities); + updateCapabilitiesForNetwork(oldNetwork); } }