Give apps with NETWORK_SETTINGS right to see any VPN.
...not only the ones that apply to them.
Bug: 73217368
Test: runtest frameworks-net and CTS
Change-Id: I436972a3e51e98bdd815771b451bcedadf684763
diff --git a/core/java/android/net/NetworkCapabilities.java b/core/java/android/net/NetworkCapabilities.java
index d8e1741..b899cbf 100644
--- a/core/java/android/net/NetworkCapabilities.java
+++ b/core/java/android/net/NetworkCapabilities.java
@@ -890,7 +890,16 @@
/**
* List of UIDs this network applies to. No restriction if null.
* <p>
- * This is typically (and at this time, only) used by VPN. This network is only available to
+ * For networks, mUids represent the list of network this applies to, and null means this
+ * network applies to all UIDs.
+ * For requests, mUids is the list of UIDs this network MUST apply to to match ; ALL UIDs
+ * must be included in a network so that they match. As an exception to the general rule,
+ * a null mUids field for requests mean "no requirements" rather than what the general rule
+ * would suggest ("must apply to all UIDs") : this is because this has shown to be what users
+ * of this API expect in practice. A network that must match all UIDs can still be
+ * expressed with a set ranging the entire set of possible UIDs.
+ * <p>
+ * mUids is typically (and at this time, only) used by VPN. This network is only available to
* the UIDs in this list, and it is their default network. Apps in this list that wish to
* bypass the VPN can do so iff the VPN app allows them to or if they are privileged. If this
* member is null, then the network is not restricted by app UID. If it's an empty list, then
@@ -1012,8 +1021,7 @@
* @hide
*/
public boolean satisfiedByUids(NetworkCapabilities nc) {
- if (null == nc.mUids) return true; // The network satisfies everything.
- if (null == mUids) return false; // Not everything allowed but requires everything
+ if (null == nc.mUids || null == mUids) return true; // The network satisfies everything.
for (UidRange requiredRange : mUids) {
if (requiredRange.contains(nc.mEstablishingVpnAppUid)) return true;
if (!nc.appliesToUidRange(requiredRange)) {
diff --git a/core/java/android/net/NetworkRequest.java b/core/java/android/net/NetworkRequest.java
index 97ded2d..f1dfbd1 100644
--- a/core/java/android/net/NetworkRequest.java
+++ b/core/java/android/net/NetworkRequest.java
@@ -19,6 +19,7 @@
import android.annotation.NonNull;
import android.os.Parcel;
import android.os.Parcelable;
+import android.os.Process;
import android.text.TextUtils;
import java.util.Objects;
@@ -131,12 +132,18 @@
* needed in terms of {@link NetworkCapabilities} features
*/
public static class Builder {
- private final NetworkCapabilities mNetworkCapabilities = new NetworkCapabilities();
+ private final NetworkCapabilities mNetworkCapabilities;
/**
* Default constructor for Builder.
*/
- public Builder() {}
+ public Builder() {
+ // By default, restrict this request to networks available to this app.
+ // Apps can rescind this restriction, but ConnectivityService will enforce
+ // it for apps that do not have the NETWORK_SETTINGS permission.
+ mNetworkCapabilities = new NetworkCapabilities();
+ mNetworkCapabilities.setSingleUid(Process.myUid());
+ }
/**
* Build {@link NetworkRequest} give the current set of capabilities.
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index fd2ef18..3021e6a 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -17,6 +17,7 @@
package com.android.server;
import static android.Manifest.permission.RECEIVE_DATA_ACTIVITY_CHANGE;
+import static android.content.pm.PackageManager.PERMISSION_GRANTED;
import static android.net.ConnectivityManager.CONNECTIVITY_ACTION;
import static android.net.ConnectivityManager.NETID_UNSET;
import static android.net.ConnectivityManager.TYPE_ETHERNET;
@@ -1329,9 +1330,8 @@
if (nai != null) {
synchronized (nai) {
if (nai.networkCapabilities != null) {
- // TODO : don't remove the UIDs when communicating with processes
- // that have the NETWORK_SETTINGS permission.
- return networkCapabilitiesWithoutUids(nai.networkCapabilities);
+ return networkCapabilitiesWithoutUidsUnlessAllowed(nai.networkCapabilities,
+ Binder.getCallingPid(), Binder.getCallingUid());
}
}
}
@@ -1344,10 +1344,18 @@
return getNetworkCapabilitiesInternal(getNetworkAgentInfoForNetwork(network));
}
- private NetworkCapabilities networkCapabilitiesWithoutUids(NetworkCapabilities nc) {
+ private NetworkCapabilities networkCapabilitiesWithoutUidsUnlessAllowed(
+ NetworkCapabilities nc, int callerPid, int callerUid) {
+ if (checkSettingsPermission(callerPid, callerUid)) return new NetworkCapabilities(nc);
return new NetworkCapabilities(nc).setUids(null);
}
+ private void restrictRequestUidsForCaller(NetworkCapabilities nc) {
+ if (!checkSettingsPermission()) {
+ nc.setSingleUid(Binder.getCallingUid());
+ }
+ }
+
@Override
public NetworkState[] getAllNetworkState() {
// Require internal since we're handing out IMSI details
@@ -1546,6 +1554,16 @@
"ConnectivityService");
}
+ private boolean checkSettingsPermission() {
+ return PERMISSION_GRANTED == mContext.checkCallingOrSelfPermission(
+ android.Manifest.permission.NETWORK_SETTINGS);
+ }
+
+ private boolean checkSettingsPermission(int pid, int uid) {
+ return PERMISSION_GRANTED == mContext.checkPermission(
+ android.Manifest.permission.NETWORK_SETTINGS, pid, uid);
+ }
+
private void enforceTetherAccessPermission() {
mContext.enforceCallingOrSelfPermission(
android.Manifest.permission.ACCESS_NETWORK_STATE,
@@ -4213,13 +4231,12 @@
enforceMeteredApnPolicy(networkCapabilities);
}
ensureRequestableCapabilities(networkCapabilities);
- // Set the UID range for this request to the single UID of the requester.
+ // Set the UID range for this request to the single UID of the requester, or to an empty
+ // set of UIDs if the caller has the appropriate permission and UIDs have not been set.
// This will overwrite any allowed UIDs in the requested capabilities. Though there
// are no visible methods to set the UIDs, an app could use reflection to try and get
// networks for other apps so it's essential that the UIDs are overwritten.
- // TODO : don't forcefully set the UID when communicating with processes
- // that have the NETWORK_SETTINGS permission.
- networkCapabilities.setSingleUid(Binder.getCallingUid());
+ restrictRequestUidsForCaller(networkCapabilities);
if (timeoutMs < 0) {
throw new IllegalArgumentException("Bad timeout specified");
@@ -4293,9 +4310,7 @@
enforceMeteredApnPolicy(networkCapabilities);
ensureRequestableCapabilities(networkCapabilities);
ensureValidNetworkSpecifier(networkCapabilities);
- // TODO : don't forcefully set the UID when communicating with processes
- // that have the NETWORK_SETTINGS permission.
- networkCapabilities.setSingleUid(Binder.getCallingUid());
+ restrictRequestUidsForCaller(networkCapabilities);
NetworkRequest networkRequest = new NetworkRequest(networkCapabilities, TYPE_NONE,
nextNetworkRequestId(), NetworkRequest.Type.REQUEST);
@@ -4349,9 +4364,7 @@
}
NetworkCapabilities nc = new NetworkCapabilities(networkCapabilities);
- // TODO : don't forcefully set the UIDs when communicating with processes
- // that have the NETWORK_SETTINGS permission.
- nc.setSingleUid(Binder.getCallingUid());
+ restrictRequestUidsForCaller(nc);
if (!ConnectivityManager.checkChangePermission(mContext)) {
// Apps without the CHANGE_NETWORK_STATE permission can't use background networks, so
// make all their listens include NET_CAPABILITY_FOREGROUND. That way, they will get
@@ -4381,9 +4394,7 @@
ensureValidNetworkSpecifier(networkCapabilities);
final NetworkCapabilities nc = new NetworkCapabilities(networkCapabilities);
- // TODO : don't forcefully set the UIDs when communicating with processes
- // that have the NETWORK_SETTINGS permission.
- nc.setSingleUid(Binder.getCallingUid());
+ restrictRequestUidsForCaller(nc);
NetworkRequest networkRequest = new NetworkRequest(nc, TYPE_NONE, nextNetworkRequestId(),
NetworkRequest.Type.LISTEN);
@@ -4947,8 +4958,8 @@
}
case ConnectivityManager.CALLBACK_CAP_CHANGED: {
// networkAgent can't be null as it has been accessed a few lines above.
- final NetworkCapabilities nc =
- networkCapabilitiesWithoutUids(networkAgent.networkCapabilities);
+ final NetworkCapabilities nc = networkCapabilitiesWithoutUidsUnlessAllowed(
+ networkAgent.networkCapabilities, nri.mPid, nri.mUid);
putParcelable(bundle, nc);
break;
}
diff --git a/tests/net/java/android/net/NetworkCapabilitiesTest.java b/tests/net/java/android/net/NetworkCapabilitiesTest.java
index 2414a8e..b32f0fd 100644
--- a/tests/net/java/android/net/NetworkCapabilitiesTest.java
+++ b/tests/net/java/android/net/NetworkCapabilitiesTest.java
@@ -223,7 +223,9 @@
assertFalse(netCap.appliesToUidRange(new UidRange(60, 3400)));
NetworkCapabilities netCap2 = new NetworkCapabilities();
- assertFalse(netCap2.satisfiedByUids(netCap));
+ // A new netcap object has null UIDs, so anything will satisfy it.
+ assertTrue(netCap2.satisfiedByUids(netCap));
+ // Still not equal though.
assertFalse(netCap2.equalsUids(netCap));
netCap2.setUids(uids);
assertTrue(netCap2.satisfiedByUids(netCap));
@@ -240,7 +242,7 @@
assertTrue(netCap.appliesToUid(650));
assertFalse(netCap.appliesToUid(500));
- assertFalse(new NetworkCapabilities().satisfiedByUids(netCap));
+ assertTrue(new NetworkCapabilities().satisfiedByUids(netCap));
netCap.combineCapabilities(new NetworkCapabilities());
assertTrue(netCap.appliesToUid(500));
assertTrue(netCap.appliesToUidRange(new UidRange(1, 100000)));
diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java
index 28f8122..5ea21ea 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -387,6 +387,7 @@
mScore = 20;
break;
case TRANSPORT_VPN:
+ mNetworkCapabilities.removeCapability(NET_CAPABILITY_NOT_VPN);
mScore = ConnectivityConstants.VPN_DEFAULT_SCORE;
break;
default:
@@ -3744,14 +3745,19 @@
final int uid = Process.myUid();
final TestNetworkCallback genericNetworkCallback = new TestNetworkCallback();
+ final TestNetworkCallback genericNotVpnNetworkCallback = new TestNetworkCallback();
final TestNetworkCallback wifiNetworkCallback = new TestNetworkCallback();
final TestNetworkCallback vpnNetworkCallback = new TestNetworkCallback();
- final NetworkRequest genericRequest = new NetworkRequest.Builder().build();
+ final NetworkRequest genericNotVpnRequest = new NetworkRequest.Builder().build();
+ final NetworkRequest genericRequest = new NetworkRequest.Builder()
+ .removeCapability(NET_CAPABILITY_NOT_VPN).build();
final NetworkRequest wifiRequest = new NetworkRequest.Builder()
.addTransportType(TRANSPORT_WIFI).build();
final NetworkRequest vpnNetworkRequest = new NetworkRequest.Builder()
+ .removeCapability(NET_CAPABILITY_NOT_VPN)
.addTransportType(TRANSPORT_VPN).build();
mCm.registerNetworkCallback(genericRequest, genericNetworkCallback);
+ mCm.registerNetworkCallback(genericNotVpnRequest, genericNotVpnNetworkCallback);
mCm.registerNetworkCallback(wifiRequest, wifiNetworkCallback);
mCm.registerNetworkCallback(vpnNetworkRequest, vpnNetworkCallback);
@@ -3759,6 +3765,7 @@
mWiFiNetworkAgent.connect(false);
genericNetworkCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent);
+ genericNotVpnNetworkCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent);
wifiNetworkCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent);
vpnNetworkCallback.assertNoCallback();
@@ -3773,16 +3780,19 @@
vpnNetworkAgent.connect(false);
genericNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent);
+ genericNotVpnNetworkCallback.assertNoCallback();
wifiNetworkCallback.assertNoCallback();
vpnNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent);
genericNetworkCallback.expectCallback(CallbackState.NETWORK_CAPABILITIES, vpnNetworkAgent);
+ genericNotVpnNetworkCallback.assertNoCallback();
vpnNetworkCallback.expectCapabilitiesLike(nc -> null == nc.getUids(), vpnNetworkAgent);
ranges.clear();
vpnNetworkAgent.setUids(ranges);
genericNetworkCallback.expectCallback(CallbackState.LOST, vpnNetworkAgent);
+ genericNotVpnNetworkCallback.assertNoCallback();
wifiNetworkCallback.assertNoCallback();
vpnNetworkCallback.expectCallback(CallbackState.LOST, vpnNetworkAgent);
@@ -3790,18 +3800,21 @@
vpnNetworkAgent.setUids(ranges);
genericNetworkCallback.expectAvailableCallbacksValidated(vpnNetworkAgent);
+ genericNotVpnNetworkCallback.assertNoCallback();
wifiNetworkCallback.assertNoCallback();
vpnNetworkCallback.expectAvailableCallbacksValidated(vpnNetworkAgent);
mWiFiNetworkAgent.disconnect();
genericNetworkCallback.expectCallback(CallbackState.LOST, mWiFiNetworkAgent);
+ genericNotVpnNetworkCallback.expectCallback(CallbackState.LOST, mWiFiNetworkAgent);
wifiNetworkCallback.expectCallback(CallbackState.LOST, mWiFiNetworkAgent);
vpnNetworkCallback.assertNoCallback();
vpnNetworkAgent.disconnect();
genericNetworkCallback.expectCallback(CallbackState.LOST, vpnNetworkAgent);
+ genericNotVpnNetworkCallback.assertNoCallback();
wifiNetworkCallback.assertNoCallback();
vpnNetworkCallback.expectCallback(CallbackState.LOST, vpnNetworkAgent);