From 4e1302eee8aac99171acfe638892983f97fdf8e2 Mon Sep 17 00:00:00 2001 From: Zofia Abramowska Date: Tue, 7 Apr 2020 17:30:03 +0200 Subject: [PATCH 01/16] Change privilege related Smack rules on cynara policy change When policy is updated recalculate privilege related Smack rules for all running applications. Change-Id: Ic6a0341399186d10404f1ce189217d963707e7be --- packaging/security-manager.spec | 1 + src/client/CMakeLists.txt | 1 + src/common/CMakeLists.txt | 1 + src/common/include/service_impl.h | 4 ++ src/common/include/smack-rules.h | 17 ++++++ src/common/service_impl.cpp | 114 ++++++++++++++++++++++++++++++++++++++ src/common/smack-rules.cpp | 22 ++++++-- src/server/CMakeLists.txt | 1 + 8 files changed, 157 insertions(+), 4 deletions(-) diff --git a/packaging/security-manager.spec b/packaging/security-manager.spec index 0d50227..d489f22 100644 --- a/packaging/security-manager.spec +++ b/packaging/security-manager.spec @@ -37,6 +37,7 @@ BuildRequires: pkgconfig(cynara-client-async) BuildRequires: pkgconfig(security-privilege-manager) BuildRequires: pkgconfig(openssl1.1) BuildRequires: pkgconfig(mount) +BuildRequires: pkgconfig(capi-appfw-app-manager) BuildRequires: boost-devel %{?systemd_requires} diff --git a/src/client/CMakeLists.txt b/src/client/CMakeLists.txt index 8862ca0..4dfe61c 100644 --- a/src/client/CMakeLists.txt +++ b/src/client/CMakeLists.txt @@ -1,5 +1,6 @@ PKG_CHECK_MODULES(CLIENT_DEP REQUIRED + capi-appfw-app-manager cynara-client-async libsmack libcap diff --git a/src/common/CMakeLists.txt b/src/common/CMakeLists.txt index 257f0a9..b37a53e 100644 --- a/src/common/CMakeLists.txt +++ b/src/common/CMakeLists.txt @@ -11,6 +11,7 @@ PKG_CHECK_MODULES(COMMON_DEP libtzplatform-config security-privilege-manager mount + capi-appfw-app-manager ) IF(DPL_WITH_DLOG) diff --git a/src/common/include/service_impl.h b/src/common/include/service_impl.h index 33a3d1d..ff98d51 100644 --- a/src/common/include/service_impl.h +++ b/src/common/include/service_impl.h @@ -28,6 +28,8 @@ #include +#include + #include "channel.h" #include "credentials.h" #include "cynara.h" @@ -440,6 +442,7 @@ private: const Smack::Label &targetAppLabel, const std::string &path); + static bool updateRunningAppSmackPolicy(app_context_h app_context, void *user_data); void updatePermissibleSet(uid_t uid, int type); Smack::Label getAppProcessLabel(const std::string &appName, const std::string &pkgName); @@ -458,6 +461,7 @@ private: CynaraAdmin m_cynaraAdmin; PrivilegeGids m_privilegeGids; NSMountLogic m_NSMountLogic; + std::map m_appIdUidMap; }; } /* namespace SecurityManager */ diff --git a/src/common/include/smack-rules.h b/src/common/include/smack-rules.h index 189611a..35f6d8d 100644 --- a/src/common/include/smack-rules.h +++ b/src/common/include/smack-rules.h @@ -99,6 +99,23 @@ public: int authorId); /** + * Disable privilege-specific smack rules for given application + * + * Function creates privilege-specific smack rules using predefined templates. + * Rules are cleared from the kernel. + * + * @param[in] appProcessLabel - process label of the application + * @param[in] pkgName - package id of given application + * @param[in] authorId - author id of given application (if not applicable, set to -1) + * @param[in] privileges - a list of privileges to be disabled for given application + */ + void disablePrivilegeRules( + const Smack::Label &appProcessLabel, + const std::string &pkgName, + int authorId, + const std::vector &privileges); + + /** * Uninstall package-specific smack rules. * * Function loads package-specific smack rules, revokes them from the kernel. diff --git a/src/common/service_impl.cpp b/src/common/service_impl.cpp index e3186f8..a1c145a 100644 --- a/src/common/service_impl.cpp +++ b/src/common/service_impl.cpp @@ -34,10 +34,12 @@ #include #include #include +#include #include #include +#include #include #include @@ -1081,6 +1083,80 @@ int ServiceImpl::userDelete(const Credentials &creds, uid_t uidDeleted) return ret; } +typedef std::map> AppsAllowedPrivilegesMap; + +namespace { + +struct AppManagerCallbackContext { + ServiceImpl *impl; + AppsAllowedPrivilegesMap *appsAllowedPrivileges; +}; + +std::string getAppIdFromContext(app_context_h app_context) +{ + char *appId; + int ret = app_context_get_app_id (app_context, &appId); + if (ret != APP_MANAGER_ERROR_NONE) { + LogError("Failed to get appId from app_context: " << ret); + return ""; + } + std::unique_ptr appIdPtr(appId, std::free); + std::string appIdStr(appId); + appIdPtr.release(); + return appIdStr; +} + +} // namespace anonymous + +bool ServiceImpl::updateRunningAppSmackPolicy(app_context_h app_context, void *user_data) { + AppManagerCallbackContext *context = reinterpret_cast(user_data); + + std::string appId = getAppIdFromContext(app_context); + if (appId.empty()) { + LogDebug("No appId from running context"); + return true; + } + + LogDebug("Found " << appId << " running"); + + std::string pkgName; + context->impl->getPkgName(appId, pkgName); + + bool isPkgHybrid = context->impl->m_privilegeDb.IsPackageHybrid(pkgName); + + auto oldAllowedPrivs = context->appsAllowedPrivileges->operator[](appId); + + auto it = context->impl->m_appIdUidMap.find(appId); + if (it == context->impl->m_appIdUidMap.end()) { + LogError("THIS IS VERY VERY BAD. RUNNING APPLICATION HAS NO SAVED UID!!!"); + return true; + } + uid_t uid = it->second; + std::vector newAllowedPrivs; + auto appProcessLabel = SmackLabels::generateProcessLabel(appId, pkgName, isPkgHybrid); + context->impl->getAppAllowedPrivileges(appProcessLabel, uid, newAllowedPrivs); + + std::vector inter; + std::sort(oldAllowedPrivs.begin(), oldAllowedPrivs.end()); + std::sort(newAllowedPrivs.begin(), newAllowedPrivs.end()); + + std::vector denied, allowed; + // Deny privileges which were in old allowed privs, but are not in new ones + std::set_difference(oldAllowedPrivs.begin(), oldAllowedPrivs.end(), + newAllowedPrivs.begin(), newAllowedPrivs.end(), + std::back_inserter(denied)); + // Allow privileges which are in new allowed privs, but were not in old ones + std::set_difference(newAllowedPrivs.begin(), newAllowedPrivs.end(), + oldAllowedPrivs.begin(), oldAllowedPrivs.end(), + std::back_inserter(allowed)); + int authorId; + context->impl->m_privilegeDb.GetPkgAuthorId(pkgName, authorId); + + context->impl->m_smackRules.disablePrivilegeRules(appProcessLabel, pkgName, authorId, denied); + context->impl->m_smackRules.enablePrivilegeRules(appProcessLabel, pkgName, authorId, allowed); + + return true; +} int ServiceImpl::policyUpdate(const Credentials &creds, const std::vector &policyEntries) { @@ -1101,9 +1177,44 @@ int ServiceImpl::policyUpdate(const Credentials &creds, const std::vector> appsAllowedPrivileges; + std::vector allPackages; + + // TODO: Can this be optimized to: if policy entries contain either wildcard + // in place of privilege or if any of the privileges has smack mapping? + if (m_smackRules.isPrivilegeMappingEnabled()) { + // Get policy of all installed application so we can recalculate it after update + m_privilegeDb.GetAllPackages(allPackages); + for (auto &package : allPackages) { + std::vector packageApps; + m_privilegeDb.GetPkgApps(package, packageApps); + bool isPkgHybrid = m_privilegeDb.IsPackageHybrid(package); + for (auto &app : packageApps) { + auto it = m_appIdUidMap.find(app); + if (it == m_appIdUidMap.end()) { + // TODO: Move app -> uid mapping to database + LogDebug("App " << app << " has never been run in security-manager instance"); + continue; + } + uid_t uid = it->second; + auto appProcessLabel = SmackLabels::generateProcessLabel(app, package, isPkgHybrid); + getAppAllowedPrivileges(appProcessLabel, uid, appsAllowedPrivileges[app]); + } + } + } // Apply updates m_cynaraAdmin.setPolicies(validatedPolicies); + if (m_smackRules.isPrivilegeMappingEnabled()) { + LogDebug("Updating privilege related Smack rules"); + AppManagerCallbackContext context{this, &appsAllowedPrivileges}; + int ret = app_manager_foreach_running_app_context(&ServiceImpl::updateRunningAppSmackPolicy, + &context); + if (ret != APP_MANAGER_ERROR_NONE) { + LogError("Failed to update Smack policy for running apps"); + } + } + // Update mount namespaces // TODO: Don't update all users, apps and privileges, filter by policyEntries if (!m_NSMountLogic.check()) @@ -2148,6 +2259,7 @@ int ServiceImpl::prepareApp(const Credentials &creds, const std::string &appName std::vector &forbiddenGroups, std::vector &allowedGroups, std::vector &privPathsStatusVector) { LogDebug("Requested prepareApp for application " << appName); + bool isHybrid; if (!m_privilegeDb.GetAppPkgInfo(appName, pkgName, isHybrid, enabledSharedRO)) return SECURITY_MANAGER_ERROR_UNKNOWN; @@ -2167,6 +2279,8 @@ int ServiceImpl::prepareApp(const Credentials &creds, const std::string &appName getPkgLabels(pkgName, pkgLabels); if (m_smackRules.isPrivilegeMappingEnabled()) { + m_appIdUidMap[appName] = creds.uid; + m_smackRules.disableAllPrivilegeRules(label, pkgName, authorId); m_smackRules.enablePrivilegeRules(label, pkgName, authorId, allowedPrivileges); } diff --git a/src/common/smack-rules.cpp b/src/common/smack-rules.cpp index ccba2e8..c205865 100644 --- a/src/common/smack-rules.cpp +++ b/src/common/smack-rules.cpp @@ -237,15 +237,29 @@ void SmackRules::enablePrivilegeRules( } void SmackRules::disableAllPrivilegeRules( + const Smack::Label &appProcessLabel, + const std::string &pkgName, + int authorId) +{ + LogDebug("Disabling all privilege rules for " << appProcessLabel); + disablePrivilegeRules(appProcessLabel, pkgName, authorId, m_templateMgr.getAllMappedPrivs()); +} + +void SmackRules::disablePrivilegeRules( const Smack::Label &appProcessLabel, const std::string &pkgName, - int authorId) + int authorId, + const std::vector &privileges) { - LogDebug("Disabling all privilege rules for " << appProcessLabel); + if (privileges.empty()) { + // No rules to apply + return; + } + + LogDebug("Disabling privilege rules for " << appProcessLabel); SmackAccesses smackRules; - addPrivilegesRules(smackRules, appProcessLabel, pkgName, authorId, - m_templateMgr.getAllMappedPrivs()); + addPrivilegesRules(smackRules, appProcessLabel, pkgName, authorId, privileges); if (smack_check()) smackRules.clear(); diff --git a/src/server/CMakeLists.txt b/src/server/CMakeLists.txt index 7f0ce3f..97f596e 100644 --- a/src/server/CMakeLists.txt +++ b/src/server/CMakeLists.txt @@ -1,6 +1,7 @@ PKG_CHECK_MODULES(SERVER_DEP REQUIRED libsystemd + capi-appfw-app-manager cynara-client mount ) -- 2.7.4 From 0e48b12e5fd0e1b0393c142d1a34466f296d8fe8 Mon Sep 17 00:00:00 2001 From: Zofia Abramowska Date: Tue, 7 Apr 2020 19:12:55 +0200 Subject: [PATCH 02/16] Remove privilege related Smack rules when multi-user is detected Privilege related Smack rules can only be used, when applications can be launched for only one user. When multiple instances of one application for different users are detected, all privilege related Smack rules for this application will be revoked. This isn't a permanent state. When application is launched only for one user it will acquire all needed permissions. Change-Id: Ibda63d3ce4ce072f48fff4ff0e2c083c69fe66d7 --- src/common/include/service_impl.h | 2 ++ src/common/service_impl.cpp | 50 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/common/include/service_impl.h b/src/common/include/service_impl.h index ff98d51..7e63ba3 100644 --- a/src/common/include/service_impl.h +++ b/src/common/include/service_impl.h @@ -443,6 +443,8 @@ private: const std::string &path); static bool updateRunningAppSmackPolicy(app_context_h app_context, void *user_data); + static bool checkRunningApps(app_context_h app_context, void *user_data); + void updatePermissibleSet(uid_t uid, int type); Smack::Label getAppProcessLabel(const std::string &appName, const std::string &pkgName); diff --git a/src/common/service_impl.cpp b/src/common/service_impl.cpp index a1c145a..5308991 100644 --- a/src/common/service_impl.cpp +++ b/src/common/service_impl.cpp @@ -2254,6 +2254,23 @@ Smack::Label ServiceImpl::getProcessLabel(const std::string &appName) return getAppProcessLabel(appName); } +struct AppMgrCheckAppsCbContext { + std::string appName; + bool isRunning; +}; + +bool ServiceImpl::checkRunningApps(app_context_h app_context, void *user_data) +{ + AppMgrCheckAppsCbContext *context = reinterpret_cast(user_data); + std::string appId = getAppIdFromContext(app_context); + if (appId == context->appName) { + context->isRunning = true; + return false; + } + + return true; +} + int ServiceImpl::prepareApp(const Credentials &creds, const std::string &appName, const std::vector &privPathsVector, Smack::Label &label, std::string &pkgName, bool &enabledSharedRO, std::vector &forbiddenGroups, std::vector &allowedGroups, std::vector &privPathsStatusVector) @@ -2279,10 +2296,39 @@ int ServiceImpl::prepareApp(const Credentials &creds, const std::string &appName getPkgLabels(pkgName, pkgLabels); if (m_smackRules.isPrivilegeMappingEnabled()) { - m_appIdUidMap[appName] = creds.uid; + uid_t savedUid; + auto it = m_appIdUidMap.find(appName); + if (it == m_appIdUidMap.end()) { + m_appIdUidMap[appName] = creds.uid; + savedUid = creds.uid; + } else { + savedUid = it->second; + } + // We have to remove all possible privilege related Smack rules, because application + // policy might have changed from last prepareApp + // (e.g. application new version was installed) m_smackRules.disableAllPrivilegeRules(label, pkgName, authorId); - m_smackRules.enablePrivilegeRules(label, pkgName, authorId, allowedPrivileges); + + if (savedUid != creds.uid) { + LogDebug("Possible second instance detected. Checking all running apps"); + + AppMgrCheckAppsCbContext context{appName, false}; + int ret = app_manager_foreach_running_app_context(&ServiceImpl::checkRunningApps, + &context); + if (ret != APP_MANAGER_ERROR_NONE) { + LogError("Couldn't check running apps. No Smack policy will be applied for " + << appName); + } else if (context.isRunning) { + LogError("Application is already running! No Smack policy will be applied for " + << appName); + } else { + m_smackRules.enablePrivilegeRules(label, pkgName, authorId, allowedPrivileges); + } + m_appIdUidMap[appName] = creds.uid; + } else { + m_smackRules.enablePrivilegeRules(label, pkgName, authorId, allowedPrivileges); + } } ret = getForbiddenAndAllowedGroups(label, allowedPrivileges, forbiddenGroups, -- 2.7.4 From 8fec3071dbe70b8bb4d4a298f72fb3c4fd1690a8 Mon Sep 17 00:00:00 2001 From: Zofia Abramowska Date: Tue, 14 Apr 2020 18:49:34 +0200 Subject: [PATCH 03/16] Use mount namespace mount points to find running apps Change-Id: Ifef7a3aa2fb9666e20f428270c41850ce7319208 --- packaging/security-manager.spec | 1 - src/client/CMakeLists.txt | 1 - src/common/CMakeLists.txt | 3 +- src/common/include/mount-namespace.h | 11 ++- src/common/include/service_impl.h | 10 +- src/common/mount-namespace.cpp | 40 ++++++++ src/common/service_impl.cpp | 176 ++++++++++++++--------------------- src/server/CMakeLists.txt | 1 - 8 files changed, 127 insertions(+), 116 deletions(-) diff --git a/packaging/security-manager.spec b/packaging/security-manager.spec index d489f22..0d50227 100644 --- a/packaging/security-manager.spec +++ b/packaging/security-manager.spec @@ -37,7 +37,6 @@ BuildRequires: pkgconfig(cynara-client-async) BuildRequires: pkgconfig(security-privilege-manager) BuildRequires: pkgconfig(openssl1.1) BuildRequires: pkgconfig(mount) -BuildRequires: pkgconfig(capi-appfw-app-manager) BuildRequires: boost-devel %{?systemd_requires} diff --git a/src/client/CMakeLists.txt b/src/client/CMakeLists.txt index 4dfe61c..8862ca0 100644 --- a/src/client/CMakeLists.txt +++ b/src/client/CMakeLists.txt @@ -1,6 +1,5 @@ PKG_CHECK_MODULES(CLIENT_DEP REQUIRED - capi-appfw-app-manager cynara-client-async libsmack libcap diff --git a/src/common/CMakeLists.txt b/src/common/CMakeLists.txt index b37a53e..e978a4f 100644 --- a/src/common/CMakeLists.txt +++ b/src/common/CMakeLists.txt @@ -11,7 +11,6 @@ PKG_CHECK_MODULES(COMMON_DEP libtzplatform-config security-privilege-manager mount - capi-appfw-app-manager ) IF(DPL_WITH_DLOG) @@ -110,6 +109,8 @@ SET_TARGET_PROPERTIES(${TARGET_COMMON} TARGET_LINK_LIBRARIES(${TARGET_COMMON} ${COMMON_DEP_LIBRARIES} ${DLOG_DEP_LIBRARIES} + boost_filesystem + boost_regex -lcrypt rt ) diff --git a/src/common/include/mount-namespace.h b/src/common/include/mount-namespace.h index 49b5587..2f4bd21 100644 --- a/src/common/include/mount-namespace.h +++ b/src/common/include/mount-namespace.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017 Samsung Electronics Co., Ltd All Rights Reserved + * Copyright (c) 2017-2020 Samsung Electronics Co., Ltd. All rights reserved * * Contact: Rafal Krypa * @@ -28,6 +28,8 @@ #include #include +#include + namespace SecurityManager { namespace MountNS { @@ -60,5 +62,12 @@ int bindMountRO(const Path &source, const Path &target); int uMount(const Path &target); bool isPathBound(const Path &what, const Path &where); +struct AppContext { + Smack::Label appProcessLabel; + std::string uidStr; +}; + +std::vector getMountNSApps(); + } // namespace MountNS } // namespace SecurityManager diff --git a/src/common/include/service_impl.h b/src/common/include/service_impl.h index 7e63ba3..248989d 100644 --- a/src/common/include/service_impl.h +++ b/src/common/include/service_impl.h @@ -28,11 +28,10 @@ #include -#include - #include "channel.h" #include "credentials.h" #include "cynara.h" +#include "mount-namespace.h" #include "nsmount-logic.h" #include "security-manager.h" #include "smack-common.h" @@ -188,6 +187,8 @@ public: */ int getAppAllowedPrivileges(const Smack::Label &appProcessLabel, uid_t uid, std::vector &allowedPrivileges); + int getAppAllowedPrivileges(const Smack::Label &appProcessLabel, const std::string &uid, + std::vector &allowedPrivileges); /** * Process query for resources group list and supplementary groups allowed for the application. @@ -442,8 +443,9 @@ private: const Smack::Label &targetAppLabel, const std::string &path); - static bool updateRunningAppSmackPolicy(app_context_h app_context, void *user_data); - static bool checkRunningApps(app_context_h app_context, void *user_data); + void updateRunningAppSmackPolicy( + const MountNS::AppContext &appContext, + const std::map> &appsAllowedPrivileges); void updatePermissibleSet(uid_t uid, int type); diff --git a/src/common/mount-namespace.cpp b/src/common/mount-namespace.cpp index f9a48ea..21a3ed7 100644 --- a/src/common/mount-namespace.cpp +++ b/src/common/mount-namespace.cpp @@ -30,6 +30,10 @@ #include #include +#include +#include +#include + #include #include #include @@ -253,5 +257,41 @@ bool isPathBound(const Path &what, const Path &where) return (st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino); } +std::vector getMountNSApps() +{ + const static std::string rootDir = getUsersAppsMountPointsPath(); + const static std::string regexp = + rootDir + "/(\\d*)/apps/(User::Pkg::[^/]*)|(User::Pkg::User::App::[^/]*)"; + + + boost::system::error_code ec; + boost::filesystem::path root(rootDir); + boost::regex mountNSAppFilter(regexp); + + std::vector apps; + for (boost::filesystem::recursive_directory_iterator it(root, ec), eit; + it != eit; + it.increment(ec)) + { + if (ec) { + LogWarning("Failed to process " + it->path().string()); + it.pop(); + continue; + } + + boost::smatch matches; + std::string path = it->path().string(); + if (boost::regex_match(path, matches, mountNSAppFilter)) { + if (matches.size() < 4) { + LogError("Failed to properly process regexp " << regexp); + return {}; + } + apps.push_back({matches[2], matches[1]}); + } + } + + return apps; +} + } // namespace MountNS } // namespace SecurityManager diff --git a/src/common/service_impl.cpp b/src/common/service_impl.cpp index 5308991..4e0ee6d 100644 --- a/src/common/service_impl.cpp +++ b/src/common/service_impl.cpp @@ -39,7 +39,6 @@ #include #include -#include #include #include @@ -1085,58 +1084,23 @@ int ServiceImpl::userDelete(const Credentials &creds, uid_t uidDeleted) } typedef std::map> AppsAllowedPrivilegesMap; -namespace { - -struct AppManagerCallbackContext { - ServiceImpl *impl; - AppsAllowedPrivilegesMap *appsAllowedPrivileges; -}; - -std::string getAppIdFromContext(app_context_h app_context) +void ServiceImpl::updateRunningAppSmackPolicy( + const MountNS::AppContext &appContext, + const std::map> &appsAllowedPrivileges) { - char *appId; - int ret = app_context_get_app_id (app_context, &appId); - if (ret != APP_MANAGER_ERROR_NONE) { - LogError("Failed to get appId from app_context: " << ret); - return ""; - } - std::unique_ptr appIdPtr(appId, std::free); - std::string appIdStr(appId); - appIdPtr.release(); - return appIdStr; -} + LogDebug("Updateing privilege Smack policy for: " << appContext.appProcessLabel); -} // namespace anonymous -bool ServiceImpl::updateRunningAppSmackPolicy(app_context_h app_context, void *user_data) { - AppManagerCallbackContext *context = reinterpret_cast(user_data); - - std::string appId = getAppIdFromContext(app_context); - if (appId.empty()) { - LogDebug("No appId from running context"); - return true; + auto it = appsAllowedPrivileges.find(appContext.appProcessLabel); + if (it == appsAllowedPrivileges.end()) { + LogError("No allowed privileges set for " << appContext.appProcessLabel); + return; } + auto oldAllowedPrivs = it->second; - LogDebug("Found " << appId << " running"); - - std::string pkgName; - context->impl->getPkgName(appId, pkgName); - - bool isPkgHybrid = context->impl->m_privilegeDb.IsPackageHybrid(pkgName); - - auto oldAllowedPrivs = context->appsAllowedPrivileges->operator[](appId); - - auto it = context->impl->m_appIdUidMap.find(appId); - if (it == context->impl->m_appIdUidMap.end()) { - LogError("THIS IS VERY VERY BAD. RUNNING APPLICATION HAS NO SAVED UID!!!"); - return true; - } - uid_t uid = it->second; std::vector newAllowedPrivs; - auto appProcessLabel = SmackLabels::generateProcessLabel(appId, pkgName, isPkgHybrid); - context->impl->getAppAllowedPrivileges(appProcessLabel, uid, newAllowedPrivs); + getAppAllowedPrivileges(appContext.appProcessLabel, appContext.uidStr, newAllowedPrivs); - std::vector inter; std::sort(oldAllowedPrivs.begin(), oldAllowedPrivs.end()); std::sort(newAllowedPrivs.begin(), newAllowedPrivs.end()); @@ -1149,13 +1113,24 @@ bool ServiceImpl::updateRunningAppSmackPolicy(app_context_h app_context, void *u std::set_difference(newAllowedPrivs.begin(), newAllowedPrivs.end(), oldAllowedPrivs.begin(), oldAllowedPrivs.end(), std::back_inserter(allowed)); + + std::string appName, pkgName; + SmackLabels::generateAppPkgNameFromLabel(appContext.appProcessLabel, appName, pkgName); + int authorId; - context->impl->m_privilegeDb.GetPkgAuthorId(pkgName, authorId); + m_privilegeDb.GetPkgAuthorId(pkgName, authorId); - context->impl->m_smackRules.disablePrivilegeRules(appProcessLabel, pkgName, authorId, denied); - context->impl->m_smackRules.enablePrivilegeRules(appProcessLabel, pkgName, authorId, allowed); + m_smackRules.disablePrivilegeRules(appContext.appProcessLabel, pkgName, authorId, denied); + m_smackRules.enablePrivilegeRules(appContext.appProcessLabel, pkgName, authorId, allowed); +} - return true; +bool isMultiUser(const MountNS::AppContext &app, const std::vector &runningApps) +{ + for (auto &runningApp: runningApps) { + if (runningApp.appProcessLabel == app.appProcessLabel && runningApp.uidStr != app.uidStr) + return true; + } + return false; } int ServiceImpl::policyUpdate(const Credentials &creds, const std::vector &policyEntries) @@ -1180,26 +1155,12 @@ int ServiceImpl::policyUpdate(const Credentials &creds, const std::vector> appsAllowedPrivileges; std::vector allPackages; - // TODO: Can this be optimized to: if policy entries contain either wildcard - // in place of privilege or if any of the privileges has smack mapping? if (m_smackRules.isPrivilegeMappingEnabled()) { - // Get policy of all installed application so we can recalculate it after update - m_privilegeDb.GetAllPackages(allPackages); - for (auto &package : allPackages) { - std::vector packageApps; - m_privilegeDb.GetPkgApps(package, packageApps); - bool isPkgHybrid = m_privilegeDb.IsPackageHybrid(package); - for (auto &app : packageApps) { - auto it = m_appIdUidMap.find(app); - if (it == m_appIdUidMap.end()) { - // TODO: Move app -> uid mapping to database - LogDebug("App " << app << " has never been run in security-manager instance"); - continue; - } - uid_t uid = it->second; - auto appProcessLabel = SmackLabels::generateProcessLabel(app, package, isPkgHybrid); - getAppAllowedPrivileges(appProcessLabel, uid, appsAllowedPrivileges[app]); - } + for (auto &appKnownContext: m_appIdUidMap) { + auto appProcessLabel = appKnownContext.first; + auto uid = appKnownContext.second; + + getAppAllowedPrivileges(appProcessLabel, uid, appsAllowedPrivileges[appProcessLabel]); } } // Apply updates @@ -1207,11 +1168,19 @@ int ServiceImpl::policyUpdate(const Credentials &creds, const std::vector &levels) return ret; } - int ServiceImpl::getAppAllowedPrivileges(const Smack::Label &appProcessLabel, uid_t uid, std::vector &allowedPrivileges) + +{ + return getAppAllowedPrivileges(appProcessLabel, std::to_string(uid), allowedPrivileges); +} + +int ServiceImpl::getAppAllowedPrivileges(const Smack::Label &appProcessLabel, + const std::string &uid, std::vector &allowedPrivileges) { try { LogDebug("Getting allowed privileges for " << appProcessLabel << " and user " << uid); - std::string uidStr = std::to_string(uid); std::vector privileges; - m_cynaraAdmin.getAppPolicy(appProcessLabel, uidStr, privileges); + m_cynaraAdmin.getAppPolicy(appProcessLabel, uid, privileges); m_cynaraAdmin.getAppPolicy(appProcessLabel, CYNARA_ADMIN_WILDCARD, privileges); m_cynaraAdmin.getAppPolicy(CYNARA_ADMIN_WILDCARD, CYNARA_ADMIN_WILDCARD, privileges); @@ -1527,7 +1501,7 @@ int ServiceImpl::getAppAllowedPrivileges(const Smack::Label &appProcessLabel, for (auto &privilege : privileges) { int result = CYNARA_ADMIN_DENY; std::string resultExtra; - m_cynaraAdmin.check(appProcessLabel, uidStr, privilege, CynaraAdmin::Buckets[Bucket::PRIVACY_MANAGER], result, resultExtra, true); + m_cynaraAdmin.check(appProcessLabel, uid, privilege, CynaraAdmin::Buckets[Bucket::PRIVACY_MANAGER], result, resultExtra, true); if (result == CYNARA_ADMIN_ALLOW) { LogDebug("Application " << appProcessLabel << " has " << privilege << " allowed"); allowedPrivileges.push_back(privilege); @@ -2254,23 +2228,6 @@ Smack::Label ServiceImpl::getProcessLabel(const std::string &appName) return getAppProcessLabel(appName); } -struct AppMgrCheckAppsCbContext { - std::string appName; - bool isRunning; -}; - -bool ServiceImpl::checkRunningApps(app_context_h app_context, void *user_data) -{ - AppMgrCheckAppsCbContext *context = reinterpret_cast(user_data); - std::string appId = getAppIdFromContext(app_context); - if (appId == context->appName) { - context->isRunning = true; - return false; - } - - return true; -} - int ServiceImpl::prepareApp(const Credentials &creds, const std::string &appName, const std::vector &privPathsVector, Smack::Label &label, std::string &pkgName, bool &enabledSharedRO, std::vector &forbiddenGroups, std::vector &allowedGroups, std::vector &privPathsStatusVector) @@ -2297,9 +2254,9 @@ int ServiceImpl::prepareApp(const Credentials &creds, const std::string &appName if (m_smackRules.isPrivilegeMappingEnabled()) { uid_t savedUid; - auto it = m_appIdUidMap.find(appName); + auto it = m_appIdUidMap.find(label); if (it == m_appIdUidMap.end()) { - m_appIdUidMap[appName] = creds.uid; + m_appIdUidMap[label] = creds.uid; savedUid = creds.uid; } else { savedUid = it->second; @@ -2313,17 +2270,22 @@ int ServiceImpl::prepareApp(const Credentials &creds, const std::string &appName if (savedUid != creds.uid) { LogDebug("Possible second instance detected. Checking all running apps"); - AppMgrCheckAppsCbContext context{appName, false}; - int ret = app_manager_foreach_running_app_context(&ServiceImpl::checkRunningApps, - &context); - if (ret != APP_MANAGER_ERROR_NONE) { - LogError("Couldn't check running apps. No Smack policy will be applied for " - << appName); - } else if (context.isRunning) { - LogError("Application is already running! No Smack policy will be applied for " - << appName); - } else { + auto runningApps = MountNS::getMountNSApps(); + auto it = std::find_if(runningApps.begin(), runningApps.end(), + [&] (const MountNS::AppContext &app) { + return app.appProcessLabel == label; + }); + if (it == runningApps.end()) { + LogDebug("Second instance of " << appName << " is not currently running"); + m_smackRules.enablePrivilegeRules(label, pkgName, authorId, allowedPrivileges); + } else if (it->uidStr == std::to_string(creds.uid)){ + LogWarning("Second instance of " << appName + << " detected, but for the same uid as requested"); m_smackRules.enablePrivilegeRules(label, pkgName, authorId, allowedPrivileges); + } else { + LogWarning("Second instance of " << appName + << " detected runnign with " << it->uidStr << ". " + << " No privilege related Smack rules will be applied"); } m_appIdUidMap[appName] = creds.uid; } else { diff --git a/src/server/CMakeLists.txt b/src/server/CMakeLists.txt index 97f596e..7f0ce3f 100644 --- a/src/server/CMakeLists.txt +++ b/src/server/CMakeLists.txt @@ -1,7 +1,6 @@ PKG_CHECK_MODULES(SERVER_DEP REQUIRED libsystemd - capi-appfw-app-manager cynara-client mount ) -- 2.7.4 From 83e7e7dc7589e4698310631185ac22b1e61e6452 Mon Sep 17 00:00:00 2001 From: Zofia Abramowska Date: Mon, 20 Apr 2020 16:19:13 +0200 Subject: [PATCH 04/16] Fix multi-user detection With appId->uid mapping, we cannot properly handle this use case: * user1 launches app A -> (appA, user1) * user1 launches app B -> conflict detected, Smack not applied, mapping saved to (appB, user1) * user1 launches app B again -> no conflict detected, Smack applied (This won't be fixed if mapping is only updated, when multi-user is not detected) This commit changes multi-user detection to be only based on apps running taken from MountNS fs structure. Change-Id: I69c729e85e05cce498abdcb4e6832df634789765 --- src/common/include/service_impl.h | 1 - src/common/service_impl.cpp | 44 +++++++++------------------------------ 2 files changed, 10 insertions(+), 35 deletions(-) diff --git a/src/common/include/service_impl.h b/src/common/include/service_impl.h index 248989d..761e239 100644 --- a/src/common/include/service_impl.h +++ b/src/common/include/service_impl.h @@ -465,7 +465,6 @@ private: CynaraAdmin m_cynaraAdmin; PrivilegeGids m_privilegeGids; NSMountLogic m_NSMountLogic; - std::map m_appIdUidMap; }; } /* namespace SecurityManager */ diff --git a/src/common/service_impl.cpp b/src/common/service_impl.cpp index 4e0ee6d..d462319 100644 --- a/src/common/service_impl.cpp +++ b/src/common/service_impl.cpp @@ -1156,11 +1156,12 @@ int ServiceImpl::policyUpdate(const Credentials &creds, const std::vector allPackages; if (m_smackRules.isPrivilegeMappingEnabled()) { - for (auto &appKnownContext: m_appIdUidMap) { - auto appProcessLabel = appKnownContext.first; - auto uid = appKnownContext.second; + auto runningApps = MountNS::getMountNSApps(); + for (auto &appContext: runningApps) { + auto &appProcessLabel = appContext.appProcessLabel; + auto &uidStr = appContext.uidStr; - getAppAllowedPrivileges(appProcessLabel, uid, appsAllowedPrivileges[appProcessLabel]); + getAppAllowedPrivileges(appProcessLabel, uidStr, appsAllowedPrivileges[appProcessLabel]); } } // Apply updates @@ -2253,41 +2254,16 @@ int ServiceImpl::prepareApp(const Credentials &creds, const std::string &appName getPkgLabels(pkgName, pkgLabels); if (m_smackRules.isPrivilegeMappingEnabled()) { - uid_t savedUid; - auto it = m_appIdUidMap.find(label); - if (it == m_appIdUidMap.end()) { - m_appIdUidMap[label] = creds.uid; - savedUid = creds.uid; - } else { - savedUid = it->second; - } - // We have to remove all possible privilege related Smack rules, because application // policy might have changed from last prepareApp // (e.g. application new version was installed) m_smackRules.disableAllPrivilegeRules(label, pkgName, authorId); - if (savedUid != creds.uid) { - LogDebug("Possible second instance detected. Checking all running apps"); - - auto runningApps = MountNS::getMountNSApps(); - auto it = std::find_if(runningApps.begin(), runningApps.end(), - [&] (const MountNS::AppContext &app) { - return app.appProcessLabel == label; - }); - if (it == runningApps.end()) { - LogDebug("Second instance of " << appName << " is not currently running"); - m_smackRules.enablePrivilegeRules(label, pkgName, authorId, allowedPrivileges); - } else if (it->uidStr == std::to_string(creds.uid)){ - LogWarning("Second instance of " << appName - << " detected, but for the same uid as requested"); - m_smackRules.enablePrivilegeRules(label, pkgName, authorId, allowedPrivileges); - } else { - LogWarning("Second instance of " << appName - << " detected runnign with " << it->uidStr << ". " - << " No privilege related Smack rules will be applied"); - } - m_appIdUidMap[appName] = creds.uid; + // TODO: Optimization is welcomed here + auto runningApps = MountNS::getMountNSApps(); + if (isMultiUser({label, std::to_string(creds.uid)}, runningApps)) { + LogWarning("Detected multiuser instance of " << appName + << ". Privilege related Smack rules are cleared and won't be reapplied."); } else { m_smackRules.enablePrivilegeRules(label, pkgName, authorId, allowedPrivileges); } -- 2.7.4 From b4be888b809ad45adaf1a2239457f991ac9dae81 Mon Sep 17 00:00:00 2001 From: Zofia Abramowska Date: Tue, 21 Apr 2020 12:01:57 +0200 Subject: [PATCH 05/16] Disable Smack privilege mapping configuration Change-Id: I89870a7aa63812b08255b05c195b1c6e85a3bb96 --- policy/privilege-smack.list | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/policy/privilege-smack.list b/policy/privilege-smack.list index a95a151..e67ff67 100644 --- a/policy/privilege-smack.list +++ b/policy/privilege-smack.list @@ -18,5 +18,7 @@ # will only be accepted, when they are between privilege label and application # based labels (e.g. application process label, application read-only path label). # Other rules will be ignored. - -http://tizen.org/privilege/internet System::Privilege::Internet default +# +# for e.g.: +# +# http://tizen.org/privilege/internet System::Privilege::Internet default -- 2.7.4 From 7790a0313c21d33a13894f285ebe5ef98cbe0972 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Tue, 14 Apr 2020 21:48:49 +0200 Subject: [PATCH 06/16] Properly handle nonexisting apps uninstallation If one or more of apps to uninstall is missing (e.g. already uninstalled) the app_inst_req::app::appName is cleared and the UninstallHelper::removeApps has no flag for given app. As a result nonexistent app is unnecessarily processed in ServiceImpl::appUninstallSmackRules and smack rules of some apps may be left untouched. This is a fix for both issues. Change-Id: Ifa6499f454cdff3d9f9d9570e6670c2998cc857b --- src/common/service_impl.cpp | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/common/service_impl.cpp b/src/common/service_impl.cpp index d462319..c976313 100644 --- a/src/common/service_impl.cpp +++ b/src/common/service_impl.cpp @@ -834,10 +834,6 @@ void ServiceImpl::appUninstallPrivileges(app_inst_req::app &app, app_inst_req &r } m_privilegeDb.GetAppDefinedPrivileges(app.appName, req.uid, uh.oldAppDefinedPrivileges); - - bool removeApp = false; - m_privilegeDb.RemoveApplication(app.appName, req.uid, removeApp, uh.removePkg, uh.removeAuthor); - uh.removeApps.push_back(removeApp); } void ServiceImpl::appUninstallCynaraPolicies(const std::string &processLabel, app_inst_req &req, @@ -924,7 +920,7 @@ int ServiceImpl::appUninstall(const Credentials &creds, app_inst_req &req) } // Before we remove the app from the database, let's fetch all apps in the package - // that this app belongs to, this will allow us to remove all rules withing the + // that this app belongs to, this will allow us to remove all rules within the // package that the app appears in UninstallHelper uh; m_privilegeDb.GetPkgAuthorId(req.pkgName, uh.authorId); @@ -940,12 +936,18 @@ int ServiceImpl::appUninstall(const Credentials &creds, app_inst_req &req) ScopedTransaction trans(m_privilegeDb); for (auto &app : req.apps) { - if (app.appName.empty()) - continue; - // [db] remove app - appUninstallPrivileges(app, req, uh); - // [cynara] update app policy - appUninstallCynaraPolicies(SmackLabels::generateProcessLabel(app.appName, req.pkgName, uh.isPkgHybrid), req, uh); + bool removeApp = false; + if (!app.appName.empty()) { + // [db] remove app privileges + appUninstallPrivileges(app, req, uh); + + // [db] remove app + m_privilegeDb.RemoveApplication(app.appName, req.uid, removeApp, uh.removePkg, uh.removeAuthor); + + // [cynara] update app policy + appUninstallCynaraPolicies(SmackLabels::generateProcessLabel(app.appName, req.pkgName, uh.isPkgHybrid), req, uh); + } + uh.removeApps.push_back(removeApp); } // [db] commit -- 2.7.4 From 981e741bedd6e1ca857ee8be53789e15115d143a Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Fri, 12 Apr 2019 13:14:34 +0200 Subject: [PATCH 07/16] Create new RPM for loading iptables rules at system start iptables rules can be used by security network control with internet and internal/appdebugging priviledges. Mapping internet GID privilege with this set of iptables rules can be much simpler alternative to nether, which also supports multiuser but doesn't support runtime policy change for running apps. Change-Id: I033b36c64fc14de5a275db00aab5825dad61341d --- packaging/security-manager-ip6tables.rules | 45 ++++++++++++++++++++++ packaging/security-manager-iptables-load.service | 13 +++++++ packaging/security-manager-iptables.rules | 49 ++++++++++++++++++++++++ packaging/security-manager.spec | 23 +++++++++++ 4 files changed, 130 insertions(+) create mode 100644 packaging/security-manager-ip6tables.rules create mode 100644 packaging/security-manager-iptables-load.service create mode 100644 packaging/security-manager-iptables.rules diff --git a/packaging/security-manager-ip6tables.rules b/packaging/security-manager-ip6tables.rules new file mode 100644 index 0000000..4d98982 --- /dev/null +++ b/packaging/security-manager-ip6tables.rules @@ -0,0 +1,45 @@ +# +# Copyright (c) 2019 Samsung Electronics Co., Ltd All Rights Reserved +# +# Contact: Lukasz Pawelczyk (l.pawelczyk@samsung.com) +# +# 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 +# + +*mangle +:PREROUTING ACCEPT +:INPUT ACCEPT +:FORWARD ACCEPT +:OUTPUT ACCEPT +:POSTROUTING ACCEPT +:CHECK-LOCALHOST - +-A INPUT ! -i lo -j SECMARK --selctx System +-A OUTPUT -o lo -j CHECK-LOCALHOST +-A OUTPUT -p igmp -j ACCEPT +-A CHECK-LOCALHOST -p udp -m udp --dport 53 -j RETURN +-A CHECK-LOCALHOST -p tcp -m tcp --dport 53 -j RETURN +-A CHECK-LOCALHOST -j ACCEPT +COMMIT +*filter +:INPUT ACCEPT +:FORWARD ACCEPT +:OUTPUT ACCEPT +:REJECT-LOG - +-A OUTPUT -m conntrack --ctstate ESTABLISHED,RELATED -j ACCEPT +-A OUTPUT -m owner --uid-owner 0-5000 -j ACCEPT +-A OUTPUT -m owner --gid-owner priv_internet --suppl-groups -j ACCEPT +-A OUTPUT -j REJECT-LOG +-A REJECT-LOG -m limit --limit 1/min -j AUDIT --type reject +-A REJECT-LOG -j REJECT --reject-with icmp6-adm-prohibited +-A REJECT-LOG -j RETURN +COMMIT diff --git a/packaging/security-manager-iptables-load.service b/packaging/security-manager-iptables-load.service new file mode 100644 index 0000000..7115a8e --- /dev/null +++ b/packaging/security-manager-iptables-load.service @@ -0,0 +1,13 @@ +[Unit] +Description=Load default Tizen iptables firewall rules +# sounds reasonable to have firewall up before any of the services go up +Before=network-pre.target +Wants=network-pre.target + +[Service] +Type=oneshot +ExecStart=/sbin/iptables-restore -w -- /etc/security-manager-iptables.rules +ExecStart=/sbin/ip6tables-restore -w -- /etc/security-manager-ip6tables.rules + +[Install] +WantedBy=basic.target diff --git a/packaging/security-manager-iptables.rules b/packaging/security-manager-iptables.rules new file mode 100644 index 0000000..f8874bb --- /dev/null +++ b/packaging/security-manager-iptables.rules @@ -0,0 +1,49 @@ +# +# Copyright (c) 2019 Samsung Electronics Co., Ltd All Rights Reserved +# +# Contact: Lukasz Pawelczyk (l.pawelczyk@samsung.com) +# +# 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 +# + +*mangle +:PREROUTING ACCEPT +:INPUT ACCEPT +:FORWARD ACCEPT +:OUTPUT ACCEPT +:POSTROUTING ACCEPT +:CHECK-LOCALHOST - +-A INPUT ! -i lo -j SECMARK --selctx System +-A OUTPUT -o lo -j CHECK-LOCALHOST +-A OUTPUT -p igmp -j ACCEPT +-A CHECK-LOCALHOST -p udp -m udp --dport 53 -j RETURN +-A CHECK-LOCALHOST -p tcp -m tcp --dport 53 -j RETURN +-A CHECK-LOCALHOST -j ACCEPT +COMMIT +*filter +:INPUT ACCEPT +:FORWARD ACCEPT +:OUTPUT ACCEPT +:REJECT-LOG - +-A OUTPUT -m conntrack --ctstate ESTABLISHED,RELATED -j ACCEPT +-A OUTPUT -m owner --uid-owner 0-5000 -j ACCEPT +-A OUTPUT -m owner --gid-owner priv_internet --suppl-groups -j ACCEPT +-A OUTPUT -d 10.0.2.2/32 -m owner --gid-owner priv_appdebugging --suppl-groups -j ACCEPT +-A OUTPUT -d 10.0.2.15/32 -m owner --gid-owner priv_appdebugging --suppl-groups -j ACCEPT +-A OUTPUT -d 192.168.129.1/32 -m owner --gid-owner priv_appdebugging --suppl-groups -j ACCEPT +-A OUTPUT -d 192.168.129.3/32 -m owner --gid-owner priv_appdebugging --suppl-groups -j ACCEPT +-A OUTPUT -j REJECT-LOG +-A REJECT-LOG -m limit --limit 1/min -j AUDIT --type reject +-A REJECT-LOG -j REJECT --reject-with icmp-net-prohibited +-A REJECT-LOG -j RETURN +COMMIT diff --git a/packaging/security-manager.spec b/packaging/security-manager.spec index 0d50227..17380e1 100644 --- a/packaging/security-manager.spec +++ b/packaging/security-manager.spec @@ -12,6 +12,9 @@ Source4: libnss-security-manager.manifest Source5: security-manager-tests.manifest Source6: security-manager-policy.manifest Source7: security-license-manager.manifest +Source8: security-manager-iptables-load.service +Source9: security-manager-iptables.rules +Source10: security-manager-ip6tables.rules Requires: security-manager-policy Requires: security-license-manager Requires: libnss-security-manager @@ -82,6 +85,15 @@ Requires(post): tizen-platform-config-tools %description policy Set of security rules that constitute security policy in the system +%package policy-iptables +Summary: Security manager iptables policy +Group: Security/Access Control +Requires: security-manager = %{version}-%{release} +Requires: iptables + +%description policy-iptables +Set of iptables rules governing the internet related priviledges + %package -n security-manager-tests Summary: Security manager unit test binaries Group: Security/Development @@ -155,6 +167,11 @@ cp -a test/data/.security-manager-test-rules*.{db,txt} %{buildroot}/%{db_test_di cp -a %{SOURCE1} %{SOURCE2} %{SOURCE3} %{SOURCE4} %{SOURCE5} %{SOURCE6} %{SOURCE7} %{buildroot}%{_datadir}/ +install -m 644 %{SOURCE8} %{buildroot}%{_unitdir}/security-manager-iptables.service +install -m 600 %{SOURCE9} %{buildroot}%{_sysconfdir}/security-manager-iptables.rules +install -m 600 %{SOURCE10} %{buildroot}%{_sysconfdir}/security-manager-ip6tables.rules +ln -sf ../security-manager-iptables.service %{buildroot}%{_unitdir}/basic.target.wants/ + %clean rm -rf %{buildroot} @@ -305,6 +322,12 @@ chsmack -a System %{db_test_dir}/.security-manager-test-rules*.txt %attr(755,root,root) %{_bindir}/security-manager-policy-reload %attr(755,root,root) %{_sysconfdir}/opt/upgrade/241.security-manager.policy-update.sh +%files -n security-manager-policy-iptables +%{_unitdir}/security-manager-iptables.service +%{_unitdir}/basic.target.wants/security-manager-iptables.service +%attr(600, root, root) %{_sysconfdir}/security-manager-iptables.rules +%attr(600, root, root) %{_sysconfdir}/security-manager-ip6tables.rules + %files -n security-manager-tests %manifest %{_datadir}/security-manager-tests.manifest %attr(755,root,root) %{_bindir}/security-manager-unit-tests -- 2.7.4 From 0c4946673a96dc23f646f99f0f2b3f7a281ecbef Mon Sep 17 00:00:00 2001 From: Lukasz Pawelczyk Date: Thu, 11 Apr 2019 17:48:40 +0200 Subject: [PATCH 08/16] Add group mapping for internal/appdebugging privilege Change-Id: I4eca8498ffec4521fcbcba3535b7c1573c9edb25 --- policy/privilege-group.list | 1 + 1 file changed, 1 insertion(+) diff --git a/policy/privilege-group.list b/policy/privilege-group.list index 2adebd8..2268a16 100644 --- a/policy/privilege-group.list +++ b/policy/privilege-group.list @@ -19,3 +19,4 @@ http://tizen.org/privilege/internal/device/audio audio http://tizen.org/privilege/internal/device/display display http://tizen.org/privilege/internal/device/video video http://tizen.org/privilege/internal/livecoredump priv_livecoredump +http://tizen.org/privilege/internal/appdebugging priv_appdebugging -- 2.7.4 From 8ec30a0ad28e54c3f2641b0d79ad487822b17248 Mon Sep 17 00:00:00 2001 From: Dariusz Michaluk Date: Thu, 16 Apr 2020 15:22:02 +0200 Subject: [PATCH 09/16] Fix security_manager_cleanup_app() After introducing sharedRO mount namespace setup, every app should cleanup own namespace after termination. Change-Id: I358007e3f47213f3038e6c3f2a05cbe5e273627f --- src/client/client-security-manager.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/client/client-security-manager.cpp b/src/client/client-security-manager.cpp index 576bcf9..8a15665 100644 --- a/src/client/client-security-manager.cpp +++ b/src/client/client-security-manager.cpp @@ -984,12 +984,8 @@ int security_manager_cleanup_app(const char *app_name, uid_t uid, pid_t pid) return SECURITY_MANAGER_ERROR_INPUT_PARAM; } - if (MountNS::isPrivacyPrivilegeMountNamespaceEnabled()) { - ClientRequest request(SecurityModuleCall::APP_CLEAN_NAMESPACE); - return request.send(std::string(app_name), uid, pid).getStatus(); - } - - return SECURITY_MANAGER_SUCCESS; + ClientRequest request(SecurityModuleCall::APP_CLEAN_NAMESPACE); + return request.send(std::string(app_name), uid, pid).getStatus(); }); } -- 2.7.4 From 437cf25c6b5677b35a9e2f18157cadf986213849 Mon Sep 17 00:00:00 2001 From: Tomasz Swierczek Date: Tue, 21 Apr 2020 14:21:11 +0200 Subject: [PATCH 10/16] Release 1.6.0 Add RPM package for iptables rules needed for GID-based internet access control Add new privilege-enforcing mechanism that uses privilege-Smack mapping Mount namespace enhancements & fixes With this release, versioning differs from branch tizen_5.5. With this release, Tizen has 3 mechanisms for controlling internet access: * nether - supports mutltiuser - allows dynamic policy change for app, during application runtime - complicated support for many protocols, many dependencies (mostly in kernel) * iptables + privilege-to-GID mapping - supports multiuser - dissallows dynamic policy change - requires patches from upstream kernel & iptables * privilege-to-Smack mapping - allows dynamic policy change - doesn't require any custom kernel changes - doesn't support simultaneous multiuser Change-Id: I9984ce4f9a761be9182535ec60ee11dbb13acc77 --- packaging/security-manager.changes | 25 +++++++++++++++++++++++++ packaging/security-manager.spec | 2 +- pc/security-manager.pc.in | 2 +- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/packaging/security-manager.changes b/packaging/security-manager.changes index 2ef67eb..db62728 100644 --- a/packaging/security-manager.changes +++ b/packaging/security-manager.changes @@ -1,3 +1,28 @@ +Release: 1.6.0 +Date: 2020.04.21 +Name: Release 1.6.0 +Description: +Fix security_manager_cleanup_app() +Add group mapping for internal/appdebugging privilege +Create new RPM for loading iptables rules at system start +Properly handle nonexisting apps uninstallation +Disable Smack privilege mapping configuration +Fix multi-user detection +Use mount namespace mount points to find running apps +Remove privilege related Smack rules when multi-user is detected +Change privilege related Smack rules on cynara policy change +Remove privilege Smack mapping rules on application uninstallation +Check if smack privilege mapping is enabled +Add Smack template files manager +Split smack API wrapper and rules management +Add restriction for privilege smack mapping rules +Change privilege and privilege status vector names for clarity +Change cynara client check to admin check for allowed privs +Add privilege-Smack mapping +Fix security-manager worker + +############################### + Release: 1.5.22 Date: 2020.04.10 Name: Release 1.5.22 diff --git a/packaging/security-manager.spec b/packaging/security-manager.spec index 17380e1..b5a8d71 100644 --- a/packaging/security-manager.spec +++ b/packaging/security-manager.spec @@ -1,6 +1,6 @@ Name: security-manager Summary: Security manager and utilities -Version: 1.5.22 +Version: 1.6.0 Release: 0 Group: Security/Service License: Apache-2.0 diff --git a/pc/security-manager.pc.in b/pc/security-manager.pc.in index 9ac13bb..db23178 100644 --- a/pc/security-manager.pc.in +++ b/pc/security-manager.pc.in @@ -5,7 +5,7 @@ includedir=${prefix}/include Name: security-manager Description: Security Manager Package -Version: 1.5.22 +Version: 1.6.0 Requires: Libs: -L${libdir} -lsecurity-manager-client Cflags: -I${includedir}/security-manager -- 2.7.4 From bf53709850c8533f5fa49dbd002e13f1db089e65 Mon Sep 17 00:00:00 2001 From: Dariusz Michaluk Date: Tue, 21 Apr 2020 13:21:25 +0200 Subject: [PATCH 11/16] Move initial namespace setup to security_manager_prepare_app_candidate() Change-Id: I43f316b8e074ff18462388b64793cbc3e2d895c1 --- src/client/client-security-manager.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/client/client-security-manager.cpp b/src/client/client-security-manager.cpp index 8a15665..f258fd5 100644 --- a/src/client/client-security-manager.cpp +++ b/src/client/client-security-manager.cpp @@ -796,14 +796,10 @@ int security_manager_drop_process_privileges(void) static int setupSharedRO(const std::string &pkg_name, bool enabledSharedRO, const std::string &userAppsRWDir, const std::string &userAppsRWSharedDir) { - int ret; + int ret = SECURITY_MANAGER_SUCCESS; std::string userPkgAppsRWSharedDir; std::string userPkgAppsRWSharedTmpDir; - ret = MountNS::makeMountSlave("/"); - if (ret != SECURITY_MANAGER_SUCCESS) - return ret; - if (enabledSharedRO) { userPkgAppsRWSharedDir = userAppsRWSharedDir + pkg_name; userPkgAppsRWSharedTmpDir = userAppsRWDir + "/.shared_tmp/" + pkg_name; @@ -878,7 +874,12 @@ int security_manager_prepare_app_candidate(void) "Abort launching the application, as it may have too high privileges and pose risk to the system."); return SECURITY_MANAGER_ERROR_INPUT_PARAM; } - return MountNS::createMountNamespace(); + + int ret = MountNS::createMountNamespace(); + if (ret != SECURITY_MANAGER_SUCCESS) + return ret; + + return MountNS::makeMountSlave("/"); } static inline int security_manager_setup_namespace_internal(const MountNS::PrivilegePathsMap &privilegePathMap, -- 2.7.4 From 5e2eee612cd01ef0aae88ed5e28692b84cf55068 Mon Sep 17 00:00:00 2001 From: Dariusz Michaluk Date: Tue, 21 Apr 2020 14:22:46 +0200 Subject: [PATCH 12/16] Properly handle ENOENT error on encrypted device Change-Id: Ica5318462304b9f96096f0376885d676e5e087ba --- src/client/client-security-manager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/client-security-manager.cpp b/src/client/client-security-manager.cpp index f258fd5..325e138 100644 --- a/src/client/client-security-manager.cpp +++ b/src/client/client-security-manager.cpp @@ -890,7 +890,7 @@ static inline int security_manager_setup_namespace_internal(const MountNS::Privi TizenPlatformConfig tpc(geteuid()); std::string userAppsRWDir = tpc.ctxGetEnv(TZ_USER_APP); std::string userAppsRWSharedDir = userAppsRWDir + "/.shared/"; - if (access(userAppsRWSharedDir.c_str(), W_OK)) { + if (FS::directoryStatus(userAppsRWSharedDir) > 0 && access(userAppsRWSharedDir.c_str(), W_OK) == -1) { LogDebug("Mount namespace setup was made by other process of multi-process app zygote"); return SECURITY_MANAGER_SUCCESS; } -- 2.7.4 From 964b061056128757a5fb67586a43a6015335b750 Mon Sep 17 00:00:00 2001 From: Dariusz Michaluk Date: Wed, 22 Apr 2020 13:51:02 +0200 Subject: [PATCH 13/16] Release 1.6.1 * Properly handle ENOENT error on encrypted device * Move initial namespace setup to security_manager_prepare_app_candidate() Change-Id: Ic99978f8d3b3b46d3322aae478bf698eb8b4f35c --- packaging/security-manager.changes | 9 +++++++++ packaging/security-manager.spec | 2 +- pc/security-manager.pc.in | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packaging/security-manager.changes b/packaging/security-manager.changes index db62728..e9617b3 100644 --- a/packaging/security-manager.changes +++ b/packaging/security-manager.changes @@ -1,3 +1,12 @@ +Release: 1.6.1 +Date: 2020.04.22 +Name: Release 1.6.1 +Description: +Properly handle ENOENT error on encrypted device +Move initial namespace setup to security_manager_prepare_app_candidate() + +############################### + Release: 1.6.0 Date: 2020.04.21 Name: Release 1.6.0 diff --git a/packaging/security-manager.spec b/packaging/security-manager.spec index b5a8d71..11100a1 100644 --- a/packaging/security-manager.spec +++ b/packaging/security-manager.spec @@ -1,6 +1,6 @@ Name: security-manager Summary: Security manager and utilities -Version: 1.6.0 +Version: 1.6.1 Release: 0 Group: Security/Service License: Apache-2.0 diff --git a/pc/security-manager.pc.in b/pc/security-manager.pc.in index db23178..0781748 100644 --- a/pc/security-manager.pc.in +++ b/pc/security-manager.pc.in @@ -5,7 +5,7 @@ includedir=${prefix}/include Name: security-manager Description: Security Manager Package -Version: 1.6.0 +Version: 1.6.1 Requires: Libs: -L${libdir} -lsecurity-manager-client Cflags: -I${includedir}/security-manager -- 2.7.4 From f29ad2910c7aa418f18bbfbf28e161c5a543acd7 Mon Sep 17 00:00:00 2001 From: Dariusz Michaluk Date: Wed, 29 Apr 2020 16:42:59 +0200 Subject: [PATCH 14/16] 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: I27f8e0d5fed42a11b96dd79fc83b36be60aeca5e --- src/common/include/mount-namespace.h | 2 +- src/common/mount-namespace.cpp | 14 ++++++++++---- src/common/worker.cpp | 15 +++++++++------ 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/common/include/mount-namespace.h b/src/common/include/mount-namespace.h index 2f4bd21..801588d 100644 --- a/src/common/include/mount-namespace.h +++ b/src/common/include/mount-namespace.h @@ -54,7 +54,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 21a3ed7..fc36b0e 100644 --- a/src/common/mount-namespace.cpp +++ b/src/common/mount-namespace.cpp @@ -164,17 +164,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 70d0d4a..a43f52d 100644 --- 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."); } -- 2.7.4 From 48444b6f5f46cdca246adbd8d1ed5160fe511741 Mon Sep 17 00:00:00 2001 From: Zofia Abramowska Date: Fri, 24 Apr 2020 17:08:33 +0200 Subject: [PATCH 15/16] Let template manager throw for configuration errors Change-Id: Iec25cd08ae5cff6ef721b77022d07f734898f773 --- src/common/include/smack-rules.h | 4 +--- src/common/include/template-manager.h | 8 ++++++++ src/common/smack-rules.cpp | 8 ++++++++ src/common/template-manager.cpp | 2 ++ 4 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/common/include/smack-rules.h b/src/common/include/smack-rules.h index 35f6d8d..53e7402 100644 --- a/src/common/include/smack-rules.h +++ b/src/common/include/smack-rules.h @@ -38,9 +38,7 @@ namespace SecurityManager { class SmackRules { public: - SmackRules() : m_templateMgr(POLICY_INSTALL_DIR) { - m_templateMgr.init(); - } + SmackRules(); /** * Install package-specific smack rules plus add rules for specified external apps. diff --git a/src/common/include/template-manager.h b/src/common/include/template-manager.h index 6844902..bb6aba9 100644 --- a/src/common/include/template-manager.h +++ b/src/common/include/template-manager.h @@ -25,12 +25,20 @@ #include #include +#include #include #include "config-file.h" namespace SecurityManager { +class TemplateManagerException { +public: + DECLARE_EXCEPTION_TYPE(SecurityManager::Exception, Base) + DECLARE_EXCEPTION_TYPE(Base, ConfigFileError) + DECLARE_EXCEPTION_TYPE(Base, ConfigParseError) +}; + class TemplateManager { public: explicit TemplateManager(const std::string &rootDir) : m_rootDir(rootDir){} diff --git a/src/common/smack-rules.cpp b/src/common/smack-rules.cpp index c205865..68aac2f 100644 --- a/src/common/smack-rules.cpp +++ b/src/common/smack-rules.cpp @@ -74,6 +74,14 @@ const std::string SMACK_APP_PATH_USER_PERMS = "rwxat"; } // namespace anonymous +SmackRules::SmackRules() : m_templateMgr(POLICY_INSTALL_DIR) { + try { + m_templateMgr.init(); + } catch (TemplateManagerException::Base &e) { + LogError("Error loading template files: " << e.DumpToString()); + } +} + void SmackRules::addFromTemplate( SmackAccesses &rules, TemplateManager::Type type, diff --git a/src/common/template-manager.cpp b/src/common/template-manager.cpp index 3f6cc09..605d9ee 100644 --- a/src/common/template-manager.cpp +++ b/src/common/template-manager.cpp @@ -57,8 +57,10 @@ void TemplateManager::init() loadFiles(); } catch (FS::Exception::Base &e) { LogError("Error loading config files: " << e.DumpToString()); + Throw(TemplateManagerException::ConfigFileError); } catch (SmackException::FileError &e) { LogError("Error parsing config files: " << e.DumpToString()); + Throw(TemplateManagerException::ConfigParseError); } } -- 2.7.4 From 3c9786a516ee2c85e41bc3b830cc9aa05d4527f9 Mon Sep 17 00:00:00 2001 From: Zofia Abramowska Date: Fri, 24 Apr 2020 17:29:03 +0200 Subject: [PATCH 16/16] Don't assume that default privilege Smack rules template exists Change-Id: I03c0fadeaf95885d191937d8c3e04fde70de047b --- src/common/template-manager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/template-manager.cpp b/src/common/template-manager.cpp index 605d9ee..c41fa69 100644 --- a/src/common/template-manager.cpp +++ b/src/common/template-manager.cpp @@ -99,7 +99,7 @@ void TemplateManager::loadFiles() } std::vector types = {Type::APP_RULES_TEMPLATE, Type::PKG_RULES_TEMPLATE, - Type::AUTHOR_RULES_TEMPLATE, Type::PRIV_DEFAULT_RULES_TEMPLATE}; + Type::AUTHOR_RULES_TEMPLATE}; for (auto &type : types) { auto path = getPolicyFile(type); m_templateRules[path] = Smack::fromConfig(path); -- 2.7.4