Fix enterMountNamespace() error handling.

There is a TOCTOU race condition between checking/entering app namespaces.
In this small time window, app can be killed,
so updating app namespace doesn't make sense, we can skip this step.

Change-Id: I2ba4c2bd701d6fc5a453c72d19e1d951e39fde53

diff --git a/src/common/include/mount-namespace.h b/src/common/include/mount-namespace.h
index 49b5587edb36002095f52742dfe5346c373b8574..1189f177d9dfcd7cb0d0c83c68f1843705d28bb1 100644 (file)
--- a/src/common/include/mount-namespace.h
+++ b/src/common/include/mount-namespace.h
@@ -52,7 +52,7 @@ std::string getUserAppServiceMountPointPath(uid_t uid, const std::string &smackL
 int applyPrivilegePath(bool allow, const PrivilegePath &privilegePath);
 bool isPrivacyPrivilegeMountNamespaceEnabled(void);
 int createMountNamespace(void);
-bool enterMountNamespace(const Path &mntPath);
+int enterMountNamespace(const Path &mntPath);
 int makeMountSlave(const Path &mountPoint);
 int makeMountPrivate(const Path &mountPoint);
 int bindMountRW(const Path &source, const Path &target);

diff --git a/src/common/mount-namespace.cpp b/src/common/mount-namespace.cpp
index f9a48ea51a04329f97b82ea99a0855a6d0aa0ad2..bbd7fcb527926c510a3616193aa095a49a305b4f 100644 (file)
--- a/src/common/mount-namespace.cpp
+++ b/src/common/mount-namespace.cpp
@@ -160,17 +160,23 @@ int createMountNamespace(void)
     return SECURITY_MANAGER_SUCCESS;
 }
 
-bool enterMountNamespace(const Path &mntPath)
+int enterMountNamespace(const Path &mntPath)
 {
     int fd = open(mntPath.c_str(), O_RDONLY);
-    if (fd < 0)
-        return false;
+    if (fd < 0) {
+        LogError("Failed to open " << mntPath << ": " << GetErrnoString(errno));
+        return SECURITY_MANAGER_ERROR_FILE_OPEN_FAILED;
+    }
 
     // 0 == allow any type of namespace
     int ret = setns(fd, 0);
     close(fd);
+    if (ret != 0) {
+        LogError("Failed to setns " << mntPath << ": " << GetErrnoString(errno));
+        return SECURITY_MANAGER_ERROR_MOUNT_ERROR;
+    }
 
-    return ret >= 0;
+    return SECURITY_MANAGER_SUCCESS;
 }
 
 int makeMountSlave(const Path &mountPoint)

diff --git a/src/common/worker.cpp b/src/common/worker.cpp
index 70d0d4a6096454205510ab007f9a765b25c82d4c..a43f52decd9d2a94d81b0d8be10107b145d8b613 100644 (file)
--- a/src/common/worker.cpp
+++ b/src/common/worker.cpp
@@ -45,12 +45,15 @@ int Worker::doWork(const NSMountLogic::EntryVector &entries)
     for (auto &entry : entries) {
         try {
             auto appNamespace = MountNS::getUserAppServiceMountPointPath(entry.uid, entry.smackLabel, entry.pid);
-            if (MountNS::enterMountNamespace(appNamespace)) {
+            int ret = MountNS::enterMountNamespace(appNamespace);
+            if (ret == SECURITY_MANAGER_SUCCESS) {
                 inGlobalNamespace = false;
-            } else {
+            } else if (ret == SECURITY_MANAGER_ERROR_MOUNT_ERROR) {
                 status = -1;
                 LogError("Error entering app mount namespace. Environment of application: "
-                         << entry.smackLabel <<  "for user: " << entry.uid << " will not be setup correctly.");
+                         << entry.smackLabel <<  " for user: " << entry.uid << " will not be setup correctly.");
+                continue;
+            } else {
                 continue;
             }
 
@@ -72,18 +75,18 @@ int Worker::doWork(const NSMountLogic::EntryVector &entries)
                     }
                     if (SECURITY_MANAGER_SUCCESS != applyPrivilegePath(allowed, privilegePath)) {
                         status = -1;
-                        LogError("Environment of application: " << entry.smackLabel <<  "for user: "
+                        LogError("Environment of application: " << entry.smackLabel <<  " for user: "
                                  << entry.uid << " will not be setup correctly.");
                     }
                 }
             }
         } catch (...) {
             status = -1;
-            LogError("Environment of application: " << entry.smackLabel <<  "for user: "
+            LogError("Environment of application: " << entry.smackLabel <<  " for user: "
                      << entry.uid << " will not be setup correctly.");
         }
 
-        if (!inGlobalNamespace && !MountNS::enterMountNamespace(MountNS::MAIN_MOUNT_NAMESPACE)) {
+        if (!inGlobalNamespace && MountNS::enterMountNamespace(MountNS::MAIN_MOUNT_NAMESPACE) != SECURITY_MANAGER_SUCCESS) {
             status = -1;
             LogError("Error entering global mount namespace.");
         }