LineageOS/android_packages_modules_Connectivity
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Sanitize NetworkCapabilities from agent on the handler thread

Browse Source 
NetworkAgents send NetworkCapabilities to ConnectivityService but
there are limits to what exactly they can send. Going forward,
some of these checks will have to happen on the handler thread,
which is already the case when an agent updates its capabilities,
but not upon registration.

This patches moves the sanitization on the handler thread, after
the network monitor is created for a network agent.

Before this patch, upon registration of a new agent, the binder
thread would copy and sanitize the capabilities, then store them
in nai.networkCapabilities. It would store the original caps from
the agent in the NAI, mix in what is known from the network info,
process the LinkProperties, and then proceed to create the
network monitor, but not yet store the NAI in the internal
structures because its registration is not finalized, so other
methods should not see it yet. After the monitor is created in
the network stack process, the NAI is stored in the internal
structures which publishes it for all methods to see. After
that is done, the NAI calls to the network monitor to warn it
that it's registered, what its capabilities are, and that it's
time to start validation if applicable.

With this patch, the validation no longer happens on the binder
thread. Instead, the binder thread stores the capabilities and
link properties as is, before sanitization, in the NAI. This is
fine because no other method can access these until the
registration completes upon notification that the monitor has
been created ; this agent is only stored in the network monitor
callbacks in a self-destructing object precisely to make sure
that's the case.
When the monitor is created and CS receives notification of the
same, it will sanitize the capabilities before adding the NAI
to the internal structures, to protect the invariant that the
un-sanitized capabilities inside the NAI can't ever be seen by
any other method. After that's done, it will call to the
monitor to start validation as usual.

Test: FrameworksNetTests CtsNetTestsCases
Change-Id: I7d43ef0e25955e0349903b4801b9dfd8c3c92586
This commit is contained in:
Chalard Jean
2022-01-25 18:27:53 +09:00
parent e9cd2084e4
commit 366c525b15
1 changed files with 28 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
service/src/com/android/server/ConnectivityService.java
View File

@@ -3352,14 +3352,11 @@ public class ConnectivityService extends IConnectivityManager.Stub
if (networkCapabilities.hasConnectivityManagedCapability()) {
Log.wtf(TAG, "BUG: " + nai + " has CS-managed capability.");
}
if (networkCapabilities.hasTransport(TRANSPORT_TEST)) {
// Make sure the original object is not mutated. NetworkAgent normally
// makes a copy of the capabilities when sending the message through
// the Messenger, but if this ever changes, not making a defensive copy
// here will give attack vectors to clients using this code path.
networkCapabilities = new NetworkCapabilities(networkCapabilities);
networkCapabilities.restrictCapabilitiesForTestNetwork(nai.creatorUid);
}
// Make sure the original object is not mutated. NetworkAgent normally
// makes a copy of the capabilities when sending the message through
// the Messenger, but if this ever changes, not making a defensive copy
// here will give attack vectors to clients using this code path.
networkCapabilities = new NetworkCapabilities(networkCapabilities);
processCapabilitiesFromAgent(nai, networkCapabilities);
updateCapabilities(nai.getCurrentScore(), nai, networkCapabilities);
break;
@@ -6976,28 +6973,18 @@ public class ConnectivityService extends IConnectivityManager.Stub
LinkProperties linkProperties, NetworkCapabilities networkCapabilities,
NetworkScore currentScore, NetworkAgentConfig networkAgentConfig, int providerId,
int uid) {
if (networkCapabilities.hasTransport(TRANSPORT_TEST)) {
// Strictly, sanitizing here is unnecessary as the capabilities will be sanitized in
// the call to mixInCapabilities below anyway, but sanitizing here means the NAI never
// sees capabilities that may be malicious, which might prevent mistakes in the future.
networkCapabilities = new NetworkCapabilities(networkCapabilities);
networkCapabilities.restrictCapabilitiesForTestNetwork(uid);
}
LinkProperties lp = new LinkProperties(linkProperties);
final NetworkCapabilities nc = new NetworkCapabilities(networkCapabilities);
// At this point the capabilities/properties are untrusted and unverified, e.g. checks that
// the capabilities' access UID comply with security limitations. They will be sanitized
// as the NAI registration finishes, in handleRegisterNetworkAgent(). This is
// because some of the checks must happen on the handler thread.
final NetworkAgentInfo nai = new NetworkAgentInfo(na,
new Network(mNetIdManager.reserveNetId()), new NetworkInfo(networkInfo), lp, nc,
new Network(mNetIdManager.reserveNetId()), new NetworkInfo(networkInfo),
linkProperties, networkCapabilities,
currentScore, mContext, mTrackerHandler, new NetworkAgentConfig(networkAgentConfig),
this, mNetd, mDnsResolver, providerId, uid, mLingerDelayMs,
mQosCallbackTracker, mDeps);
// Make sure the LinkProperties and NetworkCapabilities reflect what the agent info says.
processCapabilitiesFromAgent(nai, nc);
nai.getAndSetNetworkCapabilities(mixInCapabilities(nai, nc));
processLinkPropertiesFromAgent(nai, nai.linkProperties);
final String extraInfo = networkInfo.getExtraInfo();
final String name = TextUtils.isEmpty(extraInfo)
? nai.networkCapabilities.getSsid() : extraInfo;
@@ -7012,8 +6999,20 @@ public class ConnectivityService extends IConnectivityManager.Stub
}
private void handleRegisterNetworkAgent(NetworkAgentInfo nai, INetworkMonitor networkMonitor) {
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);
final LinkProperties lp = new LinkProperties(nai.linkProperties);
// Make sure the LinkProperties and NetworkCapabilities reflect what the agent info says.
processCapabilitiesFromAgent(nai, nc);
nai.getAndSetNetworkCapabilities(mixInCapabilities(nai, nc));
processLinkPropertiesFromAgent(nai, lp);
nai.linkProperties = lp;
nai.onNetworkMonitorCreated(networkMonitor);
if (VDBG) log("Got NetworkAgent Messenger");
mNetworkAgentInfos.add(nai);
synchronized (mNetworkForNetId) {
mNetworkForNetId.put(nai.network.getNetId(), nai);
@@ -7024,6 +7023,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
} catch (RemoteException e) {
e.rethrowAsRuntimeException();
}
nai.notifyRegistered();
NetworkInfo networkInfo = nai.networkInfo;
updateNetworkInfo(nai, networkInfo);
@@ -7484,6 +7484,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
nc.setOwnerUid(nai.networkCapabilities.getOwnerUid());
}
nai.declaredCapabilities = new NetworkCapabilities(nc);
if (nc.hasTransport(TRANSPORT_TEST)) {
nc.restrictCapabilitiesForTestNetwork(nai.creatorUid);
}
}
/** Modifies |newNc| based on the capabilities of |underlyingNetworks| and |agentCaps|. */