Fix fwmark handling for bypassable VPNs and DNS.
This is a significant change to the way fwmarks are handled for two purposes:
1. Bypassable VPN.
This was introduced in http://ag/510058 and had an issue that if there's a
default network, it would always be used in connect(), so the bypassable VPN
wouldn't get any traffic. This CL fixes that issue by using the bypassable
VPN's NetId in connect(). See the comments in the code for more details.
2. DNS.
The previous DNS code (specifically, getNetworkForUser()) had two problems:
+ Even if a user asks for a NetId they have permission for, we'd always use
the user's VPN if they were subject to one. So, for example, a system IMS
app that brings up the mobile network in the presence of a VPN would still
have its DNS queries sent over the VPN, instead of mobile as desired.
+ Any user could perform DNS over any valid network, even one they didn't
have permissions for, as long as they weren't subject to a VPN. So, for
example, an app could use the DNS servers of a different profile's VPN.
This CL fixes those problems. See getNetworkForDns() for more details.
The two pieces above are inter-related. Previously, we never set the explicit
bit from the DNS code. But we need to do that if the user asks for a network
explicitly, for two reasons:
o So that the DNS query is really restricted to that network and doesn't
fallthrough to the default network.
o So that the heuristic described in ON_CONNECT works in all cases. I.e., if the
DNS proxy's connect() request comes in with the explicit bit NOT set, we know
that the NetId can only be either the default network or a VPN.
This CL is not intended to be robust against race conditions. In general, very
little of the netd code is resilient. A separate effort needs to be undertaken
to carefully audit all the code and logic to guard against things like:
* A VPN being established between calls to getNetworkForDns() and connect().
* State changes between multiple calls to NetworkController from clients such as
FwmarkServer and DnsProxyListener.
* Routing rules / iptables rules being set up in a less-than-ideal order.
* ... etc.
Bug: 15347374
Change-Id: I5baad9168c4f4f3ef4129e07234b4bf24b0d8ba2
diff --git a/server/FwmarkServer.cpp b/server/FwmarkServer.cpp
index d8098e8..8bf8b71 100644
--- a/server/FwmarkServer.cpp
+++ b/server/FwmarkServer.cpp
@@ -102,17 +102,45 @@
}
case FwmarkCommand::ON_CONNECT: {
- // Called before a socket connect() happens. Set the default network's NetId into the
- // fwmark so that the socket routes consistently over that network. Do this even if the
- // socket already has a NetId, so that calling connect() multiple times still works.
+ // Called before a socket connect() happens. Set an appropriate NetId into the fwmark so
+ // that the socket routes consistently over that network. Do this even if the socket
+ // already has a NetId, so that calling connect() multiple times still works.
//
- // But respect the existing NetId if it had been explicitly preferred, indicated by:
- // + The explicit bit having been set.
- // + Or, the NetId being that of a VPN, which indicates a proxy acting on behalf of a
- // user who is subject to the VPN. The explicit bit is not set so that it works even
- // if the VPN is a split tunnel, but it's an explicit network preference nonetheless.
- if (!fwmark.explicitlySelected && !mNetworkController->isVirtualNetwork(fwmark.netId)) {
- fwmark.netId = mNetworkController->getDefaultNetwork();
+ // But if the explicit bit was set, the existing NetId was explicitly preferred (and not
+ // a case of connect() being called multiple times). Don't reset the NetId in that case.
+ //
+ // An "appropriate" NetId is the NetId of a bypassable VPN that applies to the user, or
+ // failing that, the default network. We'll never set the NetId of a secure VPN here.
+ // See the comments in the implementation of getNetworkForConnect() for more details.
+ //
+ // If the protect bit is set, this could be either a system proxy (e.g.: the dns proxy
+ // or the download manager) acting on behalf of another user, or a VPN provider. If it's
+ // a proxy, we shouldn't reset the NetId. If it's a VPN provider, we should set the
+ // default network's NetId.
+ //
+ // There's no easy way to tell the difference between a proxy and a VPN app. We can't
+ // use PERMISSION_SYSTEM to identify the proxy because a VPN app may also have those
+ // permissions. So we use the following heuristic:
+ //
+ // If it's a proxy, but the existing NetId is not a VPN, that means the user (that the
+ // proxy is acting on behalf of) is not subject to a VPN, so the proxy must have picked
+ // the default network's NetId. So, it's okay to replace that with the current default
+ // network's NetId (which in all likelihood is the same).
+ //
+ // Conversely, if it's a VPN provider, the existing NetId cannot be a VPN. The only time
+ // we set a VPN's NetId into a socket without setting the explicit bit is here, in
+ // ON_CONNECT, but we won't do that if the socket has the protect bit set. If the VPN
+ // provider connect()ed (and got the VPN NetId set) and then called protect(), we
+ // would've unset the NetId in PROTECT_FROM_VPN below.
+ //
+ // So, overall (when the explicit bit is not set but the protect bit is set), if the
+ // existing NetId is a VPN, don't reset it. Else, set the default network's NetId.
+ if (!fwmark.explicitlySelected) {
+ if (!fwmark.protectedFromVpn) {
+ fwmark.netId = mNetworkController->getNetworkForConnect(client->getUid());
+ } else if (!mNetworkController->isVirtualNetwork(fwmark.netId)) {
+ fwmark.netId = mNetworkController->getDefaultNetwork();
+ }
}
break;
}
@@ -136,6 +164,15 @@
if (!mNetworkController->canProtect(client->getUid())) {
return -EPERM;
}
+ // If a bypassable VPN's provider app calls connect() and then protect(), it will end up
+ // with a socket that looks like that of a system proxy but is not (see comments for
+ // ON_CONNECT above). So, reset the NetId.
+ //
+ // In any case, it's appropriate that if the socket has an implicit VPN NetId mark, the
+ // PROTECT_FROM_VPN command should unset it.
+ if (!fwmark.explicitlySelected && mNetworkController->isVirtualNetwork(fwmark.netId)) {
+ fwmark.netId = mNetworkController->getDefaultNetwork();
+ }
fwmark.protectedFromVpn = true;
permission = static_cast<Permission>(permission | fwmark.permission);
break;
@@ -145,7 +182,7 @@
if ((permission & PERMISSION_SYSTEM) != PERMISSION_SYSTEM) {
return -EPERM;
}
- fwmark.netId = mNetworkController->getNetworkForUser(command.uid, NETID_UNSET, false);
+ fwmark.netId = mNetworkController->getNetworkForUser(command.uid);
fwmark.protectedFromVpn = true;
break;
}