// 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)
{
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();
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");
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)
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;
}
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);
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;
projects / platform / core / security / security-manager.git / commitdiff