Fix multiple DNS leaks

server/NetdNativeService.cpp

@@ -372,6 +372,11 @@ binder::Status NetdNativeService::networkRejectNonSecureVpn(
     } else {
         err = RouteController::removeUsersFromRejectNonSecureNetworkRule(uidRanges);
     }
+
+    if (!err) {
+        err = gCtls->netCtrl.updateLockdownVpnUids(add, uidRanges);
+    }
+
     return statusFromErrcode(err);
 }
 

server/NetworkController.cpp

@@ -50,6 +50,7 @@
 #define DBG 0
 
 #include <android_net_platform_flags.h>
+#include <Fwmark.h>
 
 namespace netflags = android::net::platform::flags;
 
@@ -212,13 +213,40 @@ int NetworkController::setDefaultNetwork(unsigned netId) {
     return 0;
 }
 
+// This complements the default DNS server leak solution added in getNetworkForDnsLocked(). This
+// gets called much earlier in the DNS request execution. Both solutions do currently suffice on
+// their own.
+bool NetworkController::checkLockdownVpnBlockingDns(android_net_context* netcontext) const {
+    ScopedRLock lock(mRWLock);
+
+    uid_t uid = netcontext->uid;
+
+    Fwmark dnsMark;
+    dnsMark.intValue = netcontext->dns_mark;
+
+    if (dnsMark.protectedFromVpn || !mVpnLockdownUids.hasUid(uid)) {
+        return false;
+    }
+
+    VirtualNetwork* userVirtualNetwork = getVirtualNetworkForUserLocked(uid);
+    return userVirtualNetwork == nullptr || userVirtualNetwork->getNetId() != netcontext->dns_netid;
+}
+
 uint32_t NetworkController::getNetworkForDnsLocked(unsigned* netId, uid_t uid) const {
     Fwmark fwmark;
     fwmark.protectedFromVpn = canProtectLocked(uid, *netId);
     fwmark.permission = getPermissionForUserLocked(uid);
 
     Network* appDefaultNetwork = getPhysicalOrUnreachableNetworkForUserLocked(uid);
     unsigned defaultNetId = appDefaultNetwork ? appDefaultNetwork->getNetId() : mDefaultNetId;
+    if (!fwmark.protectedFromVpn && mVpnLockdownUids.hasUid(uid)) {
+        // If a uid is under a lockdown VPN, using the default network DNS servers is a DNS leak.
+        //
+        // The servers to be used for a resolution are set in resolv_populate_res_for_net(). There
+        // is no NetConfig for NETID_UNSET, so no servers will get set. Requests are terminated in
+        // res_nsend() where -ESRCH is returned due to the lack of servers.
+        defaultNetId = NETID_UNSET;
+    }
 
     // Common case: there is no VPN that applies to the user, and the query did not specify a netId.
     // Therefore, it is safe to set the explicit bit on this query and skip all the complex logic
@@ -242,8 +270,14 @@ uint32_t NetworkController::getNetworkForDnsLocked(unsigned* netId, uid_t uid) c
         // servers (through the default network). Otherwise, the query is guaranteed to fail.
         // http://b/29498052
         Network *network = getNetworkLocked(*netId);
-        if (network && network->isVirtual() && !resolv_has_nameservers(*netId)) {
-            *netId = defaultNetId;
+        if (network && network->isVirtual()) {
+            if (!resolv_has_nameservers(*netId)) {
+                // If the VPN app itself gets here then it has made a mistake. If it wouldn't cause
+                // compatibility issues, we could save it from leaking.
+                *netId = defaultNetId;
+            } else {
+                fwmark.protectedFromVpn = false;
+            }
         }
     } else {
         // If the user is subject to a VPN and the VPN provides DNS servers, use those servers
@@ -253,7 +287,7 @@ uint32_t NetworkController::getNetworkForDnsLocked(unsigned* netId, uid_t uid) c
         VirtualNetwork* virtualNetwork = getVirtualNetworkForUserLocked(uid);
         if (virtualNetwork && resolv_has_nameservers(virtualNetwork->getNetId())) {
             *netId = virtualNetwork->getNetId();
-            fwmark.explicitlySelected = true;
+            fwmark.protectedFromVpn = false;
         } else {
             // TODO: return an error instead of silently doing the DNS lookup on the wrong network.
             // http://b/27560555
@@ -660,6 +694,23 @@ int NetworkController::removeUsersFromNetwork(unsigned netId, const UidRanges& u
     return network->removeUsers(uidRanges, subPriority);
 }
 
+int NetworkController::updateLockdownVpnUids(bool add, const UidRanges& uidRanges) {
+    ScopedWLock lock(mRWLock);
+    if (add) {
+        mVpnLockdownUids.add(uidRanges);
+        // The caller should not send overlapping ranges, and if they do then hasUid won't work
+        // properly and a leak could occur.
+        if (mVpnLockdownUids.overlapsSelf()) {
+            ALOGE("Lockdown VPN uid ranges are overlapping: %s",
+                  mVpnLockdownUids.toString().c_str());
+            return -EINVAL;
+        }
+    } else {
+        mVpnLockdownUids.remove(uidRanges);
+    }
+    return 0;
+}
+
 int NetworkController::addRoute(unsigned netId, const char* interface, const char* destination,
                                 const char* nexthop, bool legacy, uid_t uid, int mtu,
                                 bool isLocalRoute) {

server/NetworkController.h

@@ -101,6 +101,7 @@ class NetworkController {
     unsigned getNetworkForUser(uid_t uid) const;
     unsigned getNetworkForConnect(uid_t uid) const;
     void getNetworkContext(unsigned netId, uid_t uid, struct android_net_context* netcontext) const;
+    bool checkLockdownVpnBlockingDns(android_net_context* netcontext) const;
     unsigned getNetworkForInterface(const char* interface) const;
     unsigned getNetworkForInterface(const int ifIndex) const;
     bool isVirtualNetwork(unsigned netId) const;
@@ -124,6 +125,7 @@ class NetworkController {
                                         int32_t subPriority);
     [[nodiscard]] int removeUsersFromNetwork(unsigned netId, const UidRanges& uidRanges,
                                              int32_t subPriority);
+    [[nodiscard]] int updateLockdownVpnUids(bool add, const UidRanges& uidRanges);
 
     // |nexthop| can be NULL (to indicate a directly-connected route), "unreachable" (to indicate a
     // route that's blocked), "throw" (to indicate the lack of a match), or a regular IP address.
@@ -211,6 +213,8 @@ class NetworkController {
     // we should fix it.
     // This map is deprecated, if flag connectivityServiceDestroySocket is enabled.
     std::unordered_map<std::string, std::unordered_set<unsigned>> mAddressToIfindices;
+
+    UidRanges mVpnLockdownUids;
 };
 
 }  // namespace android::net

server/main.cpp

@@ -107,13 +107,18 @@ bool evaluateDomainNameCallback(const android_net_context& netcontext, const cha
     return true;
 }
 
+bool checkLockdownVpnBlockingDnsCallback(android_net_context* netcontext) {
+    return gCtls->netCtrl.checkLockdownVpnBlockingDns(netcontext);
+}
+
 bool initDnsResolver() {
     ResolverNetdCallbacks callbacks = {
             .check_calling_permission = &checkCallingPermissionCallback,
             .get_network_context = &getNetworkContextCallback,
             .log = &logCallback,
             .tagSocket = &tagSocketCallback,
             .evaluate_domain_name = &evaluateDomainNameCallback,
+            .check_lockdown_vpn_blocking_dns = &checkLockdownVpnBlockingDnsCallback,
     };
     return resolv_init(&callbacks);
 }