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
diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index 7050b42..bca3082 100644
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -3604,11 +3604,12 @@
switch (msg.what) {
case NetworkAgent.EVENT_NETWORK_CAPABILITIES_CHANGED: {
- final NetworkCapabilities networkCapabilities = new NetworkCapabilities(
- (NetworkCapabilities) arg.second);
- maybeUpdateWifiRoamTimestamp(nai, networkCapabilities);
- processCapabilitiesFromAgent(nai, networkCapabilities);
- updateCapabilities(nai.getCurrentScore(), nai, networkCapabilities);
+ nai.declaredCapabilitiesUnsanitized =
+ new NetworkCapabilities((NetworkCapabilities) arg.second);
+ final NetworkCapabilities sanitized = sanitizedCapabilitiesFromAgent(
+ nai, nai.declaredCapabilitiesUnsanitized);
+ maybeUpdateWifiRoamTimestamp(nai, sanitized);
+ updateCapabilities(nai.getCurrentScore(), nai, sanitized);
break;
}
case NetworkAgent.EVENT_NETWORK_PROPERTIES_CHANGED: {
@@ -7323,11 +7324,14 @@
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
- final NetworkCapabilities nc = new NetworkCapabilities(nai.networkCapabilities);
+ // mutate their object. TODO : make this copy much earlier to avoid them mutating it
+ // 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.
- processCapabilitiesFromAgent(nai, nc);
+ // Store a copy of the declared capabilities.
+ 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 @@
}
/**
- * Called when receiving NetworkCapabilities directly from a NetworkAgent.
- * 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
- * agent is not lost when updateCapabilities is called.
+ * 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}.
*/
- 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 @@
}
if (nai.propagateUnderlyingCapabilities()) {
- applyUnderlyingCapabilities(nai.declaredUnderlyingNetworks, nai.declaredCapabilities,
+ applyUnderlyingCapabilities(nai.declaredUnderlyingNetworks,
+ sanitizedCapabilitiesFromAgent(nai, nai.declaredCapabilitiesUnsanitized),
newNc);
}
diff --git a/service/src/com/android/server/connectivity/NetworkAgentInfo.java b/service/src/com/android/server/connectivity/NetworkAgentInfo.java
index b40b6e0..04031af 100644
--- a/service/src/com/android/server/connectivity/NetworkAgentInfo.java
+++ b/service/src/com/android/server/connectivity/NetworkAgentInfo.java
@@ -181,8 +181,12 @@
// 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.