Send null UIDs to apps instead of single-uid lists.

Prior to this change ConnectivityManager used to patch in the UID
of the requesting app inside the NetworkCapabilities sent to it.
The rationale was that the app may not know what other apps may
use the network, so the view it should have of the network should
always say the network only applies to that app.

But this has an unfortunate side effect : apps can't match the
received network against a default NetworkCapabilities. Ostensibly
this only applies to the system because all involved calls are
@hide, but still : system code would get some NetworkCapabilities,
for example using networkCapabilitiesForType, and then try to
match the capabilities of an available network using
satisfiedByNetworkCapabilities. Because the passed network is
declared to only apply to one's own UID and the UIDs of the
NetworkCapabilities are set to null meaning "I need this network
to apply to all UIDs", the answer will be "false".

While this is WAI in a sense, it is very counter-intuitive that
code trying to match a network would be required to patch in its
own UIDs.
There are three ways of fixing this :
1. Require all apps to do the above. It's correct, but it's
   cumbersome and counterintuitive. Multiple places in existing
   code needs to be fixed, Tethering is an example.
2. Write the UIDs of the caller in any NetworkCapabilities object
   that is created. This is not very practical, because it imposes
   the converse requirement on all NetworkAgents, which would then
   have to clear the UIDs before they send the capabilities to
   ConnectivityService. All NetworkAgents need to be fixed.
3. Instead of sending an object with a list of one UID to apps,
   send a null list. The drawback is that the networks nominally
   look to apps like they apply to all apps. I argue this does
   not matter ; what matters is that the UID lists do not leak.
   Clients just see a null list of UIDs (and third party can't
   even access them without using reflection). No other changes
   are required besides this two-line patch.

This patch implements 3. I believe it is the saner approach, with
both the most intuitive behavior and the best backward compatibility
characteristics, as well as the easiest change.

This does not encroach on the future plans to make the actual
UID list available to apps with NETWORK_SETTINGS.

Test: runtest frameworks-net
Change-Id: I978d91197668119e051c24e1d04aafe1644a41cf

2 files changed, 15 insertions, 12 deletions

diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index bff5c10d4e82..dc70347035ce 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -1260,11 +1260,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
                         for (Network network : networks) {
                             nai = getNetworkAgentInfoForNetwork(network);
                             nc = getNetworkCapabilitiesInternal(nai);
-                            // nc is a copy of the capabilities in nai, so it's fine to mutate it
-                            // TODO : don't remove the UIDs when communicating with processes
-                            // that have the NETWORK_SETTINGS permission.
                             if (nc != null) {
-                                nc.setSingleUid(userId);
                                 result.put(network, nc);
                             }
                         }
@@ -1332,7 +1328,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
         if (nai != null) {
             synchronized (nai) {
                 if (nai.networkCapabilities != null) {
-                    return new NetworkCapabilities(nai.networkCapabilities);
+                    // TODO : don't remove the UIDs when communicating with processes
+                    // that have the NETWORK_SETTINGS permission.
+                    return networkCapabilitiesWithoutUids(nai.networkCapabilities);
                 }
             }
         }
@@ -1345,6 +1343,10 @@ public class ConnectivityService extends IConnectivityManager.Stub
         return getNetworkCapabilitiesInternal(getNetworkAgentInfoForNetwork(network));
     }
 
+    private NetworkCapabilities networkCapabilitiesWithoutUids(NetworkCapabilities nc) {
+        return new NetworkCapabilities(nc).setUids(null);
+    }
+
     @Override
     public NetworkState[] getAllNetworkState() {
         // Require internal since we're handing out IMSI details
@@ -1354,6 +1356,10 @@ public class ConnectivityService extends IConnectivityManager.Stub
         for (Network network : getAllNetworks()) {
             final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork(network);
             if (nai != null) {
+                // TODO (b/73321673) : NetworkState contains a copy of the
+                // NetworkCapabilities, which may contain UIDs of apps to which the
+                // network applies. Should the UIDs be cleared so as not to leak or
+                // interfere ?
                 result.add(nai.getNetworkState());
             }
         }
@@ -4909,7 +4915,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
         releasePendingNetworkRequestWithDelay(pendingIntent);
     }
 
-    private static void callCallbackForRequest(NetworkRequestInfo nri,
+    private void callCallbackForRequest(NetworkRequestInfo nri,
             NetworkAgentInfo networkAgent, int notificationType, int arg1) {
         if (nri.messenger == null) {
             return;  // Default request has no msgr
@@ -4927,11 +4933,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 break;
             }
             case ConnectivityManager.CALLBACK_CAP_CHANGED: {
+                // networkAgent can't be null as it has been accessed a few lines above.
                 final NetworkCapabilities nc =
-                        new NetworkCapabilities(networkAgent.networkCapabilities);
-                // TODO : don't remove the UIDs when communicating with processes
-                // that have the NETWORK_SETTINGS permission.
-                nc.setSingleUid(nri.mUid);
+                        networkCapabilitiesWithoutUids(networkAgent.networkCapabilities);
                 putParcelable(bundle, nc);
                 break;
             }
diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java
index e7abede4cda4..2ec19bf75416 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -3682,8 +3682,7 @@ public class ConnectivityServiceTest {
         vpnNetworkCallback.expectAvailableCallbacksUnvalidated(vpnNetworkAgent);
 
         genericNetworkCallback.expectCallback(CallbackState.NETWORK_CAPABILITIES, vpnNetworkAgent);
-        vpnNetworkCallback.expectCapabilitiesLike(
-                nc -> nc.appliesToUid(uid) && !nc.appliesToUid(uid + 1), vpnNetworkAgent);
+        vpnNetworkCallback.expectCapabilitiesLike(nc -> null == nc.getUids(), vpnNetworkAgent);
 
         ranges.clear();
         vpnNetworkAgent.setUids(ranges);