Clarify sanitization of caps from NetworkAgent

This is a first step towards addressing the leftover
comment from aosp/1958906.

Bug: 238139913
Test: FrameworksNetTests CtsNetTestCases
Change-Id: I46c5dfebfc12c4db71f7c9c6624b8d6cdf0bf3b5

service/src/com/android/server/ConnectivityService.java

@@ -3604,11 +3604,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
 
             switch (msg.what) {
                 case NetworkAgent.EVENT_NETWORK_CAPABILITIES_CHANGED: {
-                    final NetworkCapabilities networkCapabilities = new NetworkCapabilities(
+                    nai.declaredCapabilitiesUnsanitized =
-                            (NetworkCapabilities) arg.second);
+                            new NetworkCapabilities((NetworkCapabilities) arg.second);
-                    maybeUpdateWifiRoamTimestamp(nai, networkCapabilities);
+                    final NetworkCapabilities sanitized = sanitizedCapabilitiesFromAgent(
-                    processCapabilitiesFromAgent(nai, networkCapabilities);
+                            nai, nai.declaredCapabilitiesUnsanitized);
-                    updateCapabilities(nai.getCurrentScore(), nai, networkCapabilities);
+                    maybeUpdateWifiRoamTimestamp(nai, sanitized);
+                    updateCapabilities(nai.getCurrentScore(), nai, sanitized);
                     break;
                 }
                 case NetworkAgent.EVENT_NETWORK_PROPERTIES_CHANGED: {
@@ -7323,11 +7324,14 @@ public class ConnectivityService extends IConnectivityManager.Stub
         if (VDBG) log("Network Monitor created for " +  nai);
         // nai.nc and nai.lp are the same object that was passed by the network agent if the agent
         // lives in the same process as this code (e.g. wifi), so make sure this code doesn't
-        // mutate their object
+        // mutate their object. TODO : make this copy much earlier to avoid them mutating it
-        final NetworkCapabilities nc = new NetworkCapabilities(nai.networkCapabilities);
+        // while the network monitor is starting.
         final LinkProperties lp = new LinkProperties(nai.linkProperties);
-        // Make sure the LinkProperties and NetworkCapabilities reflect what the agent info says.
+        // Store a copy of the declared capabilities.
-        processCapabilitiesFromAgent(nai, nc);
+        nai.declaredCapabilitiesUnsanitized = new NetworkCapabilities(nai.networkCapabilities);
+        // Make sure the LinkProperties and NetworkCapabilities reflect what the agent info said.
+        final NetworkCapabilities nc =
+                sanitizedCapabilitiesFromAgent(nai, nai.declaredCapabilitiesUnsanitized);
         nai.getAndSetNetworkCapabilities(mixInCapabilities(nai, nc));
         processLinkPropertiesFromAgent(nai, lp);
         nai.linkProperties = lp;
@@ -7792,28 +7796,34 @@ public class ConnectivityService extends IConnectivityManager.Stub
     }
 
     /**
-     * Called when receiving NetworkCapabilities directly from a NetworkAgent.
+     * Sanitize capabilities coming from a network agent.
-     * Stores into |nai| any data coming from the agent that might also be written to the network's
+     *
-     * NetworkCapabilities by ConnectivityService itself. This ensures that the data provided by the
+     * Agents have restrictions on what capabilities they can send to Connectivity. For example,
-     * agent is not lost when updateCapabilities is called.
+     * they can't change the owner UID from what they declared before, and complex restrictions
+     * apply to the accessUids field.
+     * They also should not mutate immutable capabilities, although for backward-compatibility
+     * this is not enforced and limited to just a log.
+     *
+     * This method returns a sanitized copy of the passed capabilities to make sure they don't
+     * contain stuff they should not, and should generally be called by code that accesses
+     * {@link NetworkAgentInfo#declaredCapabilitiesUnsanitized}.
      */
-    private void processCapabilitiesFromAgent(NetworkAgentInfo nai, NetworkCapabilities nc) {
+    // TODO : move this to NetworkAgentInfo
+    private NetworkCapabilities sanitizedCapabilitiesFromAgent(@NonNull final NetworkAgentInfo nai,
+            @NonNull final NetworkCapabilities unsanitized) {
+        final NetworkCapabilities nc = new NetworkCapabilities(unsanitized);
         if (nc.hasConnectivityManagedCapability()) {
             Log.wtf(TAG, "BUG: " + nai + " has CS-managed capability.");
         }
-        // Note: resetting the owner UID before storing the agent capabilities in NAI means that if
-        // the agent attempts to change the owner UID, then nai.declaredCapabilities will not
-        // actually be the same as the capabilities sent by the agent. Still, it is safer to reset
-        // the owner UID here and behave as if the agent had never tried to change it.
         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());
         }
-        nai.declaredCapabilities = new NetworkCapabilities(nc);
         NetworkAgentInfo.restrictCapabilitiesFromNetworkAgent(nc, nai.creatorUid,
                 mContext.getPackageManager().hasSystemFeature(PackageManager.FEATURE_AUTOMOTIVE),
                 mCarrierPrivilegeAuthenticator);
+        return nc;
     }
 
     /** Modifies |newNc| based on the capabilities of |underlyingNetworks| and |agentCaps|. */
@@ -7940,7 +7950,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
 
         if (nai.propagateUnderlyingCapabilities()) {
-            applyUnderlyingCapabilities(nai.declaredUnderlyingNetworks, nai.declaredCapabilities,
+            applyUnderlyingCapabilities(nai.declaredUnderlyingNetworks,
+                    sanitizedCapabilitiesFromAgent(nai, nai.declaredCapabilitiesUnsanitized),
                     newNc);
         }
 

service/src/com/android/server/connectivity/NetworkAgentInfo.java

@@ -181,8 +181,12 @@ public class NetworkAgentInfo implements Comparable<NetworkAgentInfo>, NetworkRa
 
     // The capabilities originally announced by the NetworkAgent, regardless of any capabilities
     // that were added or removed due to this network's underlying networks.
-    // Only set if #propagateUnderlyingCapabilities is true.
+    //
-    public @Nullable NetworkCapabilities declaredCapabilities;
+    // As the name implies, these capabilities are not sanitized and are not to
+    // be trusted. Most callers should simply use the {@link networkCapabilities}
+    // field instead, and callers who need the declared capabilities should generally
+    // pass these to {@link ConnectivityService#sanitizedCapabilitiesFromAgent} before using them.
+    public @Nullable NetworkCapabilities declaredCapabilitiesUnsanitized;
 
     // Indicates if netd has been told to create this Network. From this point on the appropriate
     // routing rules are setup and routes are added so packets can begin flowing over the Network.