From a1067c8d2b2165f1058a3a8216bed4efacfa1c80 Mon Sep 17 00:00:00 2001
From: Lorenzo Colitti <lorenzo@google.com>
Date: Thu, 2 Oct 2014 22:47:41 +0900
Subject: [PATCH] Improve error return values on network selection.

It's very confusing to see EPERM when opening or connecting a
socket when the problem is not security-related. In the (common)
case where an app cannot select a network because it does not
exist, return ENONET ("Machine is not on network") instead.

Also, return EREMOTEIO for when we can't figure out who the user
is, and use EPERM for VPN denials and EACCES for permission
bits.

Bug: 17702933
Change-Id: Ia680c485e0ea1efad1ad374231d994e9bfd4cd5a
---
 server/FwmarkServer.cpp      |  8 +++++---
 server/NetworkController.cpp | 24 ++++++++++++++----------
 server/NetworkController.h   |  4 ++--
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/server/FwmarkServer.cpp b/server/FwmarkServer.cpp
index 8bf8b71..b11e075 100644
--- a/server/FwmarkServer.cpp
+++ b/server/FwmarkServer.cpp
@@ -151,11 +151,13 @@ int FwmarkServer::processClient(SocketClient* client, int* socketFd) {
                 fwmark.explicitlySelected = false;
                 fwmark.protectedFromVpn = false;
                 permission = PERMISSION_NONE;
-            } else if (mNetworkController->canUserSelectNetwork(client->getUid(), command.netId)) {
+            } else {
+                if (int ret = mNetworkController->checkUserNetworkAccess(client->getUid(),
+                                                                         command.netId)) {
+                    return ret;
+                }
                 fwmark.explicitlySelected = true;
                 fwmark.protectedFromVpn = mNetworkController->canProtect(client->getUid());
-            } else {
-                return -EPERM;
             }
             break;
         }
diff --git a/server/NetworkController.cpp b/server/NetworkController.cpp
index 7382617..20d8e97 100644
--- a/server/NetworkController.cpp
+++ b/server/NetworkController.cpp
@@ -181,7 +181,7 @@ uint32_t NetworkController::getNetworkForDns(unsigned* netId, uid_t uid) const {
     Fwmark fwmark;
     fwmark.protectedFromVpn = true;
     fwmark.permission = PERMISSION_SYSTEM;
-    if (canUserSelectNetworkLocked(uid, *netId)) {
+    if (checkUserNetworkAccessLocked(uid, *netId) == 0) {
         // 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.
@@ -373,9 +373,9 @@ void NetworkController::setPermissionForUsers(Permission permission,
     }
 }
 
-bool NetworkController::canUserSelectNetwork(uid_t uid, unsigned netId) const {
+int NetworkController::checkUserNetworkAccess(uid_t uid, unsigned netId) const {
     android::RWLock::AutoRLock lock(mRWLock);
-    return canUserSelectNetworkLocked(uid, netId);
+    return checkUserNetworkAccessLocked(uid, netId);
 }
 
 int NetworkController::setPermissionForNetworks(Permission permission,
@@ -493,27 +493,31 @@ Permission NetworkController::getPermissionForUserLocked(uid_t uid) const {
     return uid < FIRST_APPLICATION_UID ? PERMISSION_SYSTEM : PERMISSION_NONE;
 }
 
-bool NetworkController::canUserSelectNetworkLocked(uid_t uid, unsigned netId) const {
+int NetworkController::checkUserNetworkAccessLocked(uid_t uid, unsigned netId) const {
     Network* network = getNetworkLocked(netId);
+    if (!network) {
+        return -ENONET;
+    }
+
     // 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;
+    if (uid == INVALID_UID) {
+        return -EREMOTEIO;
     }
     Permission userPermission = getPermissionForUserLocked(uid);
     if ((userPermission & PERMISSION_SYSTEM) == PERMISSION_SYSTEM) {
-        return true;
+        return 0;
     }
     if (network->getType() == Network::VIRTUAL) {
-        return static_cast<VirtualNetwork*>(network)->appliesToUser(uid);
+        return static_cast<VirtualNetwork*>(network)->appliesToUser(uid) ? 0 : -EPERM;
     }
     VirtualNetwork* virtualNetwork = getVirtualNetworkForUserLocked(uid);
     if (virtualNetwork && virtualNetwork->isSecure() &&
             mProtectableUsers.find(uid) == mProtectableUsers.end()) {
-        return false;
+        return -EPERM;
     }
     Permission networkPermission = static_cast<PhysicalNetwork*>(network)->getPermission();
-    return (userPermission & networkPermission) == networkPermission;
+    return ((userPermission & networkPermission) == networkPermission) ? 0 : -EACCES;
 }
 
 int NetworkController::modifyRoute(unsigned netId, const char* interface, const char* destination,
diff --git a/server/NetworkController.h b/server/NetworkController.h
index d6f9a6b..5596f0c 100644
--- a/server/NetworkController.h
+++ b/server/NetworkController.h
@@ -67,7 +67,7 @@ public:
 
     Permission getPermissionForUser(uid_t uid) const;
     void setPermissionForUsers(Permission permission, const std::vector<uid_t>& uids);
-    bool canUserSelectNetwork(uid_t uid, unsigned netId) const;
+    int checkUserNetworkAccess(uid_t uid, unsigned netId) const;
     int setPermissionForNetworks(Permission permission,
                                  const std::vector<unsigned>& netIds) WARN_UNUSED_RESULT;
 
@@ -93,7 +93,7 @@ private:
     Network* getNetworkLocked(unsigned netId) const;
     VirtualNetwork* getVirtualNetworkForUserLocked(uid_t uid) const;
     Permission getPermissionForUserLocked(uid_t uid) const;
-    bool canUserSelectNetworkLocked(uid_t uid, unsigned netId) const;
+    int checkUserNetworkAccessLocked(uid_t uid, unsigned netId) const;
 
     int modifyRoute(unsigned netId, const char* interface, const char* destination,
                     const char* nexthop, bool add, bool legacy, uid_t uid) WARN_UNUSED_RESULT;
-- 
2.11.0