Merge "Force specifying sanitized/not sanitized in caps from agent"

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

@@ -3604,10 +3604,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
 
             switch (msg.what) {
                 case NetworkAgent.EVENT_NETWORK_CAPABILITIES_CHANGED: {
-                    nai.declaredCapabilitiesUnsanitized =
+                    nai.setDeclaredCapabilities((NetworkCapabilities) arg.second);
-                            new NetworkCapabilities((NetworkCapabilities) arg.second);
+                    final NetworkCapabilities sanitized =
-                    final NetworkCapabilities sanitized = sanitizedCapabilitiesFromAgent(
+                            nai.getDeclaredCapabilitiesSanitized(mCarrierPrivilegeAuthenticator);
-                            mCarrierPrivilegeAuthenticator, nai);
                     maybeUpdateWifiRoamTimestamp(nai, sanitized);
                     updateCapabilities(nai.getCurrentScore(), nai, sanitized);
                     break;
@@ -7328,11 +7327,11 @@ public class ConnectivityService extends IConnectivityManager.Stub
         // while the network monitor is starting.
         final LinkProperties lp = new LinkProperties(nai.linkProperties);
         // Store a copy of the declared capabilities.
-        nai.declaredCapabilitiesUnsanitized = new NetworkCapabilities(nai.networkCapabilities);
+        nai.setDeclaredCapabilities(nai.networkCapabilities);
         // Make sure the LinkProperties and NetworkCapabilities reflect what the agent info said.
-        final NetworkCapabilities nc =
+        final NetworkCapabilities sanitized =
-                sanitizedCapabilitiesFromAgent(mCarrierPrivilegeAuthenticator, nai);
+                nai.getDeclaredCapabilitiesSanitized(mCarrierPrivilegeAuthenticator);
-        nai.getAndSetNetworkCapabilities(mixInCapabilities(nai, nc));
+        nai.getAndSetNetworkCapabilities(mixInCapabilities(nai, sanitized));
         processLinkPropertiesFromAgent(nai, lp);
         nai.linkProperties = lp;
 
@@ -7795,38 +7794,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
     }
 
-    /**
-     * Sanitize capabilities coming from a network agent.
-     *
-     * Agents have restrictions on what capabilities they can send to Connectivity. For example,
-     * 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}.
-     */
-    // TODO : move this to NetworkAgentInfo
-    private NetworkCapabilities sanitizedCapabilitiesFromAgent(
-            final CarrierPrivilegeAuthenticator carrierPrivilegeAuthenticator,
-            @NonNull final NetworkAgentInfo nai) {
-        final NetworkCapabilities nc = new NetworkCapabilities(nai.declaredCapabilitiesUnsanitized);
-        if (nc.hasConnectivityManagedCapability()) {
-            Log.wtf(TAG, "BUG: " + nai + " has CS-managed capability.");
-        }
-        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());
-        }
-        NetworkAgentInfo.restrictCapabilitiesFromNetworkAgent(nc, nai.creatorUid,
-                mContext.getPackageManager().hasSystemFeature(PackageManager.FEATURE_AUTOMOTIVE),
-                carrierPrivilegeAuthenticator);
-        return nc;
-    }
-
     /** Modifies |newNc| based on the capabilities of |underlyingNetworks| and |agentCaps|. */
     @VisibleForTesting
     void applyUnderlyingCapabilities(@Nullable Network[] underlyingNetworks,
@@ -7952,7 +7919,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
 
         if (nai.propagateUnderlyingCapabilities()) {
             applyUnderlyingCapabilities(nai.declaredUnderlyingNetworks,
-                    sanitizedCapabilitiesFromAgent(mCarrierPrivilegeAuthenticator, nai),
+                    nai.getDeclaredCapabilitiesSanitized(mCarrierPrivilegeAuthenticator),
                     newNc);
         }
 

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

@@ -26,6 +26,7 @@ import static android.net.NetworkCapabilities.transportNamesOf;
 import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.content.Context;
+import android.content.pm.PackageManager;
 import android.net.CaptivePortalData;
 import android.net.DscpPolicy;
 import android.net.IDnsResolver;
@@ -184,9 +185,8 @@ public class NetworkAgentInfo implements Comparable<NetworkAgentInfo>, NetworkRa
     //
     // 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
+    // field instead.
-    // pass these to {@link ConnectivityService#sanitizedCapabilitiesFromAgent} before using them.
+    private @Nullable NetworkCapabilities mDeclaredCapabilitiesUnsanitized;
-    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.
@@ -240,6 +240,53 @@ public class NetworkAgentInfo implements Comparable<NetworkAgentInfo>, NetworkRa
     // URL, Terms & Conditions URL, and network friendly name.
     public CaptivePortalData networkAgentPortalData;
 
+    /**
+     * Sets the capabilities sent by the agent for later retrieval.
+     *
+     * This method does not sanitize the capabilities ; instead, use
+     * {@link #getDeclaredCapabilitiesSanitized} to retrieve a sanitized
+     * copy of the capabilities as they were passed here.
+     *
+     * This method makes a defensive copy to avoid issues where the passed object is later mutated.
+     *
+     * @param caps the caps sent by the agent
+     */
+    public void setDeclaredCapabilities(@NonNull final NetworkCapabilities caps) {
+        mDeclaredCapabilitiesUnsanitized = new NetworkCapabilities(caps);
+    }
+
+    /**
+     * Get the latest capabilities sent by the network agent, after sanitizing them.
+     *
+     * These are the capabilities as they were sent by the agent (but sanitized to conform to
+     * their restrictions). They are NOT the capabilities currently applying to this agent ;
+     * for that, use {@link #networkCapabilities}.
+     *
+     * Agents have restrictions on what capabilities they can send to Connectivity. For example,
+     * they can't change the owner UID from what they declared before, and complex restrictions
+     * apply to the allowedUids field.
+     * They also should not mutate immutable capabilities, although for backward-compatibility
+     * this is not enforced and limited to just a log.
+     *
+     * @param carrierPrivilegeAuthenticator the authenticator, to check access UIDs.
+     */
+    public NetworkCapabilities getDeclaredCapabilitiesSanitized(
+            final CarrierPrivilegeAuthenticator carrierPrivilegeAuthenticator) {
+        final NetworkCapabilities nc = new NetworkCapabilities(mDeclaredCapabilitiesUnsanitized);
+        if (nc.hasConnectivityManagedCapability()) {
+            Log.wtf(TAG, "BUG: " + this + " has CS-managed capability.");
+        }
+        if (networkCapabilities.getOwnerUid() != nc.getOwnerUid()) {
+            Log.e(TAG, toShortString() + ": ignoring attempt to change owner from "
+                    + networkCapabilities.getOwnerUid() + " to " + nc.getOwnerUid());
+            nc.setOwnerUid(networkCapabilities.getOwnerUid());
+        }
+        restrictCapabilitiesFromNetworkAgent(nc, creatorUid,
+                mContext.getPackageManager().hasSystemFeature(PackageManager.FEATURE_AUTOMOTIVE),
+                carrierPrivilegeAuthenticator);
+        return nc;
+    }
+
     // Networks are lingered when they become unneeded as a result of their NetworkRequests being
     // satisfied by a higher-scoring network. so as to allow communication to wrap up before the
     // network is taken down.  This usually only happens to the default network. Lingering ends with

tests/unit/java/com/android/server/ConnectivityServiceTest.java

@@ -241,6 +241,7 @@ import android.net.ConnectivitySettingsManager;
 import android.net.ConnectivityThread;
 import android.net.DataStallReportParcelable;
 import android.net.EthernetManager;
+import android.net.EthernetNetworkSpecifier;
 import android.net.IConnectivityDiagnosticsCallback;
 import android.net.IDnsResolver;
 import android.net.INetd;
@@ -15699,6 +15700,45 @@ public class ConnectivityServiceTest {
         mCm.unregisterNetworkCallback(cb);
     }
 
+    @Test
+    public void testSanitizedCapabilitiesFromAgentDoesNotMutateArgument()
+            throws Exception {
+        // This NetworkCapabilities builds an usual object to maximize the chance that this requires
+        // sanitization, so we have a high chance to detect any changes to the original.
+        final NetworkCapabilities unsanitized = new NetworkCapabilities.Builder()
+                .withoutDefaultCapabilities()
+                .addTransportType(TRANSPORT_WIFI)
+                .addCapability(NET_CAPABILITY_INTERNET)
+                .setOwnerUid(12345)
+                .setAdministratorUids(new int[] {12345, 23456, 34567})
+                .setLinkUpstreamBandwidthKbps(20)
+                .setLinkDownstreamBandwidthKbps(10)
+                .setNetworkSpecifier(new EthernetNetworkSpecifier("foobar"))
+                .setTransportInfo(new WifiInfo.Builder().setBssid("AA:AA:AA:AA:AA:AA").build())
+                .setSignalStrength(-75)
+                .setSsid("SSID1")
+                .setRequestorUid(98765)
+                .setRequestorPackageName("TestPackage")
+                .setSubscriptionIds(Collections.singleton(Process.myUid()))
+                .setUids(UidRange.toIntRanges(uidRangesForUids(
+                        UserHandle.getUid(PRIMARY_USER, 10100),
+                        UserHandle.getUid(SECONDARY_USER, 10101),
+                        UserHandle.getUid(TERTIARY_USER, 10043))))
+                .setAllowedUids(Set.of(45678, 56789, 65432))
+                .setUnderlyingNetworks(List.of(new Network(99999)))
+                .build();
+        final NetworkCapabilities copyOfUnsanitized = new NetworkCapabilities(unsanitized);
+        final NetworkInfo info = new NetworkInfo(TYPE_MOBILE, TelephonyManager.NETWORK_TYPE_LTE,
+                ConnectivityManager.getNetworkTypeName(TYPE_MOBILE),
+                TelephonyManager.getNetworkTypeName(TelephonyManager.NETWORK_TYPE_LTE));
+        final NetworkAgentInfo agent = fakeNai(unsanitized, info);
+        agent.setDeclaredCapabilities(unsanitized);
+        final NetworkCapabilities sanitized = agent.getDeclaredCapabilitiesSanitized(
+                null /* carrierPrivilegeAuthenticator */);
+        assertEquals(copyOfUnsanitized, unsanitized);
+        assertNotEquals(sanitized, unsanitized);
+    }
+
     /**
      * Validate request counts are counted accurately on setProfileNetworkPreference on set/replace.
      */