From d82c1ec1104aa906af42fe223e7b14b7dcf95120 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 17 May 2021 20:31:21 +0900 Subject: [PATCH] Allow unprivileged NetworkCallbacks to see other UIDs' networks. Currently, unprivileged apps can call getAllNetworks() to see all networks on the system, even networks that do not apply to them. Allow them to do this via NetworkCallbacks as well. This is the last piece of information that was only available through getAllNetworks, so this CL deprecates that API. Bug: 187921303 Test: new unit tests Test: CTS test in other CL in topic Change-Id: I30f1021927d3c8eae6525116c61ff4a4acecff6d --- framework/api/current.txt | 3 +- .../src/android/net/ConnectivityManager.java | 8 ++ framework/src/android/net/NetworkRequest.java | 35 +++++- .../android/server/ConnectivityService.java | 18 ++- .../server/ConnectivityServiceTest.java | 118 ++++++++++++++++++ 5 files changed, 175 insertions(+), 7 deletions(-) diff --git a/framework/api/current.txt b/framework/api/current.txt index 13ecb1236d..33f4d14148 100644 --- a/framework/api/current.txt +++ b/framework/api/current.txt @@ -70,7 +70,7 @@ package android.net { method @Nullable @RequiresPermission(android.Manifest.permission.ACCESS_NETWORK_STATE) public android.net.Network getActiveNetwork(); method @Deprecated @Nullable @RequiresPermission(android.Manifest.permission.ACCESS_NETWORK_STATE) public android.net.NetworkInfo getActiveNetworkInfo(); method @Deprecated @NonNull @RequiresPermission(android.Manifest.permission.ACCESS_NETWORK_STATE) public android.net.NetworkInfo[] getAllNetworkInfo(); - method @NonNull @RequiresPermission(android.Manifest.permission.ACCESS_NETWORK_STATE) public android.net.Network[] getAllNetworks(); + method @Deprecated @NonNull @RequiresPermission(android.Manifest.permission.ACCESS_NETWORK_STATE) public android.net.Network[] getAllNetworks(); method @Deprecated public boolean getBackgroundDataSetting(); method @Nullable public android.net.Network getBoundNetworkForProcess(); method public int getConnectionOwnerUid(int, @NonNull java.net.InetSocketAddress, @NonNull java.net.InetSocketAddress); @@ -406,6 +406,7 @@ package android.net { method @NonNull public android.net.NetworkRequest.Builder clearCapabilities(); method public android.net.NetworkRequest.Builder removeCapability(int); method public android.net.NetworkRequest.Builder removeTransportType(int); + method @NonNull public android.net.NetworkRequest.Builder setIncludeOtherUidNetworks(boolean); method @Deprecated public android.net.NetworkRequest.Builder setNetworkSpecifier(String); method public android.net.NetworkRequest.Builder setNetworkSpecifier(android.net.NetworkSpecifier); } diff --git a/framework/src/android/net/ConnectivityManager.java b/framework/src/android/net/ConnectivityManager.java index 5a53af44f5..93455bc800 100644 --- a/framework/src/android/net/ConnectivityManager.java +++ b/framework/src/android/net/ConnectivityManager.java @@ -1433,10 +1433,18 @@ public class ConnectivityManager { * Returns an array of all {@link Network} currently tracked by the * framework. * + * @deprecated This method does not provide any notification of network state changes, forcing + * apps to call it repeatedly. This is inefficient and prone to race conditions. + * Apps should use methods such as + * {@link #registerNetworkCallback(NetworkRequest, NetworkCallback)} instead. + * Apps that desire to obtain information about networks that do not apply to them + * can use {@link NetworkRequest.Builder#setIncludeOtherUidNetworks}. + * * @return an array of {@link Network} objects. */ @RequiresPermission(android.Manifest.permission.ACCESS_NETWORK_STATE) @NonNull + @Deprecated public Network[] getAllNetworks() { try { return mService.getAllNetworks(); diff --git a/framework/src/android/net/NetworkRequest.java b/framework/src/android/net/NetworkRequest.java index a384109e23..a71dbbf5f7 100644 --- a/framework/src/android/net/NetworkRequest.java +++ b/framework/src/android/net/NetworkRequest.java @@ -287,10 +287,15 @@ public class NetworkRequest implements Parcelable { } /** - * Set the watched UIDs for this request. This will be reset and wiped out unless - * the calling app holds the CHANGE_NETWORK_STATE permission. + * Sets this request to match only networks that apply to the specified UIDs. * - * @param uids The watched UIDs as a set of {@code Range}, or null for everything. + * By default, the set of UIDs is the UID of the calling app, and this request will match + * any network that applies to the app. Setting it to {@code null} will observe any + * network on the system, even if it does not apply to this app. In this case, any + * {@link NetworkSpecifier} set on this request will be redacted or removed to prevent the + * application deducing restricted information such as location. + * + * @param uids The UIDs as a set of {@code Range}, or null for everything. * @return The builder to facilitate chaining. * @hide */ @@ -517,6 +522,30 @@ public class NetworkRequest implements Parcelable { mNetworkCapabilities.setSubscriptionIds(subIds); return this; } + + /** + * Specifies whether the built request should also match networks that do not apply to the + * calling UID. + * + * By default, the built request will only match networks that apply to the calling UID. + * If this method is called with {@code true}, the built request will match any network on + * the system that matches the other parameters of the request. In this case, any + * information in the built request that is subject to redaction for security or privacy + * purposes, such as a {@link NetworkSpecifier}, will be redacted or removed to prevent the + * application deducing sensitive information. + * + * @param include Whether to match networks that do not apply to the calling UID. + * @return The builder to facilitate chaining. + */ + @NonNull + public Builder setIncludeOtherUidNetworks(boolean include) { + if (include) { + mNetworkCapabilities.setUids(null); + } else { + mNetworkCapabilities.setSingleUid(Process.myUid()); + } + return this; + } } // implement the Parcelable interface diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 197226af61..aa0f0582a0 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -2156,10 +2156,22 @@ public class ConnectivityService extends IConnectivityManager.Stub private void restrictRequestUidsForCallerAndSetRequestorInfo(NetworkCapabilities nc, int callerUid, String callerPackageName) { + // There is no need to track the effective UID of the request here. If the caller + // lacks the settings permission, the effective UID is the same as the calling ID. if (!checkSettingsPermission()) { - // There is no need to track the effective UID of the request here. If the caller lacks - // the settings permission, the effective UID is the same as the calling ID. - nc.setSingleUid(callerUid); + // Unprivileged apps can only pass in null or their own UID. + if (nc.getUids() == null) { + // If the caller passes in null, the callback will also match networks that do not + // apply to its UID, similarly to what it would see if it called getAllNetworks. + // In this case, redact everything in the request immediately. This ensures that the + // app is not able to get any redacted information by filing an unredacted request + // and observing whether the request matches something. + if (nc.getNetworkSpecifier() != null) { + nc.setNetworkSpecifier(nc.getNetworkSpecifier().redact()); + } + } else { + nc.setSingleUid(callerUid); + } } nc.setRequestorUidAndPackageName(callerUid, callerPackageName); nc.setAdministratorUids(new int[0]); diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 97ff3aff9b..29a411e2c9 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -4277,6 +4277,124 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(callback); } + @Test + public void testNetworkCallbackWithNullUids() throws Exception { + final NetworkRequest request = new NetworkRequest.Builder() + .removeCapability(NET_CAPABILITY_NOT_VPN) + .build(); + final TestNetworkCallback callback = new TestNetworkCallback(); + mCm.registerNetworkCallback(request, callback); + + // Attempt to file a callback for networks applying to another UID. This does not actually + // work, because this code does not currently have permission to do so. The callback behaves + // exactly the same as the one registered just above. + final int otherUid = UserHandle.getUid(RESTRICTED_USER, VPN_UID); + final NetworkRequest otherUidRequest = new NetworkRequest.Builder() + .removeCapability(NET_CAPABILITY_NOT_VPN) + .setUids(UidRange.toIntRanges(uidRangesForUids(otherUid))) + .build(); + final TestNetworkCallback otherUidCallback = new TestNetworkCallback(); + mCm.registerNetworkCallback(otherUidRequest, otherUidCallback); + + final NetworkRequest includeOtherUidsRequest = new NetworkRequest.Builder() + .removeCapability(NET_CAPABILITY_NOT_VPN) + .setIncludeOtherUidNetworks(true) + .build(); + final TestNetworkCallback includeOtherUidsCallback = new TestNetworkCallback(); + mCm.registerNetworkCallback(includeOtherUidsRequest, includeOtherUidsCallback); + + // Both callbacks see a network with no specifier that applies to their UID. + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); + mWiFiNetworkAgent.connect(false /* validated */); + callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + otherUidCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + includeOtherUidsCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + mWiFiNetworkAgent.disconnect(); + callback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); + otherUidCallback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); + includeOtherUidsCallback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); + + // Only the includeOtherUidsCallback sees a VPN that does not apply to its UID. + final UidRange range = UidRange.createForUser(UserHandle.of(RESTRICTED_USER)); + final Set vpnRanges = Collections.singleton(range); + mMockVpn.establish(new LinkProperties(), VPN_UID, vpnRanges); + includeOtherUidsCallback.expectAvailableThenValidatedCallbacks(mMockVpn); + callback.assertNoCallback(); + otherUidCallback.assertNoCallback(); + + mMockVpn.disconnect(); + includeOtherUidsCallback.expectCallback(CallbackEntry.LOST, mMockVpn); + callback.assertNoCallback(); + otherUidCallback.assertNoCallback(); + } + + private static class RedactableNetworkSpecifier extends NetworkSpecifier { + public static final int ID_INVALID = -1; + + public final int networkId; + + RedactableNetworkSpecifier(int networkId) { + this.networkId = networkId; + } + + @Override + public boolean canBeSatisfiedBy(NetworkSpecifier other) { + return other instanceof RedactableNetworkSpecifier + && this.networkId == ((RedactableNetworkSpecifier) other).networkId; + } + + @Override + public NetworkSpecifier redact() { + return new RedactableNetworkSpecifier(ID_INVALID); + } + } + + @Test + public void testNetworkCallbackWithNullUidsRedactsSpecifier() throws Exception { + final RedactableNetworkSpecifier specifier = new RedactableNetworkSpecifier(42); + final NetworkRequest request = new NetworkRequest.Builder() + .addCapability(NET_CAPABILITY_INTERNET) + .addTransportType(TRANSPORT_WIFI) + .setNetworkSpecifier(specifier) + .build(); + final TestNetworkCallback callback = new TestNetworkCallback(); + mCm.registerNetworkCallback(request, callback); + + // Attempt to file a callback for networks applying to another UID. This does not actually + // work, because this code does not currently have permission to do so. The callback behaves + // exactly the same as the one registered just above. + final int otherUid = UserHandle.getUid(RESTRICTED_USER, VPN_UID); + final NetworkRequest otherUidRequest = new NetworkRequest.Builder() + .addCapability(NET_CAPABILITY_INTERNET) + .addTransportType(TRANSPORT_WIFI) + .setNetworkSpecifier(specifier) + .setUids(UidRange.toIntRanges(uidRangesForUids(otherUid))) + .build(); + final TestNetworkCallback otherUidCallback = new TestNetworkCallback(); + mCm.registerNetworkCallback(otherUidRequest, otherUidCallback); + + final NetworkRequest includeOtherUidsRequest = new NetworkRequest.Builder() + .addCapability(NET_CAPABILITY_INTERNET) + .addTransportType(TRANSPORT_WIFI) + .setNetworkSpecifier(specifier) + .setIncludeOtherUidNetworks(true) + .build(); + final TestNetworkCallback includeOtherUidsCallback = new TestNetworkCallback(); + mCm.registerNetworkCallback(includeOtherUidsRequest, callback); + + // Only the regular callback sees the network, because callbacks filed with no UID have + // their specifiers redacted. + final LinkProperties emptyLp = new LinkProperties(); + final NetworkCapabilities ncTemplate = new NetworkCapabilities() + .addTransportType(TRANSPORT_WIFI) + .setNetworkSpecifier(specifier); + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI, emptyLp, ncTemplate); + mWiFiNetworkAgent.connect(false /* validated */); + callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + otherUidCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); + includeOtherUidsCallback.assertNoCallback(); + } + private void setCaptivePortalMode(int mode) { ContentResolver cr = mServiceContext.getContentResolver(); Settings.Global.putInt(cr, ConnectivitySettingsManager.CAPTIVE_PORTAL_MODE, mode);