Patch from Franck Joncourt for setting permissions via open()
authorMichael Rash <mbr@cipherdyne.org>
Wed, 24 Oct 2012 01:47:56 +0000 (21:47 -0400)
committerMichael Rash <mbr@cipherdyne.org>
Wed, 24 Oct 2012 01:47:56 +0000 (21:47 -0400)
[client+server] Applied patch from Franck Joncourt to remove unnecessary
chmod() call when creating client rc file and server replay cache file.
The permissions are now set appropriately via open(), and at the same
time this patch fixes a potential race condition since the previous code
used fopen() followed by chmod().

CREDITS
ChangeLog
client/config_init.c
client/fwknop.c
client/utils.c
client/utils.h
server/replay_cache.c
server/utils.c
server/utils.h

diff --git a/CREDITS b/CREDITS
index 2de7281..1a37ae9 100644 (file)
--- a/CREDITS
+++ b/CREDITS
@@ -36,6 +36,11 @@ Franck Joncourt
       the local directory (if it exists) so that it doesn't have to have libfko
       completely installed in /usr/lib/.  This allows the test suite to run FKO
       tests without installing libfko.
+    - Contributed a patch to remove unnecessary chmod() call when creating
+      client rc file and server replay cache file.  The permissions are now set
+      appropriately via open(), and at the same time this patch fixes a
+      potential race condition since the previous code used fopen() followed by
+      chmod().
 
 Jonathan Schulz
     - Submitted patches to change HTTP connection type to 'close' for -R mode
index b7762d6..87efa35 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -21,6 +21,11 @@ fwknop-2.0.4 (09/20/2012):
       modes.
     - [libfko] Restricted usernames embedded in SPA packets to be
       alpha-numeric along with "-" chars.
+    - [client+server] Applied patch from Franck Joncourt to remove unnecessary
+      chmod() call when creating client rc file and server replay cache file.
+      The permissions are now set appropriately via open(), and at the same
+      time this patch fixes a potential race condition since the previous code
+      used fopen() followed by chmod().
     - [server] Bug fix to accept SPA packets over ICMP if the fwknop client
       is executed with '-P icmp' and the user has the required privileges.
     - [test suite] Applied patch from Franck Joncourt to have the perl FKO
index 5f97560..4950320 100644 (file)
@@ -33,6 +33,8 @@
 #include "config_init.h"
 #include "cmd_opts.h"
 #include "utils.h"
+#include <sys/stat.h>
+#include <fcntl.h>
 
 /* Convert a digest_type string to its integer value.
 */
@@ -132,13 +134,28 @@ parse_time_offset(const char *offset_str)
 static int
 create_fwknoprc(const char *rcfile)
 {
-    FILE    *rc = NULL;
+    FILE *rc = NULL;
+    int   rcfile_fd = -1;
 
     fprintf(stdout, "[*] Creating initial rc file: %s.\n", rcfile);
 
+    /* Try to create the initial rcfile with user read/write rights only.
+     * If the rcfile already exists, an error is returned */
+    rcfile_fd = open(rcfile, O_WRONLY|O_CREAT|O_EXCL , S_IRUSR|S_IWUSR);
+
+    // If an error occured ...
+    if (rcfile_fd == -1) {
+            fprintf(stderr, "Unable to create initial rc file: %s: %s\n",
+                rcfile, strerror(errno));
+            return(-1);
+    }
+
+    // Free the rcfile descriptor
+    close(rcfile_fd);
+
     if ((rc = fopen(rcfile, "w")) == NULL)
     {
-        fprintf(stderr, "Unable to create rc file: %s: %s\n",
+        fprintf(stderr, "Unable to write default setup to rcfile: %s: %s\n",
             rcfile, strerror(errno));
         return(-1);
     }
@@ -218,8 +235,6 @@ create_fwknoprc(const char *rcfile)
 
     fclose(rc);
 
-    set_file_perms(rcfile);
-
     return(0);
 }
 
index 1beb66e..fbe1e4a 100644 (file)
@@ -33,6 +33,8 @@
 #include "spa_comm.h"
 #include "utils.h"
 #include "getpasswd.h"
+#include <sys/stat.h>
+#include <fcntl.h>
 
 /* prototypes
 */
@@ -660,8 +662,7 @@ save_args(int argc, char **argv)
 {
     char args_save_file[MAX_PATH_LEN];
     char args_str[MAX_LINE_LEN] = "";
-    FILE *args_file_ptr = NULL;
-    int i = 0, args_str_len = 0;
+    int i = 0, args_str_len = 0, args_file_fd = -1;
 
 #ifdef WIN32
     /* Not sure what the right thing is here on Win32, just return
@@ -671,26 +672,31 @@ save_args(int argc, char **argv)
 #endif
 
     if (get_save_file(args_save_file)) {
-        if ((args_file_ptr = fopen(args_save_file, "w")) == NULL) {
+        args_file_fd = open(args_save_file, O_WRONLY|O_CREAT, S_IRUSR|S_IWUSR);
+        if (args_file_fd == -1) {
             fprintf(stderr, "Could not open args file: %s\n",
                 args_save_file);
             exit(EXIT_FAILURE);
         }
-        for (i=0; i < argc; i++) {
-            args_str_len += strlen(argv[i]);
-            if (args_str_len >= MAX_PATH_LEN) {
-                fprintf(stderr, "argument string too long, exiting.\n");
-                exit(EXIT_FAILURE);
+        else {
+            for (i=0; i < argc; i++) {
+                args_str_len += strlen(argv[i]);
+                if (args_str_len >= MAX_PATH_LEN) {
+                    fprintf(stderr, "argument string too long, exiting.\n");
+                    exit(EXIT_FAILURE);
+                }
+                strlcat(args_str, argv[i], MAX_PATH_LEN);
+                strlcat(args_str, " ", MAX_PATH_LEN);
+            }
+            strlcat(args_str, "\n", MAX_PATH_LEN);
+            if(write(args_file_fd, args_str, strlen(args_str))
+                    != strlen(args_str)) {
+                fprintf(stderr,
+                "warning, did not write expected number of bytes to args save file\n");
             }
-            strlcat(args_str, argv[i], MAX_PATH_LEN);
-            strlcat(args_str, " ", MAX_PATH_LEN);
+            close(args_file_fd);
         }
-        fprintf(args_file_ptr, "%s\n", args_str);
-        fclose(args_file_ptr);
     }
-
-    set_file_perms(args_save_file);
-
     return;
 }
 
index e40115b..d5b9180 100644 (file)
@@ -69,24 +69,6 @@ hex_dump(const unsigned char *data, const int size)
 }
 
 int
-set_file_perms(const char *file)
-{
-    int res = 0;
-
-    res = chmod(file, S_IRUSR | S_IWUSR);
-
-    if(res != 0)
-    {
-        fprintf(stderr,
-            "[-] unable to chmod file %s to user read/write (0600, -rw-------): %s\n",
-            file,
-            strerror(errno)
-        );
-    }
-    return res;
-}
-
-int
 verify_file_perms_ownership(const char *file)
 {
     int res = 1;
index 672b8f3..c951314 100644 (file)
@@ -45,7 +45,6 @@
 /* Prototypes
 */
 void hex_dump(const unsigned char *data, const int size);
-int set_file_perms(const char *file);
 int verify_file_perms_ownership(const char *file);
 
 size_t strlcat(char *dst, const char *src, size_t siz);
index 28a31f5..0a141bc 100644 (file)
@@ -37,6 +37,8 @@
 #include "fwknopd_errors.h"
 #include "utils.h"
 
+#include <sys/stat.h>
+#include <fcntl.h>
 #include <time.h>
 
 #if HAVE_LIBGDBM
@@ -230,6 +232,8 @@ replay_file_cache_init(fko_srv_options_t *opts)
     char            src_ip[INET_ADDRSTRLEN+1] = {0};
     char            dst_ip[INET_ADDRSTRLEN+1] = {0};
     long int        time_tmp;
+    int             digest_file_fd = -1;
+    char            digest_header[] = "# <digest> <proto> <src_ip> <src_port> <dst_ip> <dst_port> <time>\n";
 
     struct digest_cache_list *digest_elm = NULL;
 
@@ -252,17 +256,25 @@ replay_file_cache_init(fko_srv_options_t *opts)
         /* the file does not exist yet, so it will be created when the first
          * successful SPA packet digest is written to disk
         */
-        if ((digest_file_ptr = fopen(opts->config[CONF_DIGEST_FILE], "w")) == NULL)
+        digest_file_fd = open(opts->config[CONF_DIGEST_FILE], O_WRONLY|O_CREAT|O_EXCL, S_IRUSR|S_IWUSR);
+        if (digest_file_fd == -1)
+        {
+            log_msg(LOG_WARNING, "Could not create digest cache: %s: %s",
+                opts->config[CONF_DIGEST_FILE], strerror(errno));
+            return(-1);
+        }
+        else
         {
-            log_msg(LOG_WARNING, "Could not open digest cache: %s",
-                opts->config[CONF_DIGEST_FILE]);
+            if(write(digest_file_fd, digest_header, strlen(digest_header))
+                    != strlen(digest_header)) {
+                log_msg(LOG_WARNING,
+                    "Did not write expected number of bytes to digest cache: %s\n",
+                    opts->config[CONF_DIGEST_FILE]);
+            }
+            close(digest_file_fd);
+
+            return(0);
         }
-        fprintf(digest_file_ptr,
-            "# <digest> <proto> <src_ip> <src_port> <dst_ip> <dst_port> <time>\n");
-        fclose(digest_file_ptr);
-
-        set_file_perms(opts->config[CONF_DIGEST_FILE]);
-        return(0);
     }
 
     verify_file_perms_ownership(opts->config[CONF_DIGEST_FILE]);
index aa24940..5b050f6 100644 (file)
@@ -167,21 +167,6 @@ is_valid_dir(const char *path)
 }
 
 int
-set_file_perms(const char *file)
-{
-    int res = 0;
-
-    res = chmod(file, S_IRUSR | S_IWUSR);
-
-    if(res != 0)
-    {
-        fprintf(stderr, "[-] unable to chmod file %s to user read/write: %s\n",
-            file, strerror(errno));
-    }
-    return res;
-}
-
-int
 verify_file_perms_ownership(const char *file)
 {
     int res = 1;
index 03ad62e..5e76eb5 100644 (file)
@@ -62,7 +62,6 @@ void hex_dump(const unsigned char *data, const int size);
 char* dump_ctx(fko_ctx_t ctx);
 int is_base64(const unsigned char *buf, unsigned short int len);
 int is_valid_dir(const char *path);
-int set_file_perms(const char *file);
 int verify_file_perms_ownership(const char *file);
 
 size_t strlcat(char *dst, const char *src, size_t siz);