Improve threads' privilege synchronisation

* Drop the caps after the threads have been listed for a second time
  (after the sync). This is to avoid errors during accessing /proc for
  newly spawned threads as a unprivileged process.
* Check if newly spawned threads have correct labels.
* Retry the privileges sync twice for all remaining privileged threads.
* Retry listing of /proc/self/task/ in case of failure.
* Use set instead of vector for easier tid checks.
* Omit main thread from the list.

Change-Id: I21e7e5dd3d5efb70fe51a1597bd7bc4ccf1099e8

3 files changed, 187 insertions, 105 deletions

diff --git a/src/client/check-proper-drop.cpp b/src/client/check-proper-drop.cpp
index d8baeae5..3159979a 100644
--- a/src/client/check-proper-drop.cpp
+++ b/src/client/check-proper-drop.cpp
@@ -24,7 +24,6 @@
  */
 
 #include <string>
-#include <vector>
 
 #include <check-proper-drop.h>
 #include <dpl/exception.h>
@@ -40,22 +39,38 @@ DECLARE_EXCEPTION_TYPE(SecurityManager::Exception, DropError)
 
 }  // namespace
 
-void checkThreads(const std::vector<int> &synced_tids)
+std::unordered_set<int> checkNewThreads(const std::unordered_set<int>& ignored)
 {
-    auto current_tids = FS::getSubDirectoriesFromDirectory("/proc/self/task", true);
+    FS::FileNameVector current_tids;
+    for (unsigned i = 0; i < 10; ++i) {
+        try {
+            current_tids = FS::getSubDirectoriesFromDirectory("/proc/self/task", true);
+            break;
+        } catch(const FS::Exception::FileError&) {
+            if (i == 9)
+                throw;
+
+            /*
+             *  This may happen if a thread disappears, an entry is removed from /proc and fstatat
+             *  fails.
+             */
+            LogWarning("Reading /proc/self/task/ failed. Retrying.");
+        }
+    }
+
+    std::unordered_set<int> result;
     for (auto &tid_s : current_tids) {
         int tid = atoi(tid_s.c_str());
-        bool found = false;
-        for (auto &synced_tid : synced_tids)
-            if (synced_tid == tid) {
-                found = true;
-                break;
-            }
-        if (!found) {
-            LogError("New thread found that was not synchronized, must have not been dropped! TID: " << tid);
-            ThrowMsg(DropError, "New thread found that was not synchronized, must have not been dropped! TID: " << tid);
+
+        if (ignored.count(tid) > 0) {
+            LogDebug("Thread " << tid_s << " ignored");
+            continue;
         }
+
+        LogDebug("Thread " << tid_s << " new");
+        result.emplace(tid);
     }
+    return result;
 }
 
 }  // namespace CheckProperDrop
diff --git a/src/client/client-security-manager.cpp b/src/client/client-security-manager.cpp
index 2f785901..7725f248 100644
--- a/src/client/client-security-manager.cpp
+++ b/src/client/client-security-manager.cpp
@@ -114,6 +114,31 @@ static int g_all_tid_num;
 // Hackish, based on glibc's definition in sysdeps/unix/sysv/linux/nptl-signals.h
 #define SIGSETXID           (__SIGRTMIN + 1)
 
+std::string readThreadLabel(int tid)
+{
+    std::string path = "/proc/self/task/" + std::to_string(tid) + "/attr/current";
+    char label[256];
+    int fd;
+    int ret;
+    label[0] = 0;
+
+    fd = open(path.c_str(), O_RDONLY);
+    if (fd <= 0) {
+        LogWarning("Failed to open " << path);
+        return label;
+    }
+
+    ret = read(fd, label, sizeof(label) - 1);
+    close(fd);
+    if (ret <    0) {
+        LogWarning("Failed to read " << path);
+        return label;
+    }
+    label[ret] = '\0';
+    return label;
+}
+
+
 SECURITY_MANAGER_API
 const char *security_manager_strerror(enum lib_retcode rc)
 {
@@ -606,12 +631,14 @@ inline static int label_for_self_internal(int tid)
     return ret < 0 ? -1 : 0;
 }
 
-static inline int security_manager_sync_threads_internal(const std::string &app_label, std::vector<int> &synced_tids)
+static inline int security_manager_sync_threads_internal(const std::string &app_label)
 {
     static_assert(ATOMIC_INT_LOCK_FREE == 2, "std::atomic<int> is not always lock free");
     g_tid_status = NULL;
 
-    FS::FileNameVector files = FS::getSubDirectoriesFromDirectory("/proc/self/task", true);
+    // no need to sync the main thread
+    std::unordered_set<int> tids_ignored = {Syscall::gettid()};
+    auto tids_before_sync = CheckProperDrop::checkNewThreads(tids_ignored);
 
     g_cap = cap_init();
 
@@ -626,14 +653,14 @@ static inline int security_manager_sync_threads_internal(const std::string &app_
     if (smack_check())
         g_p_app_label = &app_label;
 
-    LogDebug("number of threads to sync: " << files.size() - 1);
+    for (unsigned attempt = 0;;++attempt) {
+        LogDebug("number of threads to sync: " << tids_before_sync.size());
 
-    const int cur_tid = Syscall::gettid();
-    synced_tids.push_back(cur_tid);
+        if (tids_before_sync.empty())
+            break; // nothing to do
 
-    if (files.size() > 1) {
         const pid_t cur_pid = getpid();
-        g_threads_count = g_all_tid_num = files.size() - 1;
+        g_threads_count = g_all_tid_num = tids_before_sync.size();
         g_tid_status = new TidStatus[g_all_tid_num];
         if (!g_tid_status) {
             LogError("Cannot allocate g_tid_status");
@@ -664,6 +691,10 @@ static inline int security_manager_sync_threads_internal(const std::string &app_
                     break;
                 }
             }
+            // The thread that received the signal is not on the list -> this shouldn't happen
+            if (i == g_all_tid_num)
+                abort();
+
             g_tid_status[i].status = TID_STATUS_RECEIVED_SIGNAL;
             if (g_p_app_label)
                 if (label_for_self_internal(tid) != 0)
@@ -685,73 +716,150 @@ static inline int security_manager_sync_threads_internal(const std::string &app_
 
         int threads_gone = 0;
         int tid_index = 0;
-        for (auto const &e : files) {
-            const int tid = atoi(e.c_str());
+        for (auto it = tids_before_sync.begin(); it != tids_before_sync.end();) {
+            g_tid_status[tid_index++].tid = *it;
 
-            if (tid == cur_tid)
-                continue;
-            synced_tids.push_back(tid); // this will not add current TID (but its already added)
-            g_tid_status[tid_index++].tid = tid;
-
-            if (Syscall::tgkill(cur_pid, tid, SIGSETXID) < 0) {
+            if (Syscall::tgkill(cur_pid, *it, SIGSETXID) < 0) {
                 const auto err = errno;
                 if (ESRCH == err) { // thread already gone
                     threads_gone++;
                     g_tid_status[tid_index-1].status = TID_STATUS_DEAD;
+                    it = tids_before_sync.erase(it);
+                    continue;
                 } else
                     LogWithErrno(err, "tgkill()");
-
-                continue;
             }
+            ++it;
         }
 
         g_threads_count -= threads_gone;
         LogDebug("number of threads already gone (signals unsent): " << threads_gone);
 
-        unsigned counter = 0;
         for (int i = MAX_SIG_WAIT_TIME; g_threads_count && i; i--) {
+            if (i % 500 == 0)
+                LogWarning("Not all threads synchronized yet, still waiting - threads left: " <<
+                           g_threads_count);
             usleep(2000);   // 2 ms
-            counter++;
-            if (counter == 500) {
-                counter = 0; // reset counter each ~1 second and add warning log
-                LogWarning("Not all threads synchronized yet, still waiting - threads left: " << g_threads_count);
-            }
         }
 
         Syscall::sigaction(SIGSETXID, &old, nullptr);
 
+        /*
+         * Listing thread ids here (before cap drop) we can ommit issues like Smack access error on
+         * new spawned threads in /proc.
+         */
+        auto tids_after_sync = CheckProperDrop::checkNewThreads(tids_ignored);
         if (g_threads_count) {
-            LogError("Not all threads synchronized: threads left: " << g_threads_count);
+            // Here we have to check if threads that were supposedly not synchronized, are not actually dead now;
+            // they can still be NOT dead in g_tid_status[i].status because at signal sending time they could still live;
+            // such an issue also happened in VD's testing at least once
+            int additional_deaths = 0;
             for (int i = 0; i < g_all_tid_num; ++i) {
-                switch (g_tid_status[i].status) {
-                    case TID_STATUS_DEAD:
-                        /* this is totally fine */
-                        LogError("Thread " << g_tid_status[i].tid << " died during attempt to send signal - info can be useful, but this is OK");
-                        break;
-                    case TID_STATUS_NOT_SYNCED:
-                        LogError("Thread " << g_tid_status[i].tid << " didn't receive the signal, serious ERROR!!!");
-                        break;
-                    case TID_STATUS_RECEIVED_SIGNAL:
-                        LogError("Thread " << g_tid_status[i].tid << " received signal but failed at Smack label change, serious ERROR!!!");
-                        break;
-                    case TID_STATUS_SETUP_SMACK:
-                        LogError("Thread " << g_tid_status[i].tid << " received signal, changed Smack label, couldn't change caps, serious ERROR!!!");
-                        break;
-                    case TID_STATUS_FULLY_SYNCED:
-                        /* this is totally fine */
-                        LogError("Thread " << g_tid_status[i].tid << " received signal and was fully synchronized - info can be useful, but this is OK");
-                        break;
-                    default:
-                        LogError("Thread " << g_tid_status[i].tid << " has strange status: " << g_tid_status[i].status << " this is an ERROR!!!");
-                        break;
+                int tid = g_tid_status[i].tid;
+
+                if (tids_after_sync.count(tid) == 0) {
+                    // TID is actually dead!
+                    ++additional_deaths;
+                    g_tid_status[i].status = TID_STATUS_DEAD;
                 }
             }
-            LogError("Aborting - please check coredump and check offending threads!");
-            abort(); // needed to debug the offending thread(s)
+
+            if(g_threads_count - additional_deaths > 0) {
+                LogError("Not all threads synchronized: threads left: " <<
+                         g_threads_count - additional_deaths);
+
+                for (int i = 0; i < g_all_tid_num; ++i) {
+                    switch (g_tid_status[i].status) {
+                        case TID_STATUS_DEAD:
+                            /* this is totally fine */
+                            LogError("Thread " << g_tid_status[i].tid << " died during attempt to send signal - info can be useful, but this is OK");
+                            break;
+                        case TID_STATUS_NOT_SYNCED:
+                            LogError("Thread " << g_tid_status[i].tid << " didn't receive the signal, serious ERROR!!!");
+                            break;
+                        case TID_STATUS_RECEIVED_SIGNAL:
+                            LogError("Thread " << g_tid_status[i].tid << " received signal but failed at Smack label change, serious ERROR!!!");
+                            break;
+                        case TID_STATUS_SETUP_SMACK:
+                            LogError("Thread " << g_tid_status[i].tid << " received signal, changed Smack label, couldn't change caps, serious ERROR!!!");
+                            break;
+                        case TID_STATUS_FULLY_SYNCED:
+                            /* this is totally fine */
+                            LogError("Thread " << g_tid_status[i].tid << " received signal and was fully synchronized - info can be useful, but this is OK");
+                            break;
+                        default:
+                            LogError("Thread " << g_tid_status[i].tid << " has strange status: " << g_tid_status[i].status << " this is an ERROR!!!");
+                            break;
+                    }
+                }
+                LogError("Aborting - please check coredump and check offending threads!");
+                abort(); // needed to debug the offending thread(s)
+            }
         }
+
+        /*
+         * Reaching here, we can assume all threads that were read from /proc previously are synced,
+         * except current thead. What's left are threads that were spawned by other threads after
+         * we listed them using the CheckProperDrop::checkNewThreads() for the first time.
+         */
+        for (const auto& tid : tids_before_sync)
+            tids_after_sync.erase(tid);
+
+        LogDebug("New threads left after synchronisation " << tids_after_sync.size());
+
+        /*
+         * Check if any of the remaining threads has a correct smack label. If that's the case move
+         * it to ignored ones. During experiments the label was either the original (privileged)
+         * one, the desired app label (g_p_app_label) or it was unreadable at all.
+         *
+         * This is necessary as threads with dropped capabilities may spawn new threads too. Newly
+         * spawned threads have dropped capabilities as well so we don't need to bother with them
+         * and definitely we don't want to abort because of them.
+         */
+        if (g_p_app_label) {
+            for (auto it = tids_after_sync.begin(); it != tids_after_sync.end();) {
+                auto label = readThreadLabel(*it);
+                if (label == *g_p_app_label) {
+                    tids_ignored.emplace(*it);
+                    it = tids_after_sync.erase(it);
+                } else {
+                    LogDebug("Privileged thread: " << *it << " label: '" << label << "' expected: '" << *g_p_app_label << "'");
+                    it++;
+                }
+            }
+        }
+
+        LogDebug("Remaining privileged threads " << tids_after_sync.size());
+
+        if (tids_after_sync.empty())
+            break; // all done
+
+        if (attempt < 2) {
+            /*
+             * Move synced threads to ignored pool and retry. 2 retries were enough to handle 300
+             * recursively spawned threads in security_manager_300_prepare_app_recursive_threads.
+             * If necessary we can increase it.
+             */
+            LogDebug("Retrying");
+            tids_ignored.merge(std::move(tids_before_sync));
+            tids_before_sync = std::move(tids_after_sync);
+            continue;
+        }
+
+        /*
+         * If app candidate is so stubborn it continues to spawn threads, we cannot iterate this
+         * forever and need to treat that filthy bastard with proper abort().
+         */
+        for (auto &tid : tids_after_sync)
+            LogError("New thread spawned during call to prepare_app that could not get synchronized, TID: " << tid);
+
+        abort(); // needed to debug the offending thread(s).
     }
 
-    if (g_p_app_label && label_for_self_internal(cur_tid) != 0) {
+    /*
+     * The last step of successfull call - change attributes of one last thread, the main thread.
+     */
+    if (g_p_app_label && label_for_self_internal(Syscall::gettid()) != 0) {
         LogError("label_for_self_internal failed");
         return SECURITY_MANAGER_ERROR_UNKNOWN;
     }
@@ -1062,7 +1170,6 @@ int security_manager_prepare_app2(const char *app_name, const char *subsession_i
         PrepareAppFlags prepareAppFlags;
         std::vector<gid_t> forbiddenGroups, allowedGroups;
         std::vector<bool> privPathsStatusVector;
-        std::vector<int> synced_tids;
         auto privilegePathMap = MountNS::getPrivilegePathMap(getuid());
         int ret = prepareAppInitialSetupAndFetch(app_name, privilegePathMap, appLabel, pkgName, prepareAppFlags,
                 forbiddenGroups, allowedGroups, privPathsStatusVector);
@@ -1084,53 +1191,12 @@ int security_manager_prepare_app2(const char *app_name, const char *subsession_i
             return ret;
         }
 
-        ret = security_manager_sync_threads_internal(appLabel, synced_tids);
+        ret = security_manager_sync_threads_internal(appLabel);
         if (ret != SECURITY_MANAGER_SUCCESS) {
             LogError("Can't properly setup application threads (Smack label & capabilities) for application " << app_name);
             return ret;
         }
 
-        try {
-            /*
-             * At this point in time, threads detected at security_manager_sync_threads_internal() stage should either:
-             * - be all synchronized (changed Smack label & capabilities)
-             * - be dead (if threads successfully ended)
-             * For those TIDs that are still alive, if their TIDs were on the list during call to sync_threads_internal(),
-             * we can be sure permissions are properly dropped - label_for_self_internal and cap_set_proc() must have finished
-             * with success for those threads, if we reached this point.
-             *
-             * If a thread is dead somehow, but was on the list - it doesn't pose security issue.
-             *
-             * A security issue exists only if a NEW thread was spawned that doesn't exist on the list from
-             * sync_threads_internal(). It can have improper Smack label & caps, so we cannot continue.
-             * Also, we can't reliably check contents of /proc to verify Smack labels, as it can contain
-             * stale information about processes Smack labels even after successful completion of set_label_for_self() function.
-             *
-             * Linux uses sequential TID/PID assignment, so theoretically TIDs can get re-used, but not in such small timeframe,
-             * so we're not taking this issue (TID reusage by kernel) into account here.
-             *
-             * Below call will throw an exception if a new thread will be detected thats not on the original list.
-             *
-             * Previous implementation that actually checked contents of /proc (Smack labels & caps) was dropped
-             * as it was verified experimentally that the proc filesystem could contain stale information about processes
-             * Smack labels/permissions even after successful completion of smack_set_label_for_self() & cap_set_proc() functions.
-             */
-            CheckProperDrop::checkThreads(synced_tids);
-        } catch (...) {
-            LogError("Privileges haven't been properly dropped for the whole process of application " << app_name);
-            /*
-             * Not all app candidate processes behave properly and some may want to spawn
-             * new threads during this API call. This abort() below makes sure the process will:
-             *
-             * 1) not allow actual applicaton run in environment where there's a thread that still has high privileges
-             * 2) make sure a coredump will be created so that it can be analyzed later which thread caused troubles
-             *
-             * This will effectively block possible privilege escalation in application due to improper setup of
-             * the candidate process.
-             */
-            abort();
-        }
-
         LogWarning("security_manager_prepare_app2() finished with return code " << ret);
 
         return ret;
diff --git a/src/common/include/check-proper-drop.h b/src/common/include/check-proper-drop.h
index 0d64447e..8a519549 100644
--- a/src/common/include/check-proper-drop.h
+++ b/src/common/include/check-proper-drop.h
@@ -26,7 +26,7 @@
 #pragma once
 
 #include <utils.h>
-#include <vector>
+#include <unordered_set>
 
 namespace SecurityManager {
 namespace CheckProperDrop {
@@ -47,7 +47,8 @@ int8_t computeFlags();
 
 
 // CLIENT ONLY
-void checkThreads(const std::vector<int> &synced_tids);
+// returns threads TIDS
+std::unordered_set<int> checkNewThreads(const std::unordered_set<int>& ingored);
 
 }  // namespace CheckProperDrop
 }  // namespace SecurityManager