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 (file)
--- a/src/common/mount-namespace.cpp
+++ b/src/common/mount-namespace.cpp
@@ -283,11 +283,8 @@ std::vector<AppContext> 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<AppContext> 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 (file)
--- 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<policy
     } catch (const CynaraException::Base &e) {
         LogError("Error while updating Cynara rules: " << e.DumpToString());
         return SECURITY_MANAGER_ERROR_SERVER_ERROR;
+    } catch (const FS::Exception::Base &e) {
+        LogError("Filesystem error: " << e.DumpToString());
+        return SECURITY_MANAGER_ERROR_SERVER_ERROR;
     } catch (const std::bad_alloc &e) {
         LogError("Memory allocation error while updating Cynara rules: " << e.what());
         return SECURITY_MANAGER_ERROR_SERVER_ERROR;
@@ -2254,47 +2257,52 @@ int ServiceImpl::prepareApp(const Credentials &creds, const std::string &appName
         Smack::Label &label, std::string &pkgName, PrepareAppFlags &prepareAppFlags,
         std::vector<gid_t> &forbiddenGroups, std::vector<gid_t> &allowedGroups, std::vector<bool> &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<std::string> 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<std::string> 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<std::string> pkgLabels;
-    getPkgLabels(pkgName, pkgLabels);
+        std::vector<std::string> 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 */