From: Mateusz Cegielka Date: Mon, 28 Sep 2020 16:25:51 +0000 (+0200) Subject: Fix segfault when iterating directories X-Git-Tag: accepted/tizen/unified/20201006.044326~1 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=652c7df4bdfec136ff6d691de14080324cbdc91a;p=platform%2Fcore%2Fsecurity%2Fsecurity-manager.git Fix segfault when iterating directories Code used for iterating directories recursively with Boost calls .pop() if the iteration returns an error, so that it exits the current directory and continues the iteration. However, this can cause segmentation faults, and if it doesn't, it causes some other directories to be indeterministically skipped instead. What is the proper way to do this then...? Boost apparently does not place too much focus on stability, because the behaviour is different in every version I checked (1.65.0 from Ubuntu 18.04, 1.71.0 from Tizen and 1.72.0 from Arch). Also, since 1.72.0 it'll be impossible to both continue the iteration and log that anything was wrong. I changed the behaviour to stop iteration on errors and return an internal error instead. The immediate reason is making sure a Boost update won't break this code, but a system service receiving filesystem errors in directories it created is a pathological case indicating other problems with system configuration that should not be accepted. Change-Id: I69b7fb75f2b58d0ca1418b6bbb3ccd2480296918 --- diff --git a/src/common/mount-namespace.cpp b/src/common/mount-namespace.cpp index fe4d4c0..f3c8635 100644 --- a/src/common/mount-namespace.cpp +++ b/src/common/mount-namespace.cpp @@ -283,11 +283,8 @@ std::vector getMountNSApps() it != eit; it.increment(ec)) { - if (ec) { - LogWarning("Failed to process " + it->path().string()); - it.pop(); - continue; - } + if (ec) // Old Boost versions compatibility. + break; boost::smatch matches; std::string path = it->path().string(); @@ -300,6 +297,9 @@ std::vector getMountNSApps() } } + if (ec) + ThrowMsg(FS::Exception::FileError, + "Error scanning app mount points at " << rootDir << ", " << ec.message()); return apps; } diff --git a/src/common/service_impl.cpp b/src/common/service_impl.cpp index 43de45f..7575ab1 100644 --- a/src/common/service_impl.cpp +++ b/src/common/service_impl.cpp @@ -1213,6 +1213,9 @@ int ServiceImpl::policyUpdate(const Credentials &creds, const std::vector &forbiddenGroups, std::vector &allowedGroups, std::vector &privPathsStatusVector) { - LogDebug("Requested prepareApp for application " << appName); + try { + LogDebug("Requested prepareApp for application " << appName); - bool isHybrid, enabledSharedRO; - if (!m_privilegeDb.GetAppPkgInfo(appName, pkgName, isHybrid, enabledSharedRO)) - return SECURITY_MANAGER_ERROR_UNKNOWN; - prepareAppFlags = m_prepareAppFlags | (enabledSharedRO ? PREPARE_APP_SHARED_RO_FLAG : 0); - label = SmackLabels::generateProcessLabel(appName, pkgName, isHybrid); + bool isHybrid, enabledSharedRO; + if (!m_privilegeDb.GetAppPkgInfo(appName, pkgName, isHybrid, enabledSharedRO)) + return SECURITY_MANAGER_ERROR_UNKNOWN; + prepareAppFlags = m_prepareAppFlags | (enabledSharedRO ? PREPARE_APP_SHARED_RO_FLAG : 0); + label = SmackLabels::generateProcessLabel(appName, pkgName, isHybrid); - std::vector allowedPrivileges; - int ret = getAppAllowedPrivileges(label, creds.uid, allowedPrivileges); - if (ret != SECURITY_MANAGER_SUCCESS) { - LogError("Failed to fetch allowed privileges for " << label); - return ret; - } + std::vector allowedPrivileges; + int ret = getAppAllowedPrivileges(label, creds.uid, allowedPrivileges); + if (ret != SECURITY_MANAGER_SUCCESS) { + LogError("Failed to fetch allowed privileges for " << label); + return ret; + } - std::string authorHash; - m_privilegeDb.GetPkgAuthorHash(pkgName, authorHash); + std::string authorHash; + m_privilegeDb.GetPkgAuthorHash(pkgName, authorHash); - std::vector pkgLabels; - getPkgLabels(pkgName, pkgLabels); + std::vector pkgLabels; + getPkgLabels(pkgName, pkgLabels); - if (m_smackRules.isPrivilegeMappingEnabled()) { - // 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, authorHash); + if (m_smackRules.isPrivilegeMappingEnabled()) { + // 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, authorHash); - // 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, authorHash, allowedPrivileges); + // 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, authorHash, allowedPrivileges); + } } - } - ret = getForbiddenAndAllowedGroups(label, allowedPrivileges, forbiddenGroups, - allowedGroups); - return ret != SECURITY_MANAGER_SUCCESS ? ret - : appSetupNamespace(creds, label, privPathsVector, privPathsStatusVector); + ret = getForbiddenAndAllowedGroups(label, allowedPrivileges, forbiddenGroups, + allowedGroups); + return ret != SECURITY_MANAGER_SUCCESS ? ret + : appSetupNamespace(creds, label, privPathsVector, privPathsStatusVector); + } catch (const FS::Exception::Base &e) { + LogError("Filesystem error: " << e.DumpToString()); + return SECURITY_MANAGER_ERROR_SERVER_ERROR; + } } } /* namespace SecurityManager */