Addressed review comments from @vpodzime and myself

Changelog: None
Ticket: CFE-3048
Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

cf-agent/agent-diagnostics.c

@@ -102,7 +102,7 @@ AgentDiagnosticsResult AgentDiagnosticsCheckPrivateKey(const char *workdir)
     {
         res = AgentDiagnosticsResultNew(false, StringFormat("No private key found at '%s'", path));
     }
-    else if (sb.st_mode != (S_IFREG | CF_PERMS_DEFAULT))
+    else if (sb.st_mode != (S_IFREG | S_IWUSR | S_IRUSR))
     {
         res = AgentDiagnosticsResultNew(false, StringFormat("Private key found at '%s', but had incorrect permissions '%o'", path, sb.st_mode));
     }
@@ -125,7 +125,7 @@ AgentDiagnosticsResult AgentDiagnosticsCheckPublicKey(const char *workdir)
     {
         res = AgentDiagnosticsResultNew(false, StringFormat("No public key found at '%s'", path));
     }
-    else if (sb.st_mode != (S_IFREG | CF_PERMS_DEFAULT))
+    else if (sb.st_mode != (S_IFREG | S_IWUSR | S_IRUSR))
     {
         res = AgentDiagnosticsResultNew(false, StringFormat("Public key found at '%s', but had incorrect permissions '%o'", path, sb.st_mode));
     }

cf-agent/files_operators.c

@@ -294,9 +294,8 @@ static bool SaveItemListCallback(const char *dest_filename, void *param, NewLine
     Item *liststart = param, *ip;
 
     //saving list to file
-    FILE *fp = safe_fopen_create_perms(dest_filename,
-                                       (new_line_mode == NewLineMode_Native) ? "wt" : "w",
-                                       CF_PERMS_DEFAULT);
+    FILE *fp = safe_fopen(
+        dest_filename, (new_line_mode == NewLineMode_Native) ? "wt" : "w");
     if (fp == NULL)
     {
         Log(LOG_LEVEL_ERR, "Unable to open destination file '%s' for writing. (fopen: %s)",

cf-agent/verify_files.c

@@ -570,9 +570,8 @@ static PromiseResult RenderTemplateCFEngine(EvalContext *ctx, const Promise *pp,
 
 static bool SaveBufferCallback(const char *dest_filename, void *param, NewLineMode new_line_mode)
 {
-    FILE *fp = safe_fopen_create_perms(dest_filename,
-                                       (new_line_mode == NewLineMode_Native) ? "wt" : "w",
-                                       CF_PERMS_DEFAULT);
+    FILE *fp = safe_fopen(
+        dest_filename, (new_line_mode == NewLineMode_Native) ? "wt" : "w");
     if (fp == NULL)
     {
         Log(LOG_LEVEL_ERR, "Unable to open destination file '%s' for writing. (fopen: %s)",

cf-agent/verify_files_utils.c

@@ -3914,8 +3914,7 @@ bool CfCreateFile(EvalContext *ctx, char *file, const Promise *pp, const Attribu
 
             MakeParentDirectory(file, attr->move_obstructions);
 
-            int fd = safe_open_create_perms(file, O_WRONLY | O_CREAT | O_EXCL,
-                                            filemode);
+            int fd = safe_open_create_perms(file, O_WRONLY | O_CREAT | O_EXCL, filemode);
             if (fd == -1)
             {
                 char errormsg[CF_BUFSIZE];

cf-key/cf-key-functions.c

@@ -232,8 +232,7 @@ int RemoveKeys(const char *input, bool must_be_coherent)
 
 static bool KeepKeyPromisesRSA(RSA *pair, const char *public_key_file, const char *private_key_file)
 {
-    FILE *fp = safe_fopen_create_perms(private_key_file, "w",
-                                       CF_PERMS_DEFAULT);
+    FILE *fp = safe_fopen_create_perms(private_key_file, "w", 0600);
     if (fp == NULL)
     {
         Log(LOG_LEVEL_ERR,
@@ -255,7 +254,7 @@ static bool KeepKeyPromisesRSA(RSA *pair, const char *public_key_file, const cha
         return false;
     }
 
-    fp = safe_fopen_create_perms(public_key_file, "w", CF_PERMS_DEFAULT);
+    fp = safe_fopen_create_perms(public_key_file, "w", 0600);
     if (fp == NULL)
     {
         Log(LOG_LEVEL_ERR,

cf-serverd/cf-serverd-functions.c

@@ -119,15 +119,13 @@ static const char *const HINTS[] =
 #ifdef HAVE_AVAHI_COMMON_ADDRESS_H
 static int GenerateAvahiConfig(const char *path)
 {
-    FILE *fout;
-    Writer *writer = NULL;
-    fout = safe_fopen_create_perms(path, "w+", CF_PERMS_DEFAULT);
+    FILE *fout = safe_fopen(path, "w+");
     if (fout == NULL)
     {
         Log(LOG_LEVEL_ERR, "Unable to open '%s'", path);
         return -1;
     }
-    writer = FileWriter(fout);
+    Writer *writer = FileWriter(fout);
     fprintf(fout, "<?xml version=\"1.0\" standalone='no'?>\n");
     fprintf(fout, "<!DOCTYPE service-group SYSTEM \"avahi-service.dtd\">\n");
     XmlComment(writer, "This file has been automatically generated by cf-serverd.");

libpromises/bootstrap.c

@@ -269,7 +269,7 @@ bool WriteBuiltinFailsafePolicyToPath(const char *filename)
 
     Log(LOG_LEVEL_INFO, "Writing built-in failsafe policy to '%s'", filename);
 
-    FILE *fout = safe_fopen_create_perms(filename, "w", CF_PERMS_DEFAULT);
+    FILE *fout = safe_fopen(filename, "w");
     if (!fout)
     {
         Log(LOG_LEVEL_ERR, "Unable to write failsafe to '%s' (fopen: %s)", filename, GetErrorStr());

libpromises/crypto.c

@@ -100,7 +100,7 @@ void CryptoDeInitialize()
             unlink(randfile); /* do not reuse entropy */
         }
 
-        chmod(randfile, CF_PERMS_DEFAULT);
+        chmod(randfile, 0600);
         EVP_cleanup();
         CleanupOpenSSLThreadLocks();
         ERR_free_strings();

libpromises/eval_context.c

@@ -2740,8 +2740,7 @@ static void SummarizeTransaction(EvalContext *ctx, const TransactionContext *tc,
                 }
             }
 
-            FILE *fout = safe_fopen_create_perms(logname, "a",
-                                                 CF_PERMS_DEFAULT);
+            FILE *fout = safe_fopen(logname, "a");
 
             if (fout == NULL)
             {

libpromises/item_lib.c

@@ -1020,9 +1020,8 @@ bool RawSaveItemList(const Item *liststart, const char *filename, NewLineMode ne
     strcat(new, CF_EDITED);
     unlink(new);                /* Just in case of races */
 
-    FILE *fp = safe_fopen_create_perms(new,
-                                       (new_line_mode == NewLineMode_Native) ? "wt" : "w",
-                                       CF_PERMS_DEFAULT);
+    FILE *fp = safe_fopen(
+        new, (new_line_mode == NewLineMode_Native) ? "wt" : "w");
     if (fp == NULL)
     {
         Log(LOG_LEVEL_ERR, "Couldn't write file '%s'. (fopen: %s)", new, GetErrorStr());

libutils/file_lib.c

@@ -478,7 +478,7 @@ void RestoreUmask(mode_t old_mask)
 }
 
 /**
- * Opens a file safety, with default (strict) permissions on creation.
+ * Opens a file safely, with default (strict) permissions on creation.
  * See safe_open_create_perms for more documentation.
  *
  * @param pathname The path to open.
@@ -491,28 +491,27 @@ int safe_open(const char *pathname, int flags)
 }
 
 /**
- * Opens a file safely. It will follow symlinks, but only if the symlink is trusted,
- * that is, if the owner of the symlink and the owner of the target are the same,
- * or if the owner of the symlink is either root or the user running the current process.
- * All components are checked, even symlinks encountered in earlier parts of the
- * path name.
+ * Opens a file safely. It will follow symlinks, but only if the symlink is
+ * trusted, that is, if the owner of the symlink and the owner of the target are
+ * the same, or if the owner of the symlink is either root or the user running
+ * the current process. All components are checked, even symlinks encountered in
+ * earlier parts of the path name.
  *
- * It should always be used when opening a file or directory that is not guaranteed
- * to be owned by root.
+ * It should always be used when opening a file or directory that is not 
+ * guaranteed to be owned by root.
  *
- * We decided to go for two different functions to decrease ambiguity and
- * increase security. Previously there was only `safe_open`, which had an
- * optional parameter in the same way as `open`.
+ * safe_open and safe_fopen both default to secure (0600) file creation perms.
+ * The _create_perms variants allow you to explicitly set different permissions.
  *
- * @param pathname The path to open.
- * @param flags Same flags as for system open().
- * @param perms Permissions for file, only relevant on file creation.
- * @return Same errors as open().
+ * @param pathname The path to open
+ * @param flags Same flags as for system open()
+ * @param create_perms Permissions for file, only relevant on file creation
+ * @return Same errors as open()
+ * @see safe_fopen_create_perms()
+ * @see safe_open()
  */
 int safe_open_create_perms(
-        const char *const pathname,
-        int flags,
-        const mode_t perms)
+    const char *const pathname, int flags, const mode_t create_perms)
 {
     if (flags & O_TRUNC)
     {
@@ -534,7 +533,7 @@ int safe_open_create_perms(
 
 #ifdef __MINGW32__
     // Windows gets off easy. No symlinks there.
-    return open(pathname, flags, perms);
+    return open(pathname, flags, create_perms);
 #else // !__MINGW32__
 
     const size_t path_bufsize = strlen(pathname) + 1;
@@ -674,7 +673,7 @@ int safe_open_create_perms(
                 {
                     *restore_slash = '/';
                 }
-                int filefd = openat(currentfd, component, flags, perms);
+                int filefd = openat(currentfd, component, flags, create_perms);
                 if (filefd < 0)
                 {
                     if ((stat_before_result < 0  && !(orig_flags & O_EXCL)  && errno == EEXIST) ||
@@ -792,13 +791,11 @@ FILE *safe_fopen(const char *const path, const char *const mode)
  *
  * @param pathname The path to open.
  * @param flags Same mode as for system fopen().
- * @param ... Optional permissions argument, same as in safe_open()
+ * @param create_perms Permissions for file, only relevant on file creation.
  * @return Same errors as fopen().
  */
 FILE *safe_fopen_create_perms(
-        const char *const path,
-        const char *const mode,
-        const mode_t perms)
+    const char *const path, const char *const mode, const mode_t create_perms)
 {
     if (!path || !mode)
     {
@@ -830,15 +827,13 @@ FILE *safe_fopen_create_perms(
         case 't':
             flags |= O_TEXT;
             break;
-        case '\0':
-            break;
         default:
             ProgrammingError("Invalid flag for fopen: %s", mode);
             return NULL;
         }
     }
 
-    int fd = safe_open_create_perms(path, flags, perms);
+    int fd = safe_open_create_perms(path, flags, create_perms);
     if (fd < 0)
     {
         return NULL;

libutils/file_lib.h

@@ -106,11 +106,12 @@ mode_t SetUmask(mode_t new_mask);
 void RestoreUmask(mode_t old_mask);
 
 int safe_open(const char *const pathname, int flags);
-int safe_open_create_perms(const char *const pathname,
-                           int flags, const mode_t perms);
-FILE *safe_fopen(const char *const path, const char *const mode);
-FILE *safe_fopen_create_perms(const char *const path, const char *const mode,
-                              const mode_t perms);
+int safe_open_create_perms(
+    const char *pathname, int flags, mode_t create_perms);
+
+FILE *safe_fopen(const char *path, const char *mode);
+FILE *safe_fopen_create_perms(
+    const char *path, const char *mode, mode_t create_perms);
 
 int safe_chdir(const char *path);
 int safe_chown(const char *path, uid_t owner, gid_t group);

tests/unit/file_lib_test.c

@@ -511,8 +511,8 @@ static void test_safe_open_create_safe_inserted_symlink(void)
     // safe_open() instead.
     //switch_symlink_hook();
 
-    int fd = safe_open_create_perms(TEST_LINK, O_RDONLY | O_CREAT,
-                                    CF_PERMS_SHARED);
+    int fd = safe_open_create_perms(
+        TEST_LINK, (O_RDONLY | O_CREAT), CF_PERMS_SHARED);
     assert_true(fd >= 0);
     check_contents(fd, TEST_STRING);
     close(fd);