From a60f05ad44e824f6230b22f8976399340cb535dc Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Wed, 29 Aug 2012 22:21:43 -0400 Subject: [PATCH] file permissions and client buffer overflow fix - [client+server] Fernando Arnaboldi from IOActive found that strict filesystem permissions for various fwknop files are not verified. Added warnings whenever permissions are not strict enough, and ensured that files created by the fwknop client and server are only set to user read/write. - [client] Fernando Arnaboldi from IOActive found a local buffer overflow in --last processing with a maliciously constructed ~/.fwknop.run file. This has been fixed with proper validation of .fwknop.run arguments. --- ChangeLog | 8 +++++ client/config_init.c | 15 ++++++++-- client/fwknop.c | 19 +++++++++++-- client/utils.c | 66 +++++++++++++++++++++++++++++++++++++++++++- client/utils.h | 13 +++++++++ configure.ac | 2 +- server/access.c | 2 + server/config_init.c | 2 + server/fwknopd.c | 2 + server/replay_cache.c | 6 +++- server/utils.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++- server/utils.h | 2 + test/test-fwknop.pl | 21 +++++++++++++- 13 files changed, 217 insertions(+), 13 deletions(-) diff --git a/ChangeLog b/ChangeLog index 056d1ef..5e800b0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -13,6 +13,14 @@ fwknop-2.0.3 (08//2012): the server did not properly validate allow IP addresses from malicious authenticated clients. This has been fixed with stronger allow IP validation. + - [client+server] Fernando Arnaboldi from IOActive found that strict + filesystem permissions for various fwknop files are not verified. Added + warnings whenever permissions are not strict enough, and ensured that + files created by the fwknop client and server are only set to user + read/write. + - [client] Fernando Arnaboldi from IOActive found a local buffer overflow + in --last processing with a maliciously constructed ~/.fwknop.run file. + This has been fixed with proper validation of .fwknop.run arguments. - [test suite] Added a new fuzzing capability to ensure proper server-side input validation. Fuzzing data is constructed with modified fwknop client code that is designed to emulate malicious behavior. diff --git a/client/config_init.c b/client/config_init.c index 19ec13e..9e04bcc 100644 --- a/client/config_init.c +++ b/client/config_init.c @@ -124,9 +124,9 @@ parse_time_offset(const char *offset_str) static int create_fwknoprc(const char *rcfile) { - FILE *rc; + FILE *rc = NULL; - fprintf(stderr, "Creating initial rc file: %s.\n", rcfile); + fprintf(stdout, "[*] Creating initial rc file: %s.\n", rcfile); if ((rc = fopen(rcfile, "w")) == NULL) { @@ -188,7 +188,7 @@ create_fwknoprc(const char *rcfile) "# User-provided named stanzas:\n" "\n" "# Example for a destination server of 192.168.1.20 to open access to \n" - "# SSH for an IP that is resoved exteranlly, and one with a NAT request\n" + "# SSH for an IP that is resolved externally, and one with a NAT request\n" "# for a specific source IP that maps port 8088 on the server\n" "# to port 88 on 192.168.1.55 with timeout.\n" "#\n" @@ -210,6 +210,8 @@ create_fwknoprc(const char *rcfile) fclose(rc); + set_file_perms(rcfile); + return(0); } @@ -440,6 +442,13 @@ process_rc(fko_cli_options_t *options) rcfile[rcf_offset] = PATH_SEP; strlcat(rcfile, ".fwknoprc", MAX_PATH_LEN); + /* Check rc file permissions - if anything other than user read/write, + * then don't process it. This change was made to help ensure that the + * client consumes a proper rc file with strict permissions set (thanks + * to Fernando Arnaboldi from IOActive for pointing this out). + */ + verify_file_perms_ownership(rcfile); + /* Open the rc file for reading, if it does not exist, then create * an initial .fwknoprc file with defaults and go on. */ diff --git a/client/fwknop.c b/client/fwknop.c index 97192b6..3c518da 100644 --- a/client/fwknop.c +++ b/client/fwknop.c @@ -48,6 +48,8 @@ static int set_nat_access(fko_ctx_t ctx, fko_cli_options_t *options); static int get_rand_port(fko_ctx_t ctx); int resolve_ip_http(fko_cli_options_t *options); +#define MAX_CMDLINE_ARGS 50 /* should be way more than enough */ + int main(int argc, char **argv) { @@ -556,6 +558,8 @@ show_last_command(void) exit(EXIT_FAILURE); #endif + verify_file_perms_ownership(args_save_file); + if (get_save_file(args_save_file)) { if ((args_file_ptr = fopen(args_save_file, "r")) == NULL) { fprintf(stderr, "Could not open args file: %s\n", @@ -587,7 +591,7 @@ run_last_args(fko_cli_options_t *options) char args_save_file[MAX_PATH_LEN] = {0}; char args_str[MAX_LINE_LEN] = {0}; char arg_tmp[MAX_LINE_LEN] = {0}; - char *argv_new[200]; /* should be way more than enough */ + char *argv_new[MAX_CMDLINE_ARGS]; /* should be way more than enough */ #ifdef WIN32 @@ -599,6 +603,8 @@ run_last_args(fko_cli_options_t *options) if (get_save_file(args_save_file)) { + verify_file_perms_ownership(args_save_file); + if ((args_file_ptr = fopen(args_save_file, "r")) == NULL) { fprintf(stderr, "Could not open args file: %s\n", @@ -623,12 +629,17 @@ run_last_args(fko_cli_options_t *options) argv_new[argc_new] = malloc(strlen(arg_tmp)+1); if (argv_new[argc_new] == NULL) { - fprintf(stderr, "malloc failure for cmd line arg.\n"); + fprintf(stderr, "[*] malloc failure for cmd line arg.\n"); exit(EXIT_FAILURE); } strlcpy(argv_new[argc_new], arg_tmp, strlen(arg_tmp)+1); current_arg_ctr = 0; argc_new++; + if(argc_new >= MAX_CMDLINE_ARGS) + { + fprintf(stderr, "[*] max command line args exceeded.\n"); + exit(EXIT_FAILURE); + } } } } @@ -661,7 +672,6 @@ save_args(int argc, char **argv) return; #endif - if (get_save_file(args_save_file)) { if ((args_file_ptr = fopen(args_save_file, "w")) == NULL) { fprintf(stderr, "Could not open args file: %s\n", @@ -680,6 +690,9 @@ save_args(int argc, char **argv) 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 5a60528..1e5ee2f 100644 --- a/client/utils.c +++ b/client/utils.c @@ -28,8 +28,6 @@ * ***************************************************************************** */ -#include -#include #include "utils.h" /* Generic hex dump function. @@ -69,5 +67,69 @@ 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) +{ +#if HAVE_STAT + struct stat st; + + /* Every file that the fwknop client deals with should be owned + * by the user and permissions set to 600 (user read/write) + */ + if((stat(file, &st)) != 0) + { + fprintf(stderr, "[-] unable to run stat() against file: %s: %s\n", + file, strerror(errno)); + exit(EXIT_FAILURE); + } + + /* Make sure it is a regular file or symbolic link + */ + if(S_ISREG(st.st_mode) != 1 && S_ISLNK(st.st_mode) != 1) + { + fprintf(stderr, + "[-] file: %s is not a regular file or symbolic link.\n", + file + ); + return 0; + } + + if((st.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != (S_IRUSR|S_IWUSR)) + { + fprintf(stderr, + "[-] file: %s permissions should only be user read/write (0600, -rw-------)\n", + file + ); + return 0; + } + + if(st.st_uid != getuid()) + { + fprintf(stderr, "[-] file: %s not owned by current effective user id.\n", + file); + return 0; + } +#endif + + return 1; +} /***EOF***/ diff --git a/client/utils.h b/client/utils.h index df618ae..672b8f3 100644 --- a/client/utils.h +++ b/client/utils.h @@ -31,10 +31,23 @@ #ifndef UTILS_H #define UTILS_H +#include +#include +#include +#include +#include +#include + +#if HAVE_CONFIG_H + #include "config.h" +#endif /* 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); size_t strlcpy(char *dst, const char *src, size_t siz); diff --git a/configure.ac b/configure.ac index 4a2ae3d..8690d21 100644 --- a/configure.ac +++ b/configure.ac @@ -242,7 +242,7 @@ AC_FUNC_MALLOC AC_FUNC_REALLOC AC_FUNC_STAT -AC_CHECK_FUNCS([bzero gettimeofday memmove memset socket strchr strcspn strdup strncasecmp strndup strrchr strspn]) +AC_CHECK_FUNCS([bzero gettimeofday memmove memset socket strchr strcspn strdup strncasecmp strndup strrchr strspn strnlen stat chmod chown]) AC_SEARCH_LIBS([socket], [socket]) AC_SEARCH_LIBS([inet_addr], [nsl]) diff --git a/server/access.c b/server/access.c index d1057f8..280e702 100644 --- a/server/access.c +++ b/server/access.c @@ -806,6 +806,8 @@ parse_access_file(fko_srv_options_t *opts) clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); } + verify_file_perms_ownership(opts->config[CONF_ACCESS_FILE]); + if ((file_ptr = fopen(opts->config[CONF_ACCESS_FILE], "r")) == NULL) { fprintf(stderr, "[*] Could not open access file: %s\n", diff --git a/server/config_init.c b/server/config_init.c index 70c3e7c..a728cda 100644 --- a/server/config_init.c +++ b/server/config_init.c @@ -200,6 +200,8 @@ parse_config_file(fko_srv_options_t *opts, const char *config_file) clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); } + verify_file_perms_ownership(config_file); + if ((cfile_ptr = fopen(config_file, "r")) == NULL) { fprintf(stderr, "[*] Could not open config file: %s\n", diff --git a/server/fwknopd.c b/server/fwknopd.c index 5a780b3..ec49d32 100644 --- a/server/fwknopd.c +++ b/server/fwknopd.c @@ -664,6 +664,8 @@ get_running_pid(const fko_srv_options_t *opts) char buf[PID_BUFLEN] = {0}; pid_t rpid = 0; + verify_file_perms_ownership(opts->config[CONF_FWKNOP_PID_FILE]); + op_fd = open(opts->config[CONF_FWKNOP_PID_FILE], O_RDONLY); if(op_fd > 0) diff --git a/server/replay_cache.c b/server/replay_cache.c index 2b164e9..227d64c 100644 --- a/server/replay_cache.c +++ b/server/replay_cache.c @@ -261,10 +261,14 @@ replay_file_cache_init(fko_srv_options_t *opts) fprintf(digest_file_ptr, "#