From 32c53d64b311b1e0a28759442d1c203afda4505c Mon Sep 17 00:00:00 2001 From: Dariusz Michaluk Date: Fri, 31 May 2019 15:10:55 +0200 Subject: [PATCH] Improve security_manager_prepare_app() performance This commit merges getPrivilegedGroups() and getAppGroups() into one client request. Change-Id: I77b42773845b264794398af7995bba087320689d --- src/client/client-security-manager.cpp | 24 ++------- src/common/include/protocols.h | 1 - src/common/include/service_impl.h | 40 ++++++-------- src/common/service_impl.cpp | 96 ++++++++++++++-------------------- src/server/service/include/service.h | 15 ++---- src/server/service/service.cpp | 29 +++------- 6 files changed, 70 insertions(+), 135 deletions(-) diff --git a/src/client/client-security-manager.cpp b/src/client/client-security-manager.cpp index cef785c..8c2a4f7 100644 --- a/src/client/client-security-manager.cpp +++ b/src/client/client-security-manager.cpp @@ -494,27 +494,16 @@ static int setProcessGroups(const std::vector &groups) return SECURITY_MANAGER_SUCCESS; } -static int getPrivilegedGroups(std::vector &groups) +static int getPrivilegedAndAppGroups(const std::string &appProcessLabel, std::vector &privilegedGroups, std::vector &appGroups) { ClientRequest request(SecurityModuleCall::GROUPS_GET); - if (request.send().failed()) { - LogError("Failed to get list of groups from security-manager service."); - return request.getStatus(); - } - - request.recv(groups); - return SECURITY_MANAGER_SUCCESS; -} -static int getAppGroups(const std::string appProcessLabel, std::vector &groups) -{ - ClientRequest request(SecurityModuleCall::APP_GET_GROUPS); if (request.send(appProcessLabel).failed()) { LogError("Failed to get list of groups from security-manager service."); return request.getStatus(); } - request.recv(groups); + request.recv(privilegedGroups, appGroups); return SECURITY_MANAGER_SUCCESS; } @@ -731,16 +720,11 @@ static int security_manager_set_process_groups_internal(const std::string &app_l return ret; LogDebug("Current supplementary groups count: " << currentGroups.size()); - std::vector privilegedGroups; - ret = getPrivilegedGroups(privilegedGroups); + std::vector privilegedGroups, allowedGroups; + ret = getPrivilegedAndAppGroups(app_label, privilegedGroups, allowedGroups); if (ret != SECURITY_MANAGER_SUCCESS) return ret; LogDebug("All privileged supplementary groups count: " << privilegedGroups.size()); - - std::vector allowedGroups; - ret = getAppGroups(app_label, allowedGroups); - if (ret != SECURITY_MANAGER_SUCCESS) - return ret; LogDebug("Allowed privileged supplementary groups count: " << allowedGroups.size()); std::unordered_set groupsSet(currentGroups.begin(), currentGroups.end()); diff --git a/src/common/include/protocols.h b/src/common/include/protocols.h index 5c61ad4..0275c0e 100644 --- a/src/common/include/protocols.h +++ b/src/common/include/protocols.h @@ -108,7 +108,6 @@ enum class SecurityModuleCall APP_UPDATE, APP_UNINSTALL, APP_GET_PKG_NAME, - APP_GET_GROUPS, APP_APPLY_PRIVATE_SHARING, APP_DROP_PRIVATE_SHARING, USER_ADD, diff --git a/src/common/include/service_impl.h b/src/common/include/service_impl.h index d213f1e..a3bd92f 100644 --- a/src/common/include/service_impl.h +++ b/src/common/include/service_impl.h @@ -112,22 +112,6 @@ public: int getPkgName(const std::string &appName, std::string &pkgName); /** - * Process query for supplementary groups allowed for the application. - * For given \ref appProcessLabel and \ref uid, calculate allowed privileges that give - * direct access to file system resources. For each permission Cynara will be - * queried. - * Returns set of group ids that are permitted. - * - * @param[in] creds credentials of the requesting process - * @param[in] appProcessLabel application identifier - * @param[out] groups returned vector of allowed groups - * - * @return API return code, as defined in protocols.h - */ - int getAppGroups(const Credentials &creds, const std::string &appProcessLabel, - std::vector &groups); - - /** * Process user adding request. * * @param[in] creds credentials of the requesting process @@ -186,20 +170,28 @@ public: /** * Process getting policy descriptions list. * - * @param[in] descriptions empty vector for descriptions strings + * @param[out] descriptions returned vector of descriptions strings * * @return API return code, as defined in protocols.h */ int policyGetDesc(std::vector &descriptions); /** - * Process getting resources group list. - * - * @param[out] groups empty vector for group strings - * - * @return API return code, as defined in protocols.h - */ - int policyGetGroups(std::vector &groups); + * Process query for resources group list and supplementary groups allowed for the application. + * For given \ref appProcessLabel and \ref uid, calculate allowed privileges that give + * direct access to file system resources. For each permission Cynara will be + * queried. + * Returns set of group ids that are permitted. + * + * @param[in] creds credentials of the requesting process + * @param[in] appProcessLabel application identifier + * @param[out] privilegedGroups returned vector of privileged groups + * @param[out] appGroups returned vector of allowed groups + * + * @return API return code, as defined in protocols.h + */ + int getPrivilegedAndAppGroups(const Credentials &creds, const std::string &appProcessLabel, + std::vector &privilegedGroups, std::vector &appGroups); /** * Receive groups connected with uid and add them diff --git a/src/common/service_impl.cpp b/src/common/service_impl.cpp index d8d9f9e..8362481 100644 --- a/src/common/service_impl.cpp +++ b/src/common/service_impl.cpp @@ -1175,56 +1175,6 @@ int ServiceImpl::getPkgName(const std::string &appName, std::string &pkgName) return SECURITY_MANAGER_SUCCESS; } -int ServiceImpl::getAppGroups(const Credentials &creds, const std::string &appProcessLabel, - std::vector &groups) -{ - try { - LogDebug("smack label: " << appProcessLabel); - - std::vector privileges; - - std::string uidStr = std::to_string(creds.uid); - m_cynaraAdmin.getAppPolicy(appProcessLabel, uidStr, privileges); - m_cynaraAdmin.getAppPolicy(appProcessLabel, CYNARA_ADMIN_WILDCARD, privileges); - m_cynaraAdmin.getAppPolicy(CYNARA_ADMIN_WILDCARD, CYNARA_ADMIN_WILDCARD, privileges); - - vectorRemoveDuplicates(privileges); - - std::string pidStr = std::to_string(creds.pid); - for (const auto &privilege : privileges) { - auto &pgids = m_privilegeGids.getGids(privilege); - - LogDebug("Considering privilege " << privilege << " with " << - pgids.size() << " groups assigned"); - - if (pgids.empty()) - continue; - - if (m_cynara.check(appProcessLabel, privilege, uidStr, pidStr)) { - groups.insert(groups.end(), pgids.begin(), pgids.end()); - LogDebug("Cynara allowed, adding groups"); - } else { - LogDebug("Cynara denied, not adding groups"); - } - } - vectorRemoveDuplicates(groups); - } catch (const PrivilegeDb::Exception::Base &e) { - LogError("Database error: " << e.DumpToString()); - return SECURITY_MANAGER_ERROR_SERVER_ERROR; - } catch (const CynaraException::Base &e) { - LogError("Error while querying Cynara for permissions: " << e.DumpToString()); - return SECURITY_MANAGER_ERROR_SERVER_ERROR; - } catch (const SmackException::InvalidLabel &e) { - LogError("Error while generating Smack labels: " << e.DumpToString()); - return SECURITY_MANAGER_ERROR_SERVER_ERROR; - } catch (const std::bad_alloc &e) { - LogError("Memory allocation failed: " << e.what()); - return SECURITY_MANAGER_ERROR_MEMORY; - } - - return SECURITY_MANAGER_SUCCESS; -} - int ServiceImpl::userAdd(const Credentials &creds, uid_t uidAdded, int userType) { if (!authenticate(creds, PRIVILEGE_USER_ADMIN)) { @@ -1620,19 +1570,51 @@ int ServiceImpl::policyGetDesc(std::vector &levels) return ret; } -int ServiceImpl::policyGetGroups(std::vector &groups) +int ServiceImpl::getPrivilegedAndAppGroups(const Credentials &creds, const std::string &appProcessLabel, + std::vector &privilegedGroups, std::vector &appGroups) { - int ret = SECURITY_MANAGER_SUCCESS; - try { + LogDebug("smack label: " << appProcessLabel); + auto &gids = m_privilegeGids.getGids(); - groups.insert(groups.end(), gids.begin(), gids.end()); - } catch (const PrivilegeDb::Exception::Base &e) { - LogError("Error while getting groups from database: " << e.DumpToString()); + privilegedGroups.insert(privilegedGroups.end(), gids.begin(), gids.end()); + + std::vector privileges; + + std::string uidStr = std::to_string(creds.uid); + m_cynaraAdmin.getAppPolicy(appProcessLabel, uidStr, privileges); + m_cynaraAdmin.getAppPolicy(appProcessLabel, CYNARA_ADMIN_WILDCARD, privileges); + m_cynaraAdmin.getAppPolicy(CYNARA_ADMIN_WILDCARD, CYNARA_ADMIN_WILDCARD, privileges); + + vectorRemoveDuplicates(privileges); + + std::string pidStr = std::to_string(creds.pid); + for (const auto &privilege : privileges) { + const auto &pgids = m_privilegeGids.getGids(privilege); + + LogDebug("Considering privilege " << privilege << " with " << + pgids.size() << " groups assigned"); + + if (pgids.empty()) + continue; + + if (m_cynara.check(appProcessLabel, privilege, uidStr, pidStr)) { + appGroups.insert(appGroups.end(), pgids.begin(), pgids.end()); + LogDebug("Cynara allowed, adding groups"); + } else { + LogDebug("Cynara denied, not adding groups"); + } + } + vectorRemoveDuplicates(appGroups); + } catch (const CynaraException::Base &e) { + LogError("Error while querying Cynara for permissions: " << e.DumpToString()); return SECURITY_MANAGER_ERROR_SERVER_ERROR; + } catch (const std::bad_alloc &e) { + LogError("Memory allocation failed: " << e.what()); + return SECURITY_MANAGER_ERROR_MEMORY; } - return ret; + return SECURITY_MANAGER_SUCCESS; } int ServiceImpl::policyGroupsForUid(uid_t uid, std::vector &groups) diff --git a/src/server/service/include/service.h b/src/server/service/include/service.h index 5805cf0..1c697b0 100644 --- a/src/server/service/include/service.h +++ b/src/server/service/include/service.h @@ -104,15 +104,6 @@ private: */ void processGetPkgName(MessageBuffer &buffer, MessageBuffer &send); - /** - * Process getting permitted group ids for app id - * - * @param buffer Raw received data buffer - * @param send Raw data buffer to be sent - * @param creds credentials of the requesting process - */ - void processGetAppGroups(MessageBuffer &buffer, MessageBuffer &send, const Credentials &creds); - void processUserAdd(MessageBuffer &buffer, MessageBuffer &send, const Credentials &creds); void processUserDelete(MessageBuffer &buffer, MessageBuffer &send, const Credentials &creds); @@ -160,11 +151,13 @@ private: void processPolicyGetDesc(MessageBuffer &send); /** - * Process getting groups bound with privileges + * Process getting groups bound with privileges and permitted group ids for app id * + * @param buffer Raw received data buffer * @param send Raw data buffer to be sent + * @param creds credentials of the requesting process */ - void processGroupsGet(MessageBuffer &send); + void processGetPrivilegedAndAppGroups(MessageBuffer &buffer, MessageBuffer &send, const Credentials &creds); /** * Process getting groups bound with privileges for given uid diff --git a/src/server/service/service.cpp b/src/server/service/service.cpp index d881866..043f4a8 100644 --- a/src/server/service/service.cpp +++ b/src/server/service/service.cpp @@ -93,10 +93,6 @@ bool Service::processOne(const ConnectionID &conn, MessageBuffer &buffer, LogDebug("call_type: SecurityModuleCall::APP_GET_PKG_NAME"); processGetPkgName(buffer, send); break; - case SecurityModuleCall::APP_GET_GROUPS: - LogDebug("call_type: SecurityModuleCall::APP_GET_GROUPS"); - processGetAppGroups(buffer, send, creds); - break; case SecurityModuleCall::USER_ADD: LogDebug("call_type: SecurityModuleCall::USER_ADD"); processUserAdd(buffer, send, creds); @@ -127,7 +123,7 @@ bool Service::processOne(const ConnectionID &conn, MessageBuffer &buffer, break; case SecurityModuleCall::GROUPS_GET: LogDebug("call_type: SecurityModuleCall::GROUPS_GET"); - processGroupsGet(send); + processGetPrivilegedAndAppGroups(buffer, send, creds); break; case SecurityModuleCall::GROUPS_FOR_UID: processGroupsForUid(buffer, send); @@ -265,19 +261,6 @@ void Service::processGetPkgName(MessageBuffer &buffer, MessageBuffer &send) Serialization::Serialize(send, pkgName); } -void Service::processGetAppGroups(MessageBuffer &buffer, MessageBuffer &send, const Credentials &creds) -{ - std::string appProcessLabel; - std::vector groups; - int ret; - - Deserialization::Deserialize(buffer, appProcessLabel); - ret = serviceImpl.getAppGroups(creds, appProcessLabel, groups); - Serialization::Serialize(send, ret); - if (ret == SECURITY_MANAGER_SUCCESS) - Serialization::Serialize(send, groups); -} - void Service::processUserAdd(MessageBuffer &buffer, MessageBuffer &send, const Credentials &creds) { int ret; @@ -362,14 +345,16 @@ void Service::processPolicyGetDesc(MessageBuffer &send) } } -void Service::processGroupsGet(MessageBuffer &send) +void Service::processGetPrivilegedAndAppGroups(MessageBuffer &buffer, MessageBuffer &send, const Credentials &creds) { - std::vector groups; - int ret = serviceImpl.policyGetGroups(groups); + std::string appProcessLabel; + std::vector privilegedGroups, appGroups; + Deserialization::Deserialize(buffer, appProcessLabel); + int ret = serviceImpl.getPrivilegedAndAppGroups(creds, appProcessLabel, privilegedGroups, appGroups); Serialization::Serialize(send, ret); if (ret == SECURITY_MANAGER_SUCCESS) { - Serialization::Serialize(send, groups); + Serialization::Serialize(send, privilegedGroups, appGroups); } } -- 2.7.4