From: Konrad Lipinski Date: Mon, 15 Jun 2020 15:31:10 +0000 (+0200) Subject: Fix CheckProperDrop tests X-Git-Tag: submit/tizen/20200710.130420~1 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=refs%2Fchanges%2F18%2F236218%2F8;p=platform%2Fcore%2Fsecurity%2Fsecurity-manager.git Fix CheckProperDrop tests Moved into a separate commit at a reviewer's request. Accommodate the new implementation: * Run each test inside a fork() so that caps can be freely zeroed. * Add namespace unsharing, uid, gid and groups tests. Change-Id: Ic8c608b2cd301b2898cbcd3b1ae3dcc3f62cecda --- diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 6dd3e40..ec6e78f 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -66,7 +66,7 @@ SET(SM_TESTS_SOURCES ${SM_TEST_SRC}/test_service_impl_utils.cpp ${SM_TEST_SRC}/test_smack-labels.cpp ${SM_TEST_SRC}/test_smack-rules.cpp - #${SM_TEST_SRC}/test_check_proper_drop.cpp + ${SM_TEST_SRC}/test_check_proper_drop.cpp ${SM_TEST_SRC}/test_misc.cpp ${SM_TEST_SRC}/test_template-manager.cpp ${DPL_PATH}/core/src/assert.cpp @@ -81,6 +81,7 @@ SET(SM_TESTS_SOURCES ${DPL_PATH}/log/src/log.cpp ${DPL_PATH}/log/src/old_style_log_provider.cpp ${DPL_PATH}/log/src/sd_journal_provider.cpp + ${PROJECT_SOURCE_DIR}/src/common/check-proper-drop.cpp ${PROJECT_SOURCE_DIR}/src/common/config-file.cpp ${PROJECT_SOURCE_DIR}/src/common/credentials.cpp ${PROJECT_SOURCE_DIR}/src/common/filesystem.cpp diff --git a/test/test_check_proper_drop.cpp b/test/test_check_proper_drop.cpp index f44f266..cac35b6 100644 --- a/test/test_check_proper_drop.cpp +++ b/test/test_check_proper_drop.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020 Samsung Electronics Co., Ltd All Rights Reserved + * Copyright (c) 2020 Samsung Electronics Co., Ltd. All rights reserved * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,22 +20,23 @@ * @version 1.0 */ -#include -#include -#include -#include -#include +#include #include -#include +#include #include -#include +#include +#include +#include -#include +#include +#include #include +#include +#include #include -#include -#include -#include + +#include +#include #include #include @@ -44,33 +45,26 @@ using namespace SecurityManager; - namespace { -std::mutex mutex; -std::condition_variable cond; -std::condition_variable count; -unsigned counter = 0; -bool cancel = false; - -const ::size_t LABEL_SIZE = 255; - -const std::string NO_LABEL = ""; -// should work with any label if not for onlycaps in Tizen: -const std::string OTHER_LABEL = "User::Shell"; -const std::string ROGUE_LABEL = "rogue_label"; +template +Result logErrnoIfError(const IsError &isError, Result res, + const char *prettyFunction, unsigned line, const char *expression) +{ + if (isError(res)) + ::fprintf(stderr, "%s(%u) (%s) err (%m)\n", prettyFunction, line, expression); + return res; +} -using Caps = std::vector<::cap_value_t>; +#define logErrnoIf(...) logErrnoIfError([](auto r) { return r; }, \ + (__VA_ARGS__), __PRETTY_FUNCTION__, __LINE__, #__VA_ARGS__) +#define logErrnoIfNot(...) logErrnoIfError([](auto r) { return !r; }, \ + (__VA_ARGS__), __PRETTY_FUNCTION__, __LINE__, #__VA_ARGS__) +#define logErrnoIfNegative(...) logErrnoIfError([](auto r) { return r < 0; }, \ + (__VA_ARGS__), __PRETTY_FUNCTION__, __LINE__, #__VA_ARGS__) -const Caps NO_CAPS; -const Caps SMACK_CAPS = { - CAP_MAC_ADMIN, - CAP_MAC_OVERRIDE -}; -const Caps ROGUE_CAPS = { - CAP_NET_ADMIN, - CAP_SYS_ADMIN -}; +constexpr uid_t APP_UID = 5001; +constexpr gid_t APP_GID = 100; namespace fs = boost::filesystem; namespace ch = std::chrono; @@ -122,456 +116,237 @@ private: } }; -class Smack +CheckProperDrop::BitFlags getCpdFlags() { -public: - Smack(bool should_restore) - : restore(should_restore) - { - const ::pid_t tid = ::syscall(SYS_gettid); - path = "/proc/" + std::to_string(tid) + "/attr/current"; - } - - ~Smack() - { - if (saved_label.empty()) - return; - - if (restore) { - bool ret = set_self_label(saved_label); - if (!ret) { - std::cout - << "Failed to restore label, further tests might fail: " - << ::strerror(errno) << std::endl; - } - } - } - - bool set(const std::string& label) - { - if (saved_label.empty()) { - saved_label = get_self_label(); - if (saved_label.empty()) { - std::cout << "Failed to get current label: " - << ::strerror(errno) << std::endl; - return false; - } - } - - bool ret = set_self_label(label); - if (!ret) { - std::cout << "Failed to set new label: " - << ::strerror(errno) << std::endl; - return false; - } - - return true; - } - -private: - bool set_self_label(const std::string& label) - { - int fd = ::open(path.c_str(), O_WRONLY); - if (fd < 0) - return false; - - int ret = TEMP_FAILURE_RETRY(::write(fd, label.c_str(), label.length())); - - ::close(fd); - return ret == (::ssize_t)label.length(); - } - - std::string get_self_label() - { - char label[LABEL_SIZE + 1] = {0}; - - int fd = ::open(path.c_str(), O_RDONLY); - if (fd < 0) - return ""; - - TEMP_FAILURE_RETRY(::read(fd, label, LABEL_SIZE)); - - ::close(fd); - return label; - } - - std::string saved_label; - bool restore; - - std::string path; -}; + static const auto cpdFlags = CheckProperDrop::computeFlags(); + BOOST_REQUIRE(cpdFlags >= 0); + return cpdFlags; +} -class Privs +bool isProperDrop(CheckProperDrop::BitFlags cpdFlags) { -public: - Privs(bool should_restore) - : saved_cap(NULL), - restore(should_restore) - {} - - ~Privs() - { - if (saved_cap == NULL) - return; - - if (restore) { - int ret = ::cap_set_proc(saved_cap); - if (ret != 0) { - std::cout - << "Failed to restore capabilities, further tests might fail: " - << ::strerror(errno) << std::endl; - } - } - - ::cap_free(saved_cap); - } - - bool drop(const std::vector& to_drop) - { - int ret; - cap_t new_cap = NULL; - - if (saved_cap == NULL) { - saved_cap = ::cap_get_proc(); - if (saved_cap == NULL) { - std::cout << "Failed to get current capabilities: " - << ::strerror(errno) << std::endl; - goto fail; - } - } - - new_cap = ::cap_dup(saved_cap); - if (new_cap == NULL) { - std::cout << "Failed to duplicate capabilities: " - << ::strerror(errno) << std::endl; - goto fail; - } - - ret = ::cap_set_flag(new_cap, CAP_EFFECTIVE, to_drop.size(), - to_drop.data(), CAP_CLEAR); - if (ret != 0) { - std::cout << "Failed to configure capabilities: " - << ::strerror(errno) << std::endl; - goto fail; - } - - ret = ::cap_set_proc(new_cap); - if (ret != 0) { - std::cout << "Failed to drop capabilities: " - << ::strerror(errno) << std::endl; - goto fail; - } - - ::cap_free(new_cap); + try { + CheckProperDrop::checkThreads(cpdFlags); return true; - - fail: - ::cap_free(new_cap); + } catch (...) { return false; } +} -private: - cap_t saved_cap; - bool restore; -}; - -struct Thread { - Thread(const std::string& l, const Caps& c) - : label(l), - caps(c), - ret(false), - thread(std::thread(&Thread::thread_routine, this)) - {} - - ~Thread() - { - if (thread.joinable()) { - thread.join(); - } - } - - Thread(const Thread&) = delete; - Thread(Thread&& other) = default; - - Thread& operator=(const Thread&) = delete; - Thread& operator=(Thread&& other) = default; - - void thread_routine() - { - ret = false; - std::string id = "Thread: (unknown) "; - - try { - const ::pid_t tid = ::syscall(SYS_gettid); - id = "Thread: " + std::to_string(tid) + "/" + label + " "; - - std::cout << id << "reporting for duty" << std::endl; - - if (!label.empty()) { - Smack smack(false); - if (!smack.set(label)) { - std::cout << id << "failed to set label" << std::endl; - return; - } - } - - if (!caps.empty()) { - Privs privs(false); - if (!privs.drop(caps)) { - std::cout << id << "failed to drop privs" << std::endl; - return; - } - } - - { - std::unique_lock lock(mutex); - counter++; - count.notify_one(); - cond.wait(lock, [] { return cancel; }); - } - - std::cout << id << "properly canceled" << std::endl; - ret = true; - } catch (const std::exception &e) { - std::cout << id << "thrown: " << e.what() << std::endl; - } - } - - std::string label; - Caps caps; - - bool ret; - std::thread thread; -}; - -struct ThreadInfo { - std::string label; - Caps caps; -}; - -class Threads { -public: - Threads(const std::vector& thread_infos) - { - /* Reset the global variables before anything else. They're - * used to synchronize threads' start and cancel */ - counter = 0; - cancel = false; - - /* This is important, it's not only an optimization. Without - * this the vector will reallocate and move its objects while - * threads are running. */ - threads.reserve(thread_infos.size()); - - for (auto &ti: thread_infos) { - threads.emplace_back(ti.label, ti.caps); - } +bool dropCaps() +{ + cap_t cap = logErrnoIfNot(::cap_init()); + if (!cap) + return false; + bool capsDropped = logErrnoIf(::cap_set_proc(cap)) == 0; + ::cap_free(cap); + return capsDropped; +} - /* wait for all threads to launch */ - std::unique_lock lock(mutex); - bool ret = count.wait_for(lock, ch::seconds(3), - [this] { return counter == threads.size(); }); - if (!ret) { - std::cout << "Timed out waiting for threads" << std::endl; - } +void requireFork(const ::std::function &childProcess) +{ + pid_t pid = logErrnoIfNegative(::fork()); + BOOST_REQUIRE(pid >= 0); + if (pid > 0) { + int res; + BOOST_REQUIRE(logErrnoIfNegative(::wait(&res)) == pid); + BOOST_REQUIRE(WIFEXITED(res)); + BOOST_REQUIRE(WEXITSTATUS(res) == EXIT_SUCCESS); + return; } - ~Threads() - { - /* Emergency handle if cancel_threads_and_check_ret() was not - * called. Threads will join by themselves in their own - * destructors */ - if (!threads.empty()) { - cancel = true; - cond.notify_all(); - } + // Boost test registers some handlers (ex. SIGABRT). If the child process + // gets a SIGABRT, the handler causes the test suite to resume execution, + // resulting in subsequent tests being executed twice (in the parent and + // the child). + for (auto signum : { SIGHUP, SIGINT, SIGQUIT, SIGILL, SIGTRAP, SIGABRT, + SIGBUS, SIGFPE, SIGUSR1, SIGSEGV, SIGUSR2, SIGPIPE, SIGALRM, SIGTERM, + SIGSTKFLT, SIGCHLD, SIGCONT, SIGTSTP, SIGTTIN, SIGTTOU, SIGURG, + SIGXCPU, SIGXFSZ, SIGVTALRM, SIGPROF, SIGWINCH, SIGIO, SIGPWR, SIGSYS }) + ::signal(signum, SIG_DFL); + + // Catch all exceptions to hide them from boost test, thereby preventing + // it from interfering. An exception merely means that the test failed. + bool childProcessOk; + try { + childProcessOk = childProcess(); + } catch (...) { + childProcessOk = false; } - bool cancel_threads_and_check_ret() - { - { - std::unique_lock lock(mutex); - cancel = true; - cond.notify_all(); - } - - bool all_true = true; - for (auto &t: threads) { - t.thread.join(); - if (!t.ret) { - std::cout << "Thread with label: " << t.label - << " returned false" << std::endl; - all_true = false; - } - } + // bypass boost test atexit handlers + ::_exit(childProcessOk ? EXIT_SUCCESS : EXIT_FAILURE); +} - threads.clear(); - return all_true; - } +void requireThread(const ::std::function &alterThreadCreds, + bool isProper = false, CheckProperDrop::BitFlags cpdFlags = getCpdFlags()) +{ + requireFork([&] { + ::std::mutex mutex; + ::std::condition_variable cond; + bool kidOk = false; + bool nowKid = true; + + auto enter = [&](::std::unique_lock<::std::mutex> &lock, bool isKid) { + lock.lock(); + cond.wait(lock, [&]{ return isKid == nowKid; }); + }; + auto leave = [&](::std::unique_lock<::std::mutex> &lock, bool isKid) { + nowKid = !isKid; + lock.unlock(); + cond.notify_one(); + }; + + ::std::thread kid([&] { + kidOk = alterThreadCreds(); + + ::std::unique_lock<::std::mutex> lock(mutex); + leave(lock, true); // creds altered, signal parent + enter(lock, true); // parent done checking, terminate + }); + + ::std::unique_lock<::std::mutex> lock(mutex, ::std::defer_lock); + enter(lock, false); // kid has altered creds + bool ok = dropCaps() && isProperDrop(cpdFlags) == isProper; + + leave(lock, false); // check done, signal kid + kid.join(); + + return ok && kidOk; + }); +} -private: - std::vector threads; -}; +void requireThreadNs(const char *ns, int unshareFlag) +{ + size_t nsIdx = 0; + for (; nsIdx < CheckProperDrop::N_FLAG_BITS; nsIdx++) + if (!strcmp(ns, CheckProperDrop::NS_NAMES[nsIdx])) + break; + BOOST_REQUIRE(nsIdx < CheckProperDrop::N_FLAG_BITS); + auto cpdFlags = getCpdFlags(); + auto nsBit = 1 << nsIdx; + auto unshareThenDropCaps = [&] { + return logErrnoIf(::unshare(unshareFlag)) == 0 && dropCaps(); + }; + + if (cpdFlags & nsBit) + requireThread(unshareThenDropCaps); + else + ::fprintf(stderr, "%s namespace not included in checkProperDrop flags, " + "omitting failure test\n", ns); + + requireThread(unshareThenDropCaps, true, cpdFlags & ~nsBit); +} -} // namespace +} // namespace BOOST_AUTO_TEST_SUITE(CHECK_PROPER_DROP_TEST) -/* Everything should succeed */ POSITIVE_FIXTURE_TEST_CASE(T1700__no_threads, NoThreadsAssert) { - CheckProperDrop cpd; - bool ret; - - BOOST_REQUIRE_NO_THROW(ret = cpd.getThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE_NO_THROW(ret = cpd.checkThreads()); - BOOST_REQUIRE(ret == true); + auto cpdFlags = getCpdFlags(); + requireFork([&] { + return dropCaps() && isProperDrop(cpdFlags); + }); } -/* Everything should succeed */ -POSITIVE_FIXTURE_TEST_CASE(T1701__threads_unmodified, NoThreadsAssert) +// This may happen when a rogue thread: +// * Survives sync_threads_internal due to a lucky interleaving. +// * Uses capset() to restore main thread capabilities to nonzero. +// * Terminates before checkProperDrop() starts. +NEGATIVE_FIXTURE_TEST_CASE(T1701__no_threads_privileged, NoThreadsAssert) { - CheckProperDrop cpd; - bool ret; - - Threads t({{NO_LABEL, NO_CAPS}, - {NO_LABEL, NO_CAPS}, - {NO_LABEL, NO_CAPS}}); - - BOOST_REQUIRE_NO_THROW(ret = cpd.getThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE_NO_THROW(ret = cpd.checkThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE(t.cancel_threads_and_check_ret() == true); + auto cpdFlags = getCpdFlags(); + requireFork([&] { + return !isProperDrop(cpdFlags); + }); } -/* Everything should succeed */ -POSITIVE_FIXTURE_TEST_CASE(T1702__threads_unprivileged, NoThreadsAssert) +POSITIVE_FIXTURE_TEST_CASE(T1702__thread_unmodified, NoThreadsAssert) { - CheckProperDrop cpd; - bool ret; - Privs privs(true); - - Threads t({{NO_LABEL, SMACK_CAPS}, - {NO_LABEL, SMACK_CAPS}, - {NO_LABEL, SMACK_CAPS}}); - BOOST_REQUIRE(privs.drop(SMACK_CAPS) == true); - - BOOST_REQUIRE_NO_THROW(ret = cpd.getThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE_NO_THROW(ret = cpd.checkThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE(t.cancel_threads_and_check_ret() == true); + requireThread(dropCaps, true); } -/* Everything should succeed */ -POSITIVE_FIXTURE_TEST_CASE(T1704__threads_labeled, NoThreadsAssert) +// Caps not dropped. +NEGATIVE_FIXTURE_TEST_CASE(T1703__thread_privileged, NoThreadsAssert) { - CheckProperDrop cpd; - bool ret; - Smack smack(true); - - Threads t({{OTHER_LABEL, NO_CAPS}, - {OTHER_LABEL, NO_CAPS}, - {OTHER_LABEL, NO_CAPS}}); - BOOST_REQUIRE(smack.set(OTHER_LABEL) == true); - - BOOST_REQUIRE_NO_THROW(ret = cpd.getThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE_NO_THROW(ret = cpd.checkThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE(t.cancel_threads_and_check_ret() == true); + requireThread([]{ return true; }); } -/* Everything should succeed */ -POSITIVE_FIXTURE_TEST_CASE(T1704__threads_labeled_unprivileged, NoThreadsAssert) +// Wrong label. +NEGATIVE_FIXTURE_TEST_CASE(T1704__thread_mislabeled, NoThreadsAssert) { - CheckProperDrop cpd; - bool ret; - Smack smack(true); - Privs privs(true); - - Threads t({{OTHER_LABEL, SMACK_CAPS}, - {OTHER_LABEL, SMACK_CAPS}, - {OTHER_LABEL, SMACK_CAPS}}); - BOOST_REQUIRE(smack.set(OTHER_LABEL) == true); - BOOST_REQUIRE(privs.drop(SMACK_CAPS) == true); - - BOOST_REQUIRE_NO_THROW(ret = cpd.getThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE_NO_THROW(ret = cpd.checkThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE(t.cancel_threads_and_check_ret() == true); + requireThread([]{ + int fd = logErrnoIfNegative(TEMP_FAILURE_RETRY(::open( + ("/proc/" + ::std::to_string(syscall(SYS_gettid)) + "/attr/current").c_str(), + O_WRONLY))); + if (fd < 0) + return false; + static constexpr char label[] = "rogueLabel"; + static constexpr auto labelLen = ::strlen(label); + auto written = logErrnoIfNegative(TEMP_FAILURE_RETRY(::write(fd, label, labelLen))); + ::close(fd); + if (written != labelLen) { + if (written >= 0) + ::fprintf(stderr, "short smack label write, written=%u\n", unsigned(written)); + return false; + } + return dropCaps(); + }); } -/* checkThreads should fail due to different label between main - * thread and one of the children */ -NEGATIVE_FIXTURE_TEST_CASE(T1711__threads__rogue_label, NoThreadsAssert) +// Wrong mnt ns, failure iff ns included in CheckProperDrop::computeFlags(). +NEGATIVE_FIXTURE_TEST_CASE(T1705__thread_ns_mnt, NoThreadsAssert) { - CheckProperDrop cpd; - bool ret; - - Threads t({{ NO_LABEL, NO_CAPS}, - { NO_LABEL, NO_CAPS}, - {ROGUE_LABEL, NO_CAPS}}); - - BOOST_REQUIRE_NO_THROW(ret = cpd.getThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE_NO_THROW(ret = cpd.checkThreads()); - BOOST_REQUIRE(ret == false); - - BOOST_REQUIRE(t.cancel_threads_and_check_ret() == true); + requireThreadNs("mnt", CLONE_NEWNS); } -/* checkThreads should fail due to different caps between main - * thread and one of the children */ -NEGATIVE_FIXTURE_TEST_CASE(T1712__threads__rogue_caps, NoThreadsAssert) +NEGATIVE_FIXTURE_TEST_CASE(T1706__thread_ns_ipc, NoThreadsAssert) { - CheckProperDrop cpd; - bool ret; - - Threads t({{NO_LABEL, NO_CAPS}, - {NO_LABEL, NO_CAPS}, - {NO_LABEL, ROGUE_CAPS}}); - - BOOST_REQUIRE_NO_THROW(ret = cpd.getThreads()); - BOOST_REQUIRE(ret == true); - - BOOST_REQUIRE_NO_THROW(ret = cpd.checkThreads()); - BOOST_REQUIRE(ret == false); + requireThreadNs("ipc", CLONE_NEWIPC); +} - BOOST_REQUIRE(t.cancel_threads_and_check_ret() == true); +// Wrong net ns, failure iff ns included in CheckProperDrop::computeFlags(). +NEGATIVE_FIXTURE_TEST_CASE(T1707__thread_ns_net, NoThreadsAssert) +{ + requireThreadNs("net", CLONE_NEWNET); } -/* getThreads should fail due to the main thread having no access to - * one of the children */ -NEGATIVE_FIXTURE_TEST_CASE(T1721__threads_unprivileged__rogue_label, NoThreadsAssert) +// Wrong uts ns, failure iff ns included in CheckProperDrop::computeFlags(). +NEGATIVE_FIXTURE_TEST_CASE(T1708__thread_ns_uts, NoThreadsAssert) { - CheckProperDrop cpd; - bool ret; - Privs privs(true); + requireThreadNs("uts", CLONE_NEWUTS); +} - Threads t({{ NO_LABEL, SMACK_CAPS}, - { NO_LABEL, SMACK_CAPS}, - {ROGUE_LABEL, SMACK_CAPS}}); - BOOST_REQUIRE(privs.drop(SMACK_CAPS) == true); +// Wrong uid. +NEGATIVE_FIXTURE_TEST_CASE(T1709__thread_uid, NoThreadsAssert) +{ + requireThread([]{ + ::setfsuid(APP_UID); + return dropCaps(); + }); +} - BOOST_REQUIRE_NO_THROW(ret = cpd.getThreads()); - BOOST_REQUIRE(ret == false); +// Wrong gid. +NEGATIVE_FIXTURE_TEST_CASE(T1710__thread_gid, NoThreadsAssert) +{ + requireThread([]{ + ::setfsgid(APP_GID); + return dropCaps(); + }); +} - BOOST_REQUIRE(t.cancel_threads_and_check_ret() == true); +// Wrong supplementary groups. +NEGATIVE_FIXTURE_TEST_CASE(T1711__thread_groups, NoThreadsAssert) +{ + requireThread([]{ + ::gid_t dummy; + // Use setgroups syscall directly to affect this thread only + // (see man getgroups(2), NOTES, C library/kernel differences) + return logErrnoIf(static_cast(::syscall(SYS_setgroups, 0, &dummy))) == 0 + && dropCaps(); + }); } BOOST_AUTO_TEST_SUITE_END()