From 627035fb22ac375d19cdde3b132f2d7fa85fcbe7 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Tue, 23 Oct 2012 21:47:56 -0400 Subject: [PATCH] Patch from Franck Joncourt for setting permissions via open() [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 | 5 +++++ ChangeLog | 5 +++++ client/config_init.c | 23 +++++++++++++++++++---- client/fwknop.c | 36 +++++++++++++++++++++--------------- client/utils.c | 18 ------------------ client/utils.h | 1 - server/replay_cache.c | 28 ++++++++++++++++++++-------- server/utils.c | 15 --------------- server/utils.h | 1 - 9 files changed, 70 insertions(+), 62 deletions(-) diff --git a/CREDITS b/CREDITS index 2de7281c..1a37ae97 100644 --- 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 diff --git a/ChangeLog b/ChangeLog index b7762d6e..87efa35c 100644 --- 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 diff --git a/client/config_init.c b/client/config_init.c index 5f975602..49503206 100644 --- a/client/config_init.c +++ b/client/config_init.c @@ -33,6 +33,8 @@ #include "config_init.h" #include "cmd_opts.h" #include "utils.h" +#include +#include /* 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); } diff --git a/client/fwknop.c b/client/fwknop.c index 1beb66ee..fbe1e4a9 100644 --- a/client/fwknop.c +++ b/client/fwknop.c @@ -33,6 +33,8 @@ #include "spa_comm.h" #include "utils.h" #include "getpasswd.h" +#include +#include /* 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, 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"); + } + close(args_file_fd); } - fprintf(args_file_ptr, "%s\n", args_str); - fclose(args_file_ptr); } - - set_file_perms(args_save_file); - return; } diff --git a/client/utils.c b/client/utils.c index e40115ba..d5b91807 100644 --- a/client/utils.c +++ b/client/utils.c @@ -68,24 +68,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) { diff --git a/client/utils.h b/client/utils.h index 672b8f35..c951314e 100644 --- a/client/utils.h +++ b/client/utils.h @@ -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); diff --git a/server/replay_cache.c b/server/replay_cache.c index 28a31f5f..0a141bcb 100644 --- a/server/replay_cache.c +++ b/server/replay_cache.c @@ -37,6 +37,8 @@ #include "fwknopd_errors.h" #include "utils.h" +#include +#include #include #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[] = "#