EnsureFilePermissions compliance check module tests

Author: robertwoj-microsoft

.github/workflows/module-test-run.yml

@@ -50,9 +50,12 @@ jobs:
         working-directory: ${{ steps.download.outputs.download-path }}/modules/test
         run: |
           sudo chmod +x ./moduletest
+          sudo cat /etc/passwd
+          sudo cat /etc/sudoers
+          id
 
           result=0
-          recipes=$(ls -d ../../src/modules/test/recipes/*.json)
+          recipes=$(find ../../src/modules/test/recipes/ -name "*.json")
 
           for recipe in $recipes; do
             if [ ! -f ../../src/tests/e2e-test-recipes/$(basename $recipe) ]; then
@@ -68,6 +71,9 @@ jobs:
               echo failed
               result=1
               echo "::warning file=$name.log::Error(s) in module-test for '$name'"
+              cat /etc/passwd
+              cat /etc/sudoers
+              id
             fi
 
             echo "$output"

src/common/commonutils/FileUtils.c

@@ -1023,6 +1023,8 @@ int ReplaceMarkedLinesInFile(const char* fileName, const char* marker, const cha
                                 status = (0 == errno) ? EPERM : errno;
                                 OsConfigLogInfo(log, "ReplaceMarkedLinesInFile: cannot write to temporary file '%s' (%d)", tempFileName, status);
                             }
+                        } else {
+                            OsConfigLogInfo(log, "Skipping line: %s", line);
                         }
 
                         memset(line, 0, lineMax + 1);
@@ -1074,6 +1076,7 @@ int ReplaceMarkedLinesInFile(const char* fileName, const char* marker, const cha
 
     if (0 == status)
     {
+        OsConfigLogInfo(log, "ReplaceMarkedLinesInFile: replacing '%s' with '%s' in '%s'", marker, newline, fileName);
         if (preserveAccess)
         {
             if (0 != (status = RenameFileWithOwnerAndAccess(tempFileName, fileName, log)))

src/common/commonutils/UserUtils.c

@@ -121,6 +121,7 @@ static int CopyUserEntry(SimplifiedUser* destination, struct passwd* source, OsC
 
     if (0 != status)
     {
+        OsConfigLogError(log, "CopyUserEntry: failed to copy user entry for '%s' (uid: %u, gid: %u)", source->pw_name, source->pw_uid, source->pw_gid);
         ResetUserEntry(destination);
     }
 
@@ -358,6 +359,7 @@ int EnumerateUsers(SimplifiedUser** userList, unsigned int* size, char** reason,
 
     if (0 != (*size = GetNumberOfLinesInFile(passwdFile)))
     {
+        OsConfigLogInfo(log, "EnumerateUsers: %u lines in '%s'", *size, passwdFile);
         listSize = (*size) * sizeof(SimplifiedUser);
         if (NULL != (*userList = malloc(listSize)))
         {
@@ -367,6 +369,7 @@ int EnumerateUsers(SimplifiedUser** userList, unsigned int* size, char** reason,
 
             while ((NULL != (userEntry = getpwent())) && (i < *size))
             {
+                OsConfigLogInfo(log, "EnumerateUsers: user %u: '%s' (%u, %u) in '%s'", i, userEntry->pw_name, userEntry->pw_uid, userEntry->pw_gid, passwdFile);
                 if (0 != (status = CopyUserEntry(&((*userList)[i]), userEntry, log)))
                 {
                     OsConfigLogError(log, "EnumerateUsers: failed making copy of user entry (%d)", status);
@@ -833,6 +836,8 @@ int RemoveUser(SimplifiedUser* user, bool removeHome, OsConfigLogHandle log)
         return EPERM;
     }
 
+    OsConfigLogError(log, "RemoveUser: attempting to delete user '%s' (%u, %u) with home directory '%s'",
+        user->username, user->userId, user->groupId, user->home);
     if (NULL != (command = FormatAllocateString(commandTemplate, removeHome ? "-f -r" : "-f", user->username)))
     {
         if (0 == (status = ExecuteCommand(NULL, command, false, false, 0, 0, NULL, NULL, log)))

src/modules/compliance/src/lib/Base64.cpp

@@ -6,7 +6,6 @@
 #include "Result.h"
 
 #include <string>
-
 namespace compliance
 {
 static inline char Base64Char(const unsigned char c)

src/modules/compliance/src/lib/ComplianceInterface.cpp

@@ -142,14 +142,25 @@ int ComplianceMmiSet(MMI_HANDLE clientSession, const char* componentName, const
 
     try
     {
+        OsConfigLogInfo(engine.log(), "MmiSet(%p, %s, %s, %.*s, %d)", clientSession, componentName, objectName, payloadSizeBytes, payload, payloadSizeBytes);
         std::string payloadStr(payload, payloadSizeBytes);
         auto object = parseJSON(payloadStr.c_str());
-        if (NULL == object || JSONString != json_value_get_type(object.get()))
+        if (NULL == object || (JSONString != json_value_get_type(object.get()) && JSONObject != json_value_get_type(object.get())))
         {
             OsConfigLogError(engine.log(), "ComplianceMmiSet failed: Failed to parse JSON string");
             return EINVAL;
         }
-        std::string realPayload = json_value_get_string(object.get());
+        std::string realPayload;
+        if (json_value_get_type(object.get()) == JSONString)
+        {
+            realPayload = json_value_get_string(object.get());
+        }
+        else if (json_value_get_type(object.get()) == JSONObject)
+        {
+            char* tmp = json_serialize_to_string(object.get());
+            realPayload = tmp;
+            // json_free_serialized_string(tmp);
+        }
         auto result = engine.mmiSet(objectName, realPayload);
         if (!result.has_value())
         {

src/modules/compliance/src/lib/Engine.cpp

@@ -119,7 +119,13 @@ Optional<Error> Engine::setProcedure(const std::string& ruleName, const std::str
     auto ruleJSON = decodeB64JSON(payload);
     if (!ruleJSON.has_value())
     {
-        return ruleJSON.error();
+        // Fall back to plain JSON, both formats are supported
+        ruleJSON = compliance::parseJSON(payload.c_str());
+        if (!ruleJSON.has_value())
+        {
+            OsConfigLogError(log(), "Failed to parse JSON: %s", ruleJSON.error().message.c_str());
+            return ruleJSON.error();
+        }
     }
 
     auto object = json_value_get_object(ruleJSON.value().get());

src/modules/compliance/src/lib/Evaluator.cpp

@@ -45,6 +45,8 @@ Result<Status> Evaluator::ExecuteRemediation()
         return result.error();
     }
 
+    OsConfigLogInfo(mLog, "Remediation result: %s", result.value() == Status::Compliant ? "Compliant" : "Non-compliant");
+    OsConfigLogInfo(mLog, "Remediation log: %s", mLogstream.str().c_str());
     return result;
 }
 

src/modules/compliance/src/lib/procedures/ensureFilePermissions.cpp

@@ -11,6 +11,10 @@
 
 namespace compliance
 {
+namespace
+{
+const mode_t supportedMask = 0x1FF;
+} // anonymous namespace
 
 AUDIT_FN(ensureFilePermissions)
 {
@@ -19,6 +23,11 @@ AUDIT_FN(ensureFilePermissions)
     {
         return Error("No filename provided");
     }
+    if (args.find("permissions") != args.end() && args.find("mask") != args.end())
+    {
+        return Error("Cannot specify both permissions and mask");
+    }
+
     logstream << "ensureFilePermissions for '" << args["filename"] << "' ";
 
     if (0 != stat(args["filename"].c_str(), &statbuf))
@@ -37,7 +46,7 @@ AUDIT_FN(ensureFilePermissions)
         }
         if (args["user"] != pwd->pw_name)
         {
-            logstream << "Invalid user - is " << pwd->pw_name << " should be " << args["user"];
+            logstream << "Invalid user - is '" << pwd->pw_name << "' should be '" << args["user"] << "'";
             return false;
         }
     }
@@ -63,42 +72,42 @@ AUDIT_FN(ensureFilePermissions)
         }
         if (!groupOk)
         {
-            logstream << "Invalid group - is '" << grp->gr_name << "' should be '" << args["group"] << "' ";
+            logstream << "Invalid group - is '" << grp->gr_name << "' should be '" << args["group"] << "'";
             return false;
         }
     }
 
-    unsigned short perms = 0xFFF;
-    unsigned short mask = 0xFFF;
-    bool has_perms_or_mask = false;
+    const mode_t supportedMask = 0x1FF;
     if (args.find("permissions") != args.end())
     {
         char* endptr;
-        perms = strtol(args["permissions"].c_str(), &endptr, 8);
-        if ('\0' != *endptr)
+        mode_t perms = strtol(args["permissions"].c_str(), &endptr, 8);
+        if (('\0' != *endptr) || ((perms & supportedMask) != perms))
+        {
+            return Error("Invalid permissions parameter");
+        }
+        if (perms != (statbuf.st_mode & supportedMask))
         {
-            logstream << "Invalid permissions: " << args["permissions"];
+            logstream << "Invalid permissions - are " << std::oct << (statbuf.st_mode & supportedMask) << " should be " << std::oct << perms << std::dec;
             return false;
         }
-        has_perms_or_mask = true;
     }
     if (args.find("mask") != args.end())
     {
         char* endptr;
-        mask = strtol(args["mask"].c_str(), &endptr, 8);
-        if ('\0' != *endptr)
+        mode_t mask = strtol(args["mask"].c_str(), &endptr, 8);
+        if (('\0' != *endptr) || ((mask & supportedMask) != mask))
         {
-            logstream << "Invalid permissions mask: " << args["mask"];
+            return Error("Invalid mask parameter");
+        }
+        if (((statbuf.st_mode & supportedMask) & mask) != 0)
+        {
+            logstream << "Invalid permissions - are " << std::oct << (statbuf.st_mode & supportedMask) << " should be " << std::oct
+                      << ((statbuf.st_mode & supportedMask) & (~mask)) << " with mask " << std::oct << mask << std::dec;
             return false;
         }
-        has_perms_or_mask = true;
-    }
-    if (has_perms_or_mask && ((perms & mask) != (statbuf.st_mode & mask)))
-    {
-        logstream << "Invalid permissions - are " << std::oct << statbuf.st_mode << " should be " << std::oct << perms << " with mask " << std::oct
-                  << mask << std::dec;
-        return false;
     }
+
     return true;
 }
 
@@ -109,6 +118,10 @@ REMEDIATE_FN(ensureFilePermissions)
         logstream << "ERROR: No filename provided";
         return Error("No filename provided");
     }
+    if (args.find("permissions") != args.end() && args.find("mask") != args.end())
+    {
+        return Error("Cannot specify both permissions and mask");
+    }
 
     struct stat statbuf;
     if (stat(args["filename"].c_str(), &statbuf) < 0)
@@ -126,7 +139,7 @@ REMEDIATE_FN(ensureFilePermissions)
         if (pwd == nullptr)
         {
             logstream << "ERROR: No user with name " << args["user"];
-            return Error("No user with name " + args["user"]);
+            return false;
         }
         uid = pwd->pw_uid;
         owner_changed = true;
@@ -161,7 +174,7 @@ REMEDIATE_FN(ensureFilePermissions)
             if (grp == nullptr)
             {
                 logstream << "ERROR: No group with name " << args["group"];
-                return Error("No group with name " + args["group"]);
+                return false;
             }
             gid = grp->gr_gid;
             owner_changed = true;
@@ -172,42 +185,44 @@ REMEDIATE_FN(ensureFilePermissions)
         if (0 != chown(args["filename"].c_str(), uid, gid))
         {
             logstream << "ERROR: Chown error " << strerror(errno);
-            return Error("Chown error");
+            return Error("Chown error", errno);
         }
     }
 
-    unsigned short perms = 0xFFF;
-    unsigned short mask = 0xFFF;
-    bool has_perms_or_mask = false;
     if (args.find("permissions") != args.end())
     {
         char* endptr;
-        perms = strtol(args["permissions"].c_str(), &endptr, 8);
-        if ('\0' != *endptr)
+        const mode_t perms = strtol(args["permissions"].c_str(), &endptr, 8);
+        if (('\0' != *endptr) || ((perms & supportedMask) != perms))
         {
             logstream << "ERROR: Invalid permissions: " << args["permissions"];
             return Error("Invalid permissions: " + args["permissions"]);
         }
-        has_perms_or_mask = true;
+        if (perms != (statbuf.st_mode & supportedMask))
+        {
+            if (chmod(args["filename"].c_str(), perms) < 0)
+            {
+                logstream << "ERROR: Chmod error " << strerror(errno);
+                return false;
+            }
+        }
     }
     if (args.find("mask") != args.end())
     {
         char* endptr;
-        mask = strtol(args["mask"].c_str(), &endptr, 8);
-        if ('\0' != *endptr)
+        const mode_t mask = strtol(args["mask"].c_str(), &endptr, 8);
+        if (('\0' != *endptr) || ((mask & supportedMask) != mask))
         {
             logstream << "ERROR: Invalid permissions mask: " << args["mask"];
             return Error("Invalid permissions mask: " + args["mask"]);
         }
-        has_perms_or_mask = true;
-    }
-    unsigned short new_perms = (statbuf.st_mode & ~mask) | (perms & mask);
-    if (has_perms_or_mask && (new_perms != statbuf.st_mode))
-    {
-        if (chmod(args["filename"].c_str(), new_perms) < 0)
+        if (((statbuf.st_mode & supportedMask) & mask) != 0)
         {
-            logstream << "ERROR: Chmod error";
-            return Error("Chmod error");
+            if (chmod(args["filename"].c_str(), statbuf.st_mode & (~mask)) < 0)
+            {
+                logstream << "ERROR: Chmod error " << strerror(errno);
+                return false;
+            }
         }
     }