From 075995ce5550be68604cedff2951a35b31c907ca Mon Sep 17 00:00:00 2001 From: Witek Krecicki Date: Fri, 31 Jul 2026 14:02:10 +0000 Subject: [PATCH 1/2] Allow daemon-owned logfiles (cherry picked from commit b603647ddc2fca9cf9a3777be8cda26620cdd2a1) --- .../src/lib/procedures/LogFilePermissions.cpp | 85 ++++++++++++++--- .../procedures/LogFilePermissionsTest.cpp | 92 +++++++++++++++++++ 2 files changed, 166 insertions(+), 11 deletions(-) diff --git a/src/modules/complianceengine/src/lib/procedures/LogFilePermissions.cpp b/src/modules/complianceengine/src/lib/procedures/LogFilePermissions.cpp index c1b868044..2517db016 100644 --- a/src/modules/complianceengine/src/lib/procedures/LogFilePermissions.cpp +++ b/src/modules/complianceengine/src/lib/procedures/LogFilePermissions.cpp @@ -5,11 +5,15 @@ #include #include #include +#include #include #include #include +#include #include #include +#include +#include #include #include @@ -45,10 +49,46 @@ const std::map> g_logfilePattern {"*.journal~", {{"owner", "root"}, {"group", "root|systemd-journal"}, {"mask", "0137"}}}, }; -// Default mask for files that don't match any pattern -// TODO(wpk) add the magic related to daemon-owned files. +// Default arguments for files that don't match any pattern. const std::map g_defaultLogfileArgs = {{"owner", "root|syslog"}, {"group", "root|adm"}, {"mask", "0137"}}; +using DaemonUidSet = std::set; + +// Builds the set of uids that belong to root or a daemon/service account (one whose login shell is +// not a valid interactive shell per /etc/shells). Computed once from /etc/passwd so we don't have to +// look up each file's owner while walking the log directory. +Result BuildDaemonUidSet(ContextInterface& context) +{ + auto validShells = ListValidShells(context); + if (!validShells.HasValue()) + { + OsConfigLogError(context.GetLogHandle(), "Failed to list valid shells: %s", validShells.Error().message.c_str()); + OSConfigTelemetryStatusTrace("ListValidShells", validShells.Error().code); + return validShells.Error(); + } + + auto users = UsersRange::Make(context.GetSpecialFilePath("/etc/passwd"), context.GetLogHandle()); + if (!users.HasValue()) + { + OsConfigLogError(context.GetLogHandle(), "Failed to enumerate users: %s", users.Error().message.c_str()); + OSConfigTelemetryStatusTrace("UsersRange", users.Error().code); + return users.Error(); + } + + DaemonUidSet daemonUids; + for (const auto& user : users.Value()) + { + const std::string name = (user.pw_name != nullptr) ? user.pw_name : std::string(); + const std::string shell = (user.pw_shell != nullptr) ? user.pw_shell : std::string(); + if (name == "root" || validShells.Value().find(shell) == validShells.Value().end()) + { + daemonUids.insert(user.pw_uid); + } + } + + return daemonUids; +} + FilePermissionsParams GetFilePermissionsParams(const std::string& filename, std::map args) { args["path"] = filename; @@ -60,7 +100,18 @@ FilePermissionsParams GetFilePermissionsParams(const std::string& filename, std: return result.Value(); } -FilePermissionsParams GetFilePermissionArgs(const std::string& filename, const std::string& fullPath) +FilePermissionsParams GetDefaultFilePermissionArgs(const std::string& fullPath, const struct stat& statInfo, const DaemonUidSet& daemonUids, bool remediate) +{ + if (!remediate && daemonUids.find(statInfo.st_uid) != daemonUids.end()) + { + return GetFilePermissionsParams(fullPath, {{"mask", "0137"}}); + } + + return GetFilePermissionsParams(fullPath, g_defaultLogfileArgs); +} + +FilePermissionsParams GetFilePermissionArgs(const std::string& filename, const std::string& fullPath, const struct stat& statInfo, + const DaemonUidSet& daemonUids, bool remediate) { for (const auto& pattern : g_logfilePatterns) { @@ -70,11 +121,11 @@ FilePermissionsParams GetFilePermissionArgs(const std::string& filename, const s } } - return GetFilePermissionsParams(fullPath, g_defaultLogfileArgs); + return GetDefaultFilePermissionArgs(fullPath, statInfo, daemonUids, remediate); } -Result ProcessLogfile(const std::string& path, const std::string& filename, const struct stat& statInfo, IndicatorsTree& indicators, - ContextInterface& context, bool remediate) +Result ProcessLogfile(const std::string& path, const std::string& filename, const struct stat& statInfo, const DaemonUidSet& daemonUids, + IndicatorsTree& indicators, ContextInterface& context, bool remediate) { if (S_ISDIR(statInfo.st_mode)) { @@ -93,7 +144,7 @@ Result ProcessLogfile(const std::string& path, const std::string& filena } const std::string fullPath = path + "/" + filename; - const auto params = GetFilePermissionArgs(filename, fullPath); + const auto params = GetFilePermissionArgs(filename, fullPath, statInfo, daemonUids, remediate); OsConfigLogDebug(context.GetLogHandle(), "Processing logfile: %s with pattern-matched permissions", fullPath.c_str()); @@ -126,8 +177,14 @@ Result AuditLogFilePermissions(const LogFilePermissionsParams& params, I assert(params.path.HasValue()); OsConfigLogInfo(context.GetLogHandle(), "Auditing logfile access permissions in directory: %s", params.path->c_str()); - auto callback = [&indicators, &context](const std::string& dirPath, const std::string& filename, const struct stat& statInfo) -> Result { - return ProcessLogfile(dirPath, filename, statInfo, indicators, context, false); + auto daemonUids = BuildDaemonUidSet(context); + if (!daemonUids.HasValue()) + { + return daemonUids.Error(); + } + + auto callback = [&daemonUids, &indicators, &context](const std::string& dirPath, const std::string& filename, const struct stat& statInfo) -> Result { + return ProcessLogfile(dirPath, filename, statInfo, daemonUids.Value(), indicators, context, false); }; auto result = FileTreeWalk(params.path.Value(), callback, BreakOnNonCompliant::False, context); @@ -154,8 +211,14 @@ Result RemediateLogFilePermissions(const LogFilePermissionsParams& param assert(params.path.HasValue()); OsConfigLogInfo(context.GetLogHandle(), "Remediating logfile access permissions in directory: %s", params.path->c_str()); - auto callback = [&indicators, &context](const std::string& dirPath, const std::string& filename, const struct stat& statInfo) -> Result { - return ProcessLogfile(dirPath, filename, statInfo, indicators, context, true); + auto daemonUids = BuildDaemonUidSet(context); + if (!daemonUids.HasValue()) + { + return daemonUids.Error(); + } + + auto callback = [&daemonUids, &indicators, &context](const std::string& dirPath, const std::string& filename, const struct stat& statInfo) -> Result { + return ProcessLogfile(dirPath, filename, statInfo, daemonUids.Value(), indicators, context, true); }; auto result = FileTreeWalk(params.path.Value(), callback, BreakOnNonCompliant::False, context); diff --git a/src/modules/complianceengine/tests/procedures/LogFilePermissionsTest.cpp b/src/modules/complianceengine/tests/procedures/LogFilePermissionsTest.cpp index ea2fbcfa5..0fc47459d 100644 --- a/src/modules/complianceengine/tests/procedures/LogFilePermissionsTest.cpp +++ b/src/modules/complianceengine/tests/procedures/LogFilePermissionsTest.cpp @@ -50,6 +50,14 @@ class EnsureLogfileAccessTest : public ::testing::Test system("useradd -g adm syslog >/dev/null"); system("groupadd systemd-journal >/dev/null"); + // A daemon/service account: no valid interactive login shell. + system("groupadd logdaemon >/dev/null 2>&1"); + system("useradd -M -g logdaemon -s /usr/sbin/nologin logdaemon >/dev/null 2>&1"); + + // A regular interactive account: has a valid login shell. + system("groupadd loguser >/dev/null 2>&1"); + system("useradd -M -g loguser -s /bin/bash loguser >/dev/null 2>&1"); + testDir = mkdtemp(dirTemplate); ASSERT_FALSE(testDir.empty()); indicators.Push("EnsureLogfileAccess"); @@ -418,3 +426,87 @@ TEST_F(EnsureLogfileAccessTest, SpecialSystemLogFiles) ASSERT_TRUE(result.HasValue()); ASSERT_EQ(result.Value(), Status::Compliant); } + +// A file matching no known pattern that is owned by a daemon/service account (one without a valid +// login shell) only needs an acceptable permission mask; its owner/group are not constrained. +TEST_F(EnsureLogfileAccessTest, AuditDaemonOwnedFileWithNonDefaultOwnershipIsCompliant) +{ + if (getpwnam("logdaemon") == nullptr) + { + GTEST_SKIP() << "requires the 'logdaemon' service account to be created"; + } + + // Constrain the set of valid login shells so that /usr/sbin/nologin is not one of them. + mContext.SetSpecialFilePath("/etc/shells", mContext.MakeTempfile("/bin/sh\n/bin/bash\n")); + + // Default-pattern file owned by a daemon account with a mask-compliant mode (0640 & 0137 == 0). + CreateLogFile("customservice.log", "logdaemon", "logdaemon", 0640); + + LogFilePermissionsParams params; + params.path = testDir; + + auto result = AuditLogFilePermissions(params, indicators, mContext); + ASSERT_TRUE(result.HasValue()); + ASSERT_EQ(result.Value(), Status::Compliant); +} + +// A root-owned file matching no known pattern is likewise only checked against the mask, regardless +// of its group (root is treated as a daemon/service account). +TEST_F(EnsureLogfileAccessTest, AuditRootOwnedFileWithNonDefaultGroupIsCompliant) +{ + mContext.SetSpecialFilePath("/etc/shells", mContext.MakeTempfile("/bin/sh\n/bin/bash\n")); + + // Group is neither root nor adm, but ownership is not enforced for daemon-owned files. + CreateLogFile("customservice.log", "root", "bin", 0640); + + LogFilePermissionsParams params; + params.path = testDir; + + auto result = AuditLogFilePermissions(params, indicators, mContext); + ASSERT_TRUE(result.HasValue()); + ASSERT_EQ(result.Value(), Status::Compliant); +} + +// A daemon-owned default-pattern file must still satisfy the permission mask. +TEST_F(EnsureLogfileAccessTest, AuditDaemonOwnedFileWithBadMaskIsNonCompliant) +{ + if (getpwnam("logdaemon") == nullptr) + { + GTEST_SKIP() << "requires the 'logdaemon' service account to be created"; + } + + mContext.SetSpecialFilePath("/etc/shells", mContext.MakeTempfile("/bin/sh\n/bin/bash\n")); + + // 0644 & 0137 != 0 (world-readable), so the mask check fails even though the owner is a daemon. + CreateLogFile("customservice.log", "logdaemon", "logdaemon", 0644); + + LogFilePermissionsParams params; + params.path = testDir; + + auto result = AuditLogFilePermissions(params, indicators, mContext); + ASSERT_TRUE(result.HasValue()); + ASSERT_EQ(result.Value(), Status::NonCompliant); +} + +// A file matching no known pattern owned by a regular interactive account must satisfy the default +// ownership (root|syslog : root|adm) even when the permission mask is acceptable. +TEST_F(EnsureLogfileAccessTest, AuditInteractiveUserOwnedFileEnforcesOwnership) +{ + if (getpwnam("loguser") == nullptr) + { + GTEST_SKIP() << "requires the 'loguser' interactive account to be created"; + } + + // /bin/bash is a valid login shell, so 'loguser' is not treated as a daemon account. + mContext.SetSpecialFilePath("/etc/shells", mContext.MakeTempfile("/bin/sh\n/bin/bash\n")); + + // Mask 0640 is fine, but loguser:loguser is not an allowed default owner/group. + CreateLogFile("customservice.log", "loguser", "loguser", 0640); + + LogFilePermissionsParams params; + params.path = testDir; + + auto result = AuditLogFilePermissions(params, indicators, mContext); + ASSERT_TRUE(result.HasValue()); + ASSERT_EQ(result.Value(), Status::NonCompliant); +} From 00d8f57f75d8ffa35712be74d2d7b5ab6b77babb Mon Sep 17 00:00:00 2001 From: Witek Krecicki Date: Fri, 31 Jul 2026 14:01:42 +0000 Subject: [PATCH 2/2] Require blocklist only for kernel modules that exist in running kernel (cherry picked from commit d1c76b8f9058957675544c05819378b5fd170c8a) --- .../src/lib/KernelModuleTools.cpp | 109 ++++++++++++++++-- .../src/lib/KernelModuleTools.h | 3 +- .../src/lib/procedures/KernelModule.cpp | 8 +- .../src/lib/procedures/WirelessDisabled.cpp | 2 +- .../tests/procedures/KernelModuleTest.cpp | 31 ++++- 5 files changed, 136 insertions(+), 17 deletions(-) diff --git a/src/modules/complianceengine/src/lib/KernelModuleTools.cpp b/src/modules/complianceengine/src/lib/KernelModuleTools.cpp index 0350219c9..01995d769 100644 --- a/src/modules/complianceengine/src/lib/KernelModuleTools.cpp +++ b/src/modules/complianceengine/src/lib/KernelModuleTools.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -137,7 +138,88 @@ Result IsKernelModuleLoaded(std::string moduleName, ContextInterface& cont return false; } -Result IsKernelModuleBlocked(std::string moduleName, IndicatorsTree& indicators, ContextInterface& context) +// Reads the currently-running kernel release (equivalent to `uname -r`) from +// /proc/sys/kernel/osrelease. Returns an empty string if it cannot be read. +static std::string GetRunningKernelRelease(ContextInterface& context) +{ + std::ifstream ifs(context.GetSpecialFilePath("/proc/sys/kernel/osrelease")); + std::string release; + std::getline(ifs, release); + while (!release.empty() && (release.back() == '\n' || release.back() == '\r' || release.back() == ' ' || release.back() == '\t')) + { + release.pop_back(); + } + return release; +} + +// Returns true if the module object file is present in the currently-running +// kernel's module directory (/lib/modules/$(uname -r)/kernel). +// +// CIS only requires the "install /bin/false" masking when the module is +// loadable in the running kernel. When the module exists only in a non-running +// installed kernel (or not at all in the running kernel), deny-listing alone is +// sufficient, so the mask must not be required in that case. +Result IsModuleAvailableInRunningKernel(const std::string& moduleName, ContextInterface& context) +{ + std::string release = GetRunningKernelRelease(context); + if (release.empty()) + { + // Unable to determine the running kernel; be conservative and assume the module + // may be loadable so the stricter mask requirement still applies. + return true; + } + + std::string kernelDirPath = context.GetSpecialFilePath("/lib/modules") + "/" + release + "/kernel"; + struct stat st; + if (stat(kernelDirPath.c_str(), &st) != 0) + { + if (errno == ENOENT) + { + return false; + } + OsConfigLogError(context.GetLogHandle(), "Failed to stat %s - errno %d", kernelDirPath.c_str(), errno); + OSConfigTelemetryStatusTrace("stat", errno); + return true; + } + if (!S_ISDIR(st.st_mode)) + { + return false; + } + + char* paths[] = {const_cast(kernelDirPath.c_str()), nullptr}; + FTS* fts = fts_open(paths, FTS_PHYSICAL, nullptr); + if (!fts) + { + OsConfigLogError(context.GetLogHandle(), "Failed to open %s - errno %d", kernelDirPath.c_str(), errno); + OSConfigTelemetryStatusTrace("fts_open", errno); + return false; + } + auto ftsDeleter = std::unique_ptr(fts, fts_close); + + std::string target = moduleName + ".ko"; + std::string overlayTarget = moduleName + "_overlay.ko"; + std::string targetUnderscore = target; + std::replace(targetUnderscore.begin(), targetUnderscore.end(), '-', '_'); + std::string overlayTargetUnderscore = overlayTarget; + std::replace(overlayTargetUnderscore.begin(), overlayTargetUnderscore.end(), '-', '_'); + + FTSENT* node = nullptr; + while ((node = fts_read(fts)) != nullptr) + { + if (node->fts_info != FTS_F) + { + continue; + } + std::string baseName = node->fts_name; + if (baseName.find(target) == 0 || baseName.find(targetUnderscore) == 0 || baseName.find(overlayTarget) == 0 || baseName.find(overlayTargetUnderscore) == 0) + { + return true; + } + } + return false; +} + +Result IsKernelModuleBlocked(std::string moduleName, bool requireMask, IndicatorsTree& indicators, ContextInterface& context) { Result modprobeOutput = context.ExecuteCommand("modprobe --showconfig"); if (modprobeOutput.HasValue()) @@ -158,18 +240,21 @@ Result IsKernelModuleBlocked(std::string moduleName, IndicatorsTree& ind return indicators.NonCompliant("Module " + moduleName + " is not blacklisted in modprobe configuration"); } - regex modprobeInstallRegex; - try + if (requireMask) { - modprobeInstallRegex = regex("^install\\s+" + UnderscoreForRegex(moduleName) + "\\s+(/usr)?/bin/(true|false)"); - } - catch (std::exception& e) - { - return Error(e.what()); - } - if (!MultilineRegexSearch(modprobeOutput.Value(), modprobeInstallRegex)) - { - return indicators.NonCompliant("Module " + moduleName + " is not masked in modprobe configuration"); + regex modprobeInstallRegex; + try + { + modprobeInstallRegex = regex("^install\\s+" + UnderscoreForRegex(moduleName) + "\\s+(/usr)?/bin/(true|false)"); + } + catch (std::exception& e) + { + return Error(e.what()); + } + if (!MultilineRegexSearch(modprobeOutput.Value(), modprobeInstallRegex)) + { + return indicators.NonCompliant("Module " + moduleName + " is not masked in modprobe configuration"); + } } } else diff --git a/src/modules/complianceengine/src/lib/KernelModuleTools.h b/src/modules/complianceengine/src/lib/KernelModuleTools.h index f4481a60b..62dbb1762 100644 --- a/src/modules/complianceengine/src/lib/KernelModuleTools.h +++ b/src/modules/complianceengine/src/lib/KernelModuleTools.h @@ -13,7 +13,8 @@ namespace ComplianceEngine Result SearchFilesystemForModuleName(std::string& moduleName, ContextInterface& context); Result IsKernelModuleLoaded(std::string moduleName, ContextInterface& context); -Result IsKernelModuleBlocked(std::string moduleName, IndicatorsTree& indicators, ContextInterface& context); +Result IsModuleAvailableInRunningKernel(const std::string& moduleName, ContextInterface& context); +Result IsKernelModuleBlocked(std::string moduleName, bool requireMask, IndicatorsTree& indicators, ContextInterface& context); } // namespace ComplianceEngine diff --git a/src/modules/complianceengine/src/lib/procedures/KernelModule.cpp b/src/modules/complianceengine/src/lib/procedures/KernelModule.cpp index 0d01fa9b3..d25dac317 100644 --- a/src/modules/complianceengine/src/lib/procedures/KernelModule.cpp +++ b/src/modules/complianceengine/src/lib/procedures/KernelModule.cpp @@ -32,7 +32,13 @@ Result AuditKernelModuleUnavailable(const KernelModuleUnavailableParams& return indicators.NonCompliant("Module " + moduleName + " is loaded"); } - return IsKernelModuleBlocked(moduleName, indicators, context); + auto inRunningKernel = IsModuleAvailableInRunningKernel(moduleName, context); + if (!inRunningKernel.HasValue()) + { + return Result(inRunningKernel.Error()); + } + + return IsKernelModuleBlocked(moduleName, inRunningKernel.Value(), indicators, context); } } // namespace ComplianceEngine diff --git a/src/modules/complianceengine/src/lib/procedures/WirelessDisabled.cpp b/src/modules/complianceengine/src/lib/procedures/WirelessDisabled.cpp index eaa7c9915..d869fa304 100644 --- a/src/modules/complianceengine/src/lib/procedures/WirelessDisabled.cpp +++ b/src/modules/complianceengine/src/lib/procedures/WirelessDisabled.cpp @@ -114,7 +114,7 @@ Result AuditWirelessDisabled(IndicatorsTree& indicators, ContextInterfac { return indicators.NonCompliant("Kernel module loaded '" + module + "'"); } - auto isModuleBlocked = IsKernelModuleBlocked(module, indicators, context); + auto isModuleBlocked = IsKernelModuleBlocked(module, true, indicators, context); if (!isModuleBlocked.HasValue() || isModuleBlocked.Value() == Status::NonCompliant) { return isModuleBlocked; diff --git a/src/modules/complianceengine/tests/procedures/KernelModuleTest.cpp b/src/modules/complianceengine/tests/procedures/KernelModuleTest.cpp index 1276d0494..42b918f7f 100644 --- a/src/modules/complianceengine/tests/procedures/KernelModuleTest.cpp +++ b/src/modules/complianceengine/tests/procedures/KernelModuleTest.cpp @@ -58,7 +58,8 @@ class EnsureKernelModuleTest : public ::testing::Test // TODO(kkanas) remove // Helper to create a fake /lib/modules tree -static std::string CreateModulesTree(MockContext& ctx, const std::vector& files) +static std::string CreateModulesTree(MockContext& ctx, const std::vector& files, const std::string& kernelVersion = "5.15.test", + const std::string& runningRelease = "5.15.test") { std::string root = ctx.GetTempdirPath() + "/modulesRoot"; if (::mkdir(root.c_str(), 0755) != 0) @@ -66,7 +67,7 @@ static std::string CreateModulesTree(MockContext& ctx, const std::vector(procModulesNegativeOutput))); + // Deny-listed but no install/mask line present. + EXPECT_CALL(mContext, ExecuteCommand(::testing::HasSubstr(modprobeCommand))).WillRepeatedly(::testing::Return(Result(modprobeBlocklistOutput))); + + KernelModuleUnavailableParams params; + params.moduleName = "usb-storage"; + + auto result = AuditKernelModuleUnavailable(params, indicators, mContext); + ASSERT_TRUE(result.HasValue()); + ASSERT_EQ(result.Value(), Status::Compliant); +}