From b1e0fb389d0a8c2230985b3ea7aab7d363a7e403 Mon Sep 17 00:00:00 2001 From: Rafal Krypa Date: Thu, 14 Jan 2016 16:48:19 +0100 Subject: [PATCH] Add check if privileges were properly dropped Check if every thread in process has same stats as thread calling security_manager_prepare_app() and exit from process if they do not. Change-Id: I008c2b8e442edb6a5f9f1d74bf13f95465b6bdca Signed-off-by: Rafal Krypa --- packaging/security-manager.spec | 1 + src/client/CMakeLists.txt | 2 + src/client/check-proper-drop.cpp | 129 +++++++++++++++++++++++++++++++++ src/client/client-security-manager.cpp | 41 +++++++---- src/client/include/check-proper-drop.h | 76 +++++++++++++++++++ 5 files changed, 236 insertions(+), 13 deletions(-) create mode 100644 src/client/check-proper-drop.cpp create mode 100644 src/client/include/check-proper-drop.h diff --git a/packaging/security-manager.spec b/packaging/security-manager.spec index 20a84bd..82c27f2 100644 --- a/packaging/security-manager.spec +++ b/packaging/security-manager.spec @@ -15,6 +15,7 @@ BuildRequires: cmake BuildRequires: zip # BuildRequires: pkgconfig(dlog) BuildRequires: libattr-devel +BuildRequires: pkgconfig(libprocps) BuildRequires: pkgconfig(libsmack) BuildRequires: pkgconfig(libcap) BuildRequires: pkgconfig(libsystemd-daemon) diff --git a/src/client/CMakeLists.txt b/src/client/CMakeLists.txt index f934617..44d898f 100644 --- a/src/client/CMakeLists.txt +++ b/src/client/CMakeLists.txt @@ -2,6 +2,7 @@ PKG_CHECK_MODULES(CLIENT_DEP REQUIRED libsmack libcap + libprocps ) SET(CLIENT_VERSION_MAJOR 1) @@ -24,6 +25,7 @@ SET(CLIENT_SOURCES ${CLIENT_PATH}/client-common.cpp ${CLIENT_PATH}/client-offline.cpp ${CLIENT_PATH}/client-label-monitor.cpp + ${CLIENT_PATH}/check-proper-drop.cpp ) ADD_LIBRARY(${TARGET_CLIENT} SHARED ${CLIENT_SOURCES}) diff --git a/src/client/check-proper-drop.cpp b/src/client/check-proper-drop.cpp new file mode 100644 index 0000000..4d136c5 --- /dev/null +++ b/src/client/check-proper-drop.cpp @@ -0,0 +1,129 @@ +/* + * Copyright (c) 2015 Samsung Electronics Co., Ltd All Rights Reserved + * + * Contact: Rafal Krypa + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License + */ +/* + * @file check-proper-drop.cpp + * @author Rafal Krypa + * @version 1.0 + * @brief Implementation of proper privilege dropping check utilities + */ + +#include "check-proper-drop.h" +#include "smack-labels.h" +#include + +#include + +#include +#include + +namespace SecurityManager { + +using proctabPtr = std::unique_ptr; +using capPtr = std::unique_ptr<_cap_struct, int (*)(void*)>; +using capStrPtr = std::unique_ptr; + +CheckProperDrop::~CheckProperDrop() +{ + for (const auto &thread : m_threads) + freeproc(thread); + freeproc(m_proc); +} + +void CheckProperDrop::getThreads() +{ + pid_t pid[2] = {m_pid, 0}; + proctabPtr PT(openproc(PROC_FILLSTATUS | PROC_PID, pid), closeproc); + if (!PT) + ThrowMsg(Exception::ProcError, + "Unable to open proc interface"); + + m_proc = readproc(PT.get(), nullptr); + if (!m_proc) + ThrowMsg(Exception::ProcError, + "Unable read process information for " << pid); + + proc_t *thread; + while ((thread = readtask(PT.get(), m_proc, nullptr))) + if (thread->tid != m_pid) + m_threads.push_back(thread); +} + +bool CheckProperDrop::checkThreads() +{ +#define REPORT_THREAD_ERROR(TID, NAME, VAL1, VAL2) { \ + LogError("Invalid value of " << (NAME) << " for thread " << (TID) << "." \ + << ". Process has " << (VAL1) << ", thread has " << (VAL2) << "."); \ + return false; \ +} + +#define CHECK_THREAD_CRED_FIELD(P, T, FIELD) { \ + int pval = (P)->FIELD, tval = (T)->FIELD; \ + if (pval != tval) \ + REPORT_THREAD_ERROR((T)->tid, #FIELD, pval, tval); \ +} + + std::string smackProc = SmackLabels::getSmackLabelFromPid(m_pid); + + capPtr capProc(cap_get_pid(m_pid), cap_free); + if (!capProc) + ThrowMsg(Exception::CapError, + "Unable to get capabilities for " << m_pid); + + capStrPtr capStrProc(cap_to_text(capProc.get(), nullptr), cap_free); + if (!capStrProc) + ThrowMsg(Exception::CapError, + "Unable to get capabilities for " << m_pid); + + for (const auto &thread : m_threads) { + capPtr capThread(cap_get_pid(thread->tid), cap_free); + if (!capThread) + ThrowMsg(Exception::CapError, + "Unable to get capabilities for " << thread->tid); + + if (cap_compare(capProc.get(), capThread.get())) { + capStrPtr capStrThread(cap_to_text(capThread.get(), nullptr), cap_free); + if (!capStrThread) + ThrowMsg(Exception::CapError, "Unable to get capabilities for " << thread->tid); + + REPORT_THREAD_ERROR(thread->tid, "capabilities", + capStrProc.get(), capStrThread.get()); + } + + std::string smackThread = SmackLabels::getSmackLabelFromPid(thread->tid); + if (smackProc != smackThread) + REPORT_THREAD_ERROR(thread->tid, "Smack label", + smackProc, smackThread); + + if (strcmp(m_proc->supgid, thread->supgid)) + REPORT_THREAD_ERROR(thread->tid, "Supplementary groups", + m_proc->supgid, thread->supgid); + + CHECK_THREAD_CRED_FIELD(m_proc, thread, euid); + CHECK_THREAD_CRED_FIELD(m_proc, thread, egid); + CHECK_THREAD_CRED_FIELD(m_proc, thread, ruid); + CHECK_THREAD_CRED_FIELD(m_proc, thread, rgid); + CHECK_THREAD_CRED_FIELD(m_proc, thread, suid); + CHECK_THREAD_CRED_FIELD(m_proc, thread, sgid); + CHECK_THREAD_CRED_FIELD(m_proc, thread, fuid); + CHECK_THREAD_CRED_FIELD(m_proc, thread, fgid); + } + + return true; +} + +} // namespace SecurityManager diff --git a/src/client/client-security-manager.cpp b/src/client/client-security-manager.cpp index 40b037f..55109fc 100644 --- a/src/client/client-security-manager.cpp +++ b/src/client/client-security-manager.cpp @@ -26,6 +26,7 @@ */ #include +#include #include #include #include @@ -52,6 +53,7 @@ #include #include #include +#include #include #include @@ -694,22 +696,35 @@ int security_manager_drop_process_privileges(void) SECURITY_MANAGER_API int security_manager_prepare_app(const char *app_name) { - LogDebug("security_manager_prepare_app() called"); - int ret; + return try_catch([&] { + LogDebug("security_manager_prepare_app() called"); + int ret; - ret = security_manager_set_process_groups_from_appid(app_name); - if (ret != SECURITY_MANAGER_SUCCESS) { - LogWarning("Unable to setup process groups for application. Privileges with direct access to resources will not work."); - ret = SECURITY_MANAGER_SUCCESS; - } + ret = security_manager_set_process_groups_from_appid(app_name); + if (ret != SECURITY_MANAGER_SUCCESS) { + LogWarning("Unable to setup process groups for application. Privileges with direct access to resources will not work."); + } - ret = security_manager_sync_threads_internal(app_name); - if (ret != SECURITY_MANAGER_SUCCESS) { - LogError("Can't properly setup application threads (Smack label & capabilities)"); - return ret; - } + ret = security_manager_sync_threads_internal(app_name); + if (ret != SECURITY_MANAGER_SUCCESS) { + LogError("Can't properly setup application threads (Smack label & capabilities)"); + exit(EXIT_FAILURE); + } - return ret; + try { + CheckProperDrop cpd; + cpd.getThreads(); + if (!cpd.checkThreads()) { + LogError("Privileges haven't been properly dropped for the whole process"); + exit(EXIT_FAILURE); + } + } catch (const SecurityManager::Exception &e) { + LogError("Error while checking privileges of the process: " << e.DumpToString()); + exit(EXIT_FAILURE); + } + + return ret; + }); } SECURITY_MANAGER_API diff --git a/src/client/include/check-proper-drop.h b/src/client/include/check-proper-drop.h new file mode 100644 index 0000000..ad1df5b --- /dev/null +++ b/src/client/include/check-proper-drop.h @@ -0,0 +1,76 @@ +/* + * Copyright (c) 2015 Samsung Electronics Co., Ltd All Rights Reserved + * + * Contact: Rafal Krypa + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License + */ +/* + * @file check-proper-drop.h + * @author Rafal Krypa + * @version 1.0 + * @brief Definition of proper privilege dropping check utilities + */ + +#ifndef SECURITY_MANAGER_CHECK_PROPER_DROP_ +#define SECURITY_MANAGER_CHECK_PROPER_DROP_ + +#include + +#include +#include + +#include + +namespace SecurityManager { + +class CheckProperDrop { +public: + class Exception { + public: + DECLARE_EXCEPTION_TYPE(SecurityManager::Exception, Base) + DECLARE_EXCEPTION_TYPE(Base, ProcError) + DECLARE_EXCEPTION_TYPE(Base, CapError) + }; + + ~CheckProperDrop(); + CheckProperDrop(pid_t pid = getpid()) : m_pid(pid) {}; + + /** + * Fetch credentials of the process and all its threads. + * Must be called before checkThreads(). + */ + void getThreads(); + + /** + * Check whether all threads of the process has properly aligned + * credentials: + * - uids + * - gids + * - capabilities + * - Smack labels + * + * It will terminate the calling process if any thread has different + * value than the other threads. This prevents security risks associated + * with improperly dropped privileges during application launch. + */ + bool checkThreads(); + +private: + pid_t m_pid; + proc_t *m_proc = nullptr; + std::vector m_threads; +}; + +} // namespace SecurityManager +#endif /* SECURITY_MANAGER_CHECK_PROPER_DROP_H_ */ -- 2.7.4