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/NetworkController.cpp b/server/NetworkController.cpp
index d151490..16fc99f 100644
--- a/server/NetworkController.cpp
+++ b/server/NetworkController.cpp
@@ -32,6 +32,7 @@
#include "NetworkController.h"
+#include "Fwmark.h"
#include "LocalNetwork.h"
#include "PhysicalNetwork.h"
#include "RouteController.h"
@@ -92,14 +93,62 @@
return 0;
}
-unsigned NetworkController::getNetworkForUser(uid_t uid, unsigned requestedNetId,
- bool forDns) const {
+uint32_t NetworkController::getNetworkForDns(unsigned* netId, uid_t uid) const {
android::RWLock::AutoRLock lock(mRWLock);
- VirtualNetwork* virtualNetwork = getVirtualNetworkForUserLocked(uid);
- if (virtualNetwork && (!forDns || virtualNetwork->getHasDns())) {
+ Fwmark fwmark;
+ fwmark.protectedFromVpn = true;
+ fwmark.permission = PERMISSION_SYSTEM;
+ if (canUserSelectNetworkLocked(uid, *netId)) {
+ // If a non-zero NetId was explicitly specified, and the user has permission for that
+ // network, use that network's DNS servers. Do not fall through to the default network even
+ // if the explicitly selected network is a split tunnel VPN or a VPN without DNS servers.
+ fwmark.explicitlySelected = true;
+ } else {
+ // If the user is subject to a VPN and the VPN provides DNS servers, use those servers
+ // (possibly falling through to the default network if the VPN doesn't provide a route to
+ // them). Otherwise, use the default network's DNS servers.
+ VirtualNetwork* virtualNetwork = getVirtualNetworkForUserLocked(uid);
+ if (virtualNetwork && virtualNetwork->getHasDns()) {
+ *netId = virtualNetwork->getNetId();
+ } else {
+ *netId = mDefaultNetId;
+ }
+ }
+ fwmark.netId = *netId;
+ return fwmark.intValue;
+}
+
+// Returns the NetId that a given UID would use if no network is explicitly selected. Specifically,
+// the VPN that applies to the UID if any; otherwise, the default network.
+unsigned NetworkController::getNetworkForUser(uid_t uid) const {
+ android::RWLock::AutoRLock lock(mRWLock);
+ if (VirtualNetwork* virtualNetwork = getVirtualNetworkForUserLocked(uid)) {
return virtualNetwork->getNetId();
}
- return getNetworkLocked(requestedNetId) ? requestedNetId : mDefaultNetId;
+ return mDefaultNetId;
+}
+
+// Returns the NetId that will be set when a socket connect()s. This is the bypassable VPN that
+// applies to the user if any; otherwise, the default network.
+//
+// In general, we prefer to always set the default network's NetId in connect(), so that if the VPN
+// is a split-tunnel and disappears later, the socket continues working (since the default network's
+// NetId is still valid). Secure VPNs will correctly grab the socket's traffic since they have a
+// high-priority routing rule that doesn't care what NetId the socket has.
+//
+// But bypassable VPNs have a very low priority rule, so we need to mark the socket with the
+// bypassable VPN's NetId if we expect it to get any traffic at all. If the bypassable VPN is a
+// split-tunnel, that's okay, because we have fallthrough rules that will direct the fallthrough
+// traffic to the default network. But it does mean that if the bypassable VPN goes away (and thus
+// the fallthrough rules also go away), the socket that used to fallthrough to the default network
+// will stop working.
+unsigned NetworkController::getNetworkForConnect(uid_t uid) const {
+ android::RWLock::AutoRLock lock(mRWLock);
+ VirtualNetwork* virtualNetwork = getVirtualNetworkForUserLocked(uid);
+ if (virtualNetwork && !virtualNetwork->isSecure()) {
+ return virtualNetwork->getNetId();
+ }
+ return mDefaultNetId;
}
unsigned NetworkController::getNetworkForInterface(const char* interface) const {
@@ -224,24 +273,7 @@
bool NetworkController::canUserSelectNetwork(uid_t uid, unsigned netId) const {
android::RWLock::AutoRLock lock(mRWLock);
- Network* network = getNetworkLocked(netId);
- if (!network || uid == INVALID_UID) {
- return false;
- }
- Permission userPermission = getPermissionForUserLocked(uid);
- if ((userPermission & PERMISSION_SYSTEM) == PERMISSION_SYSTEM) {
- return true;
- }
- if (network->getType() == Network::VIRTUAL) {
- return static_cast<VirtualNetwork*>(network)->appliesToUser(uid);
- }
- VirtualNetwork* virtualNetwork = getVirtualNetworkForUserLocked(uid);
- if (virtualNetwork && virtualNetwork->isSecure() &&
- mProtectableUsers.find(uid) == mProtectableUsers.end()) {
- return false;
- }
- Permission networkPermission = static_cast<PhysicalNetwork*>(network)->getPermission();
- return (userPermission & networkPermission) == networkPermission;
+ return canUserSelectNetworkLocked(uid, netId);
}
int NetworkController::setPermissionForNetworks(Permission permission,
@@ -347,6 +379,29 @@
return uid < FIRST_APPLICATION_UID ? PERMISSION_SYSTEM : PERMISSION_NONE;
}
+bool NetworkController::canUserSelectNetworkLocked(uid_t uid, unsigned netId) const {
+ Network* network = getNetworkLocked(netId);
+ // If uid is INVALID_UID, this likely means that we were unable to retrieve the UID of the peer
+ // (using SO_PEERCRED). Be safe and deny access to the network, even if it's valid.
+ if (!network || uid == INVALID_UID) {
+ return false;
+ }
+ Permission userPermission = getPermissionForUserLocked(uid);
+ if ((userPermission & PERMISSION_SYSTEM) == PERMISSION_SYSTEM) {
+ return true;
+ }
+ if (network->getType() == Network::VIRTUAL) {
+ return static_cast<VirtualNetwork*>(network)->appliesToUser(uid);
+ }
+ VirtualNetwork* virtualNetwork = getVirtualNetworkForUserLocked(uid);
+ if (virtualNetwork && virtualNetwork->isSecure() &&
+ mProtectableUsers.find(uid) == mProtectableUsers.end()) {
+ return false;
+ }
+ Permission networkPermission = static_cast<PhysicalNetwork*>(network)->getPermission();
+ return (userPermission & networkPermission) == networkPermission;
+}
+
int NetworkController::modifyRoute(unsigned netId, const char* interface, const char* destination,
const char* nexthop, bool add, bool legacy, uid_t uid) {
unsigned existingNetId = getNetworkForInterface(interface);