From b567514a6c722886fef5044a44abfc1514eff032 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Fri, 31 Aug 2012 22:59:44 -0400 Subject: [PATCH 01/11] Added fko_context.h file to lib/Makefile.am --- lib/Makefile.am | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Makefile.am b/lib/Makefile.am index b0ee5a74..fc1e93f3 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -7,7 +7,8 @@ libfko_source_files = \ fko_message.h fko_nat_access.c fko_rand_value.c fko_server_auth.c \ fko.h fko_limits.h fko_timestamp.c fko_user.c fko_util.h md5.c md5.h \ rijndael.c rijndael.h sha1.c sha1.h sha2.c sha2.h strlcat.c \ - strlcpy.c fko_state.h fko_context.h gpgme_funcs.c gpgme_funcs.h + strlcpy.c fko_context.h fko_state.h fko_context.h gpgme_funcs.c \ + gpgme_funcs.h libfko_la_SOURCES = $(libfko_source_files) From dafcfbc488f1e713ef6cfa9e86571a2b14e649d8 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Fri, 31 Aug 2012 23:00:45 -0400 Subject: [PATCH 02/11] bug fix to make sure to verify file permissions/ownership on files that actually exist --- client/fwknop.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/client/fwknop.c b/client/fwknop.c index 3c518da5..d3096ad0 100644 --- a/client/fwknop.c +++ b/client/fwknop.c @@ -558,14 +558,13 @@ 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", args_save_file); exit(EXIT_FAILURE); } + verify_file_perms_ownership(args_save_file); if ((fgets(args_str, MAX_LINE_LEN, args_file_ptr)) != NULL) { printf("Last fwknop client command line: %s", args_str); } else { @@ -603,14 +602,13 @@ 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", args_save_file); exit(EXIT_FAILURE); } + verify_file_perms_ownership(args_save_file); if ((fgets(args_str, MAX_LINE_LEN, args_file_ptr)) != NULL) { args_str[MAX_LINE_LEN-1] = '\0'; From 1548cbafc886af802b639913bb10e6a746222478 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Fri, 31 Aug 2012 23:05:05 -0400 Subject: [PATCH 03/11] get MAX_PORT_STR_LEN constant from fko_message.h --- client/spa_comm.c | 4 ++-- client/utils.c | 1 + common/common.h | 2 -- lib/fko.h | 1 + 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/client/spa_comm.c b/client/spa_comm.c index 53ba1a39..eabdb582 100644 --- a/client/spa_comm.c +++ b/client/spa_comm.c @@ -118,7 +118,7 @@ send_spa_packet_tcp_or_udp(const char *spa_data, const int sd_len, { int sock, res=0, error; struct addrinfo *result, *rp, hints; - char port_str[MAX_PORT_STR_LEN]; + char port_str[MAX_PORT_STR_LEN+1]; if (options->test) { @@ -147,7 +147,7 @@ send_spa_packet_tcp_or_udp(const char *spa_data, const int sd_len, hints.ai_protocol = IPPROTO_TCP; } - snprintf(port_str, MAX_PORT_STR_LEN, "%d", options->spa_dst_port); + snprintf(port_str, MAX_PORT_STR_LEN+1, "%d", options->spa_dst_port); error = getaddrinfo(options->spa_server_str, port_str, &hints, &result); diff --git a/client/utils.c b/client/utils.c index 1e5ee2fe..02718ceb 100644 --- a/client/utils.c +++ b/client/utils.c @@ -28,6 +28,7 @@ * ***************************************************************************** */ +#include "fwknop_common.h" #include "utils.h" /* Generic hex dump function. diff --git a/common/common.h b/common/common.h index 0c1c26da..9ec43888 100644 --- a/common/common.h +++ b/common/common.h @@ -102,8 +102,6 @@ enum { #define DEFAULT_NAT_PORT 55000 #define MIN_HIGH_PORT 10000 /* sensible minimum for SPA dest port */ #define MAX_PORT 65535 -#define MAX_PORT_STR_LEN 6 -#define MAX_PROTO_STR_LEN 6 #define MAX_SERVER_STR_LEN 50 #define MAX_LINE_LEN 1024 diff --git a/lib/fko.h b/lib/fko.h index 4910b17f..08c652e3 100644 --- a/lib/fko.h +++ b/lib/fko.h @@ -33,6 +33,7 @@ #include #include "fko_limits.h" +#include "fko_message.h" #ifdef __cplusplus extern "C" { From e3a78a175c664ee51de1fb8086deb96a1d017ac3 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sat, 1 Sep 2012 21:55:52 -0400 Subject: [PATCH 04/11] verify_file_perms_ownership() to just return if the file doesn't exist --- client/config_init.c | 2 +- client/fwknop.c | 4 ++-- client/utils.c | 23 ++++++++++++++++------- server/fwknopd.c | 3 ++- server/utils.c | 25 +++++++++++++++++-------- 5 files changed, 38 insertions(+), 19 deletions(-) diff --git a/client/config_init.c b/client/config_init.c index 5c001fca..e3c7e31d 100644 --- a/client/config_init.c +++ b/client/config_init.c @@ -448,7 +448,7 @@ process_rc(fko_cli_options_t *options) 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 + * then throw a warning. 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). */ diff --git a/client/fwknop.c b/client/fwknop.c index d3096ad0..1beb66ee 100644 --- a/client/fwknop.c +++ b/client/fwknop.c @@ -559,12 +559,12 @@ show_last_command(void) #endif 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", args_save_file); exit(EXIT_FAILURE); } - verify_file_perms_ownership(args_save_file); if ((fgets(args_str, MAX_LINE_LEN, args_file_ptr)) != NULL) { printf("Last fwknop client command line: %s", args_str); } else { @@ -602,13 +602,13 @@ 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", args_save_file); exit(EXIT_FAILURE); } - verify_file_perms_ownership(args_save_file); if ((fgets(args_str, MAX_LINE_LEN, args_file_ptr)) != NULL) { args_str[MAX_LINE_LEN-1] = '\0'; diff --git a/client/utils.c b/client/utils.c index 02718ceb..e40115ba 100644 --- a/client/utils.c +++ b/client/utils.c @@ -89,6 +89,7 @@ set_file_perms(const char *file) int verify_file_perms_ownership(const char *file) { + int res = 1; #if HAVE_STAT struct stat st; @@ -97,9 +98,17 @@ verify_file_perms_ownership(const char *file) */ if((stat(file, &st)) != 0) { - fprintf(stderr, "[-] unable to run stat() against file: %s: %s\n", - file, strerror(errno)); - exit(EXIT_FAILURE); + /* if the path doesn't exist, just return, but otherwise something + * went wrong + */ + if(errno == ENOENT) + { + return 0; + } else { + fprintf(stderr, "[-] stat() against file: %s returned: %s\n", + file, strerror(errno)); + exit(EXIT_FAILURE); + } } /* Make sure it is a regular file or symbolic link @@ -110,7 +119,7 @@ verify_file_perms_ownership(const char *file) "[-] file: %s is not a regular file or symbolic link.\n", file ); - return 0; + res = 0; } if((st.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != (S_IRUSR|S_IWUSR)) @@ -119,18 +128,18 @@ verify_file_perms_ownership(const char *file) "[-] file: %s permissions should only be user read/write (0600, -rw-------)\n", file ); - return 0; + res = 0; } if(st.st_uid != getuid()) { fprintf(stderr, "[-] file: %s not owned by current effective user id.\n", file); - return 0; + res = 0; } #endif - return 1; + return res; } /***EOF***/ diff --git a/server/fwknopd.c b/server/fwknopd.c index f66ef812..eabd5e7d 100644 --- a/server/fwknopd.c +++ b/server/fwknopd.c @@ -677,11 +677,12 @@ get_running_pid(const fko_srv_options_t *opts) 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) { - verify_file_perms_ownership(opts->config[CONF_FWKNOP_PID_FILE]); if (read(op_fd, buf, PID_BUFLEN) > 0) { buf[PID_BUFLEN-1] = '\0'; diff --git a/server/utils.c b/server/utils.c index 44636071..aa249402 100644 --- a/server/utils.c +++ b/server/utils.c @@ -184,17 +184,26 @@ set_file_perms(const char *file) int verify_file_perms_ownership(const char *file) { + int res = 1; #if HAVE_STAT struct stat st; - /* Every file that the fwknop client deals with should be owned + /* Every file that fwknopd 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 stat() file: %s: %s\n", - file, strerror(errno)); - exit(EXIT_FAILURE); + /* if the path doesn't exist, just return, but otherwise something + * went wrong + */ + if(errno == ENOENT) + { + return 0; + } else { + fprintf(stderr, "[-] stat() against file: %s returned: %s\n", + file, strerror(errno)); + exit(EXIT_FAILURE); + } } /* Make sure it is a regular file @@ -205,7 +214,7 @@ verify_file_perms_ownership(const char *file) "[-] file: %s is not a regular file or symbolic link.\n", file ); - return 0; + res = 0; } if((st.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != (S_IRUSR|S_IWUSR)) @@ -214,18 +223,18 @@ verify_file_perms_ownership(const char *file) "[-] file: %s permissions should only be user read/write (0600, -rw-------)\n", file ); - return 0; + res = 0; } if(st.st_uid != getuid()) { fprintf(stderr, "[-] file: %s not owned by current effective user id\n", file); - return 0; + res = 0; } #endif - return 1; + return res; } /* Determine if a buffer contains only characters from the base64 From 86b403dadb90c30deb51b3530e8ebbb791531615 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sat, 1 Sep 2012 23:37:03 -0400 Subject: [PATCH 05/11] fixed potential buffer overflow discovered by Fernando Arnaboldi of IOActive --- server/access.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/server/access.c b/server/access.c index 280e7024..5778c014 100644 --- a/server/access.c +++ b/server/access.c @@ -501,6 +501,12 @@ expand_acc_string_list(acc_string_list_t **stlist, char *stlist_str) while(isspace(*start)) start++; + if(((ndx-start)+1) >= 1024) + { + fprintf(stderr, "Fatal str->list too long"); + exit(EXIT_FAILURE); + } + strlcpy(buf, start, (ndx-start)+1); add_string_list_ent(stlist, buf); start = ndx+1; @@ -512,6 +518,12 @@ expand_acc_string_list(acc_string_list_t **stlist, char *stlist_str) while(isspace(*start)) start++; + if(((ndx-start)+1) >= 1024) + { + fprintf(stderr, "Fatal str->list too long"); + exit(EXIT_FAILURE); + } + strlcpy(buf, start, (ndx-start)+1); add_string_list_ent(stlist, buf); From ffe4d3b162bbfea143704461aab4244cc4acdfcf Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sun, 2 Sep 2012 15:53:54 -0400 Subject: [PATCH 06/11] minor spacing update to make merges into hmac_master easier --- test/test-fwknop.pl | 56 ++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/test/test-fwknop.pl b/test/test-fwknop.pl index 8efd78a3..bfe7dd4f 100755 --- a/test/test-fwknop.pl +++ b/test/test-fwknop.pl @@ -21,34 +21,34 @@ my $gpg_client_home_dir = "$conf_dir/client-gpg"; my $gpg_client_home_dir_no_pw = "$conf_dir/client-gpg-no-pw"; my %cf = ( - 'nat' => "$conf_dir/nat_fwknopd.conf", - 'def' => "$conf_dir/default_fwknopd.conf", - 'def_access' => "$conf_dir/default_access.conf", - 'exp_access' => "$conf_dir/expired_stanza_access.conf", - 'future_exp_access' => "$conf_dir/future_expired_stanza_access.conf", - 'exp_epoch_access' => "$conf_dir/expired_epoch_stanza_access.conf", - 'invalid_exp_access' => "$conf_dir/invalid_expire_access.conf", - 'force_nat_access' => "$conf_dir/force_nat_access.conf", - 'local_nat' => "$conf_dir/local_nat_fwknopd.conf", - 'ipfw_active_expire' => "$conf_dir/ipfw_active_expire_equal_fwknopd.conf", - 'dual_key_access' => "$conf_dir/dual_key_usage_access.conf", - 'gpg_access' => "$conf_dir/gpg_access.conf", - 'gpg_no_pw_access' => "$conf_dir/gpg_no_pw_access.conf", - 'open_ports_access' => "$conf_dir/open_ports_access.conf", - 'multi_gpg_access' => "$conf_dir/multi_gpg_access.conf", - 'multi_stanza_access' => "$conf_dir/multi_stanzas_access.conf", - 'broken_keys_access' => "$conf_dir/multi_stanzas_with_broken_keys.conf", - 'open_ports_mismatch' => "$conf_dir/mismatch_open_ports_access.conf", - 'require_user_access' => "$conf_dir/require_user_access.conf", - 'user_mismatch_access' => "$conf_dir/mismatch_user_access.conf", - 'require_src_access' => "$conf_dir/require_src_access.conf", - 'no_src_match' => "$conf_dir/no_source_match_access.conf", - 'no_subnet_match' => "$conf_dir/no_subnet_source_match_access.conf", - 'no_multi_src' => "$conf_dir/no_multi_source_match_access.conf", - 'multi_src_access' => "$conf_dir/multi_source_match_access.conf", - 'ip_src_match' => "$conf_dir/ip_source_match_access.conf", - 'subnet_src_match' => "$conf_dir/ip_source_match_access.conf", - 'disable_aging' => "$conf_dir/disable_aging_fwknopd.conf", + 'nat' => "$conf_dir/nat_fwknopd.conf", + 'def' => "$conf_dir/default_fwknopd.conf", + 'def_access' => "$conf_dir/default_access.conf", + 'exp_access' => "$conf_dir/expired_stanza_access.conf", + 'future_exp_access' => "$conf_dir/future_expired_stanza_access.conf", + 'exp_epoch_access' => "$conf_dir/expired_epoch_stanza_access.conf", + 'invalid_exp_access' => "$conf_dir/invalid_expire_access.conf", + 'force_nat_access' => "$conf_dir/force_nat_access.conf", + 'local_nat' => "$conf_dir/local_nat_fwknopd.conf", + 'ipfw_active_expire' => "$conf_dir/ipfw_active_expire_equal_fwknopd.conf", + 'dual_key_access' => "$conf_dir/dual_key_usage_access.conf", + 'gpg_access' => "$conf_dir/gpg_access.conf", + 'gpg_no_pw_access' => "$conf_dir/gpg_no_pw_access.conf", + 'open_ports_access' => "$conf_dir/open_ports_access.conf", + 'multi_gpg_access' => "$conf_dir/multi_gpg_access.conf", + 'multi_stanza_access' => "$conf_dir/multi_stanzas_access.conf", + 'broken_keys_access' => "$conf_dir/multi_stanzas_with_broken_keys.conf", + 'open_ports_mismatch' => "$conf_dir/mismatch_open_ports_access.conf", + 'require_user_access' => "$conf_dir/require_user_access.conf", + 'user_mismatch_access' => "$conf_dir/mismatch_user_access.conf", + 'require_src_access' => "$conf_dir/require_src_access.conf", + 'no_src_match' => "$conf_dir/no_source_match_access.conf", + 'no_subnet_match' => "$conf_dir/no_subnet_source_match_access.conf", + 'no_multi_src' => "$conf_dir/no_multi_source_match_access.conf", + 'multi_src_access' => "$conf_dir/multi_source_match_access.conf", + 'ip_src_match' => "$conf_dir/ip_source_match_access.conf", + 'subnet_src_match' => "$conf_dir/ip_source_match_access.conf", + 'disable_aging' => "$conf_dir/disable_aging_fwknopd.conf", ); my $default_digest_file = "$run_dir/digest.cache"; From 263fa01f2af1d336961df320f1c7a9ea84ddac9a Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Mon, 3 Sep 2012 00:21:32 -0400 Subject: [PATCH 07/11] added inet_aton() call for IP strong IP validation (credit: Fernando Arnaboldi) --- lib/fko_message.c | 29 ++++++++++++++++---- lib/fko_message.h | 5 ++++ test/conf/fuzzing_open_ports_access.conf | 4 +++ test/conf/fuzzing_restrict_ports_access.conf | 5 ++++ test/conf/fuzzing_source_access.conf | 4 +++ 5 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 test/conf/fuzzing_open_ports_access.conf create mode 100644 test/conf/fuzzing_restrict_ports_access.conf create mode 100644 test/conf/fuzzing_source_access.conf diff --git a/lib/fko_message.c b/lib/fko_message.c index 3228dfad..d74d58a5 100644 --- a/lib/fko_message.c +++ b/lib/fko_message.c @@ -29,6 +29,7 @@ * ***************************************************************************** */ +#include "fko_message.h" #include "fko_common.h" #include "fko.h" @@ -265,12 +266,17 @@ validate_nat_access_msg(const char *msg) int got_allow_ip(const char *msg) { - const char *ndx = msg; - int dot_ctr = 0, char_ctr = 0; - int res = FKO_SUCCESS; + const char *ndx = msg; + char ip_str[MAX_IPV4_STR_LEN]; + int dot_ctr = 0, char_ctr = 0; + int res = FKO_SUCCESS; +#if HAVE_SYS_SOCKET_H + struct in_addr in; +#endif while(*ndx != ',' && *ndx != '\0') { + ip_str[char_ctr] = *ndx; char_ctr++; if(char_ctr >= MAX_IPV4_STR_LEN) { @@ -287,12 +293,25 @@ got_allow_ip(const char *msg) ndx++; } - if (char_ctr < MIN_IPV4_STR_LEN) + if(char_ctr < MAX_IPV4_STR_LEN) + ip_str[char_ctr] = '\0'; + else res = FKO_ERROR_INVALID_ALLOW_IP; - if(dot_ctr != 3) + if ((res == FKO_SUCCESS) && (char_ctr < MIN_IPV4_STR_LEN)) res = FKO_ERROR_INVALID_ALLOW_IP; + if((res == FKO_SUCCESS) && dot_ctr != 3) + res = FKO_ERROR_INVALID_ALLOW_IP; + +#if HAVE_SYS_SOCKET_H + /* Stronger IP validation now that we have a candidate that looks + * close enough + */ + if((res == FKO_SUCCESS) && (inet_aton(ip_str, &in) == 0)) + res = FKO_ERROR_INVALID_ALLOW_IP; +#endif + return(res); } diff --git a/lib/fko_message.h b/lib/fko_message.h index 8350460e..7be313aa 100644 --- a/lib/fko_message.h +++ b/lib/fko_message.h @@ -32,6 +32,11 @@ #ifndef FKO_MESSAGE_H #define FKO_MESSAGE_H 1 +#if HAVE_SYS_SOCKET_H + #include +#endif +#include + #define MAX_PROTO_STR_LEN 4 /* tcp, udp, icmp for now */ #define MAX_PORT_STR_LEN 5 diff --git a/test/conf/fuzzing_open_ports_access.conf b/test/conf/fuzzing_open_ports_access.conf new file mode 100644 index 00000000..d79f1cea --- /dev/null +++ b/test/conf/fuzzing_open_ports_access.conf @@ -0,0 +1,4 @@ +SOURCE: 4.3.2.0/24, 127.0.0.0/24, 123.123.123.123/24, 23.43.0.0/16, 10.10.10.10; +OPEN_PORTS: udp/6001, tcp/22, tcp/80, tcp/123453; +KEY: fwknoptest; +FW_ACCESS_TIMEOUT: 3; diff --git a/test/conf/fuzzing_restrict_ports_access.conf b/test/conf/fuzzing_restrict_ports_access.conf new file mode 100644 index 00000000..baf6ae02 --- /dev/null +++ b/test/conf/fuzzing_restrict_ports_access.conf @@ -0,0 +1,5 @@ +SOURCE: 4.3.2.0/24, 127.0.0.0/24, 123.123.123.123/24, 23.43.0.0/16, 10.10.10.10; +OPEN_PORTS: udp/6001, tcp/22, tcp/80, tcp/12345; +RESTRICT_PORTS: AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA; +KEY: fwknoptest; +FW_ACCESS_TIMEOUT: 3; diff --git a/test/conf/fuzzing_source_access.conf b/test/conf/fuzzing_source_access.conf new file mode 100644 index 00000000..78281ba8 --- /dev/null +++ b/test/conf/fuzzing_source_access.conf @@ -0,0 +1,4 @@ +SOURCE: 4.3.2.0/24, 127.0.0.0/24, 123.123.123.1234/24, 23.43.0.0/16, A0.10.10.10; +OPEN_PORTS: udp/6001, tcp/22, tcp/80, tcp/12345; +KEY: fwknoptest; +FW_ACCESS_TIMEOUT: 3; From e2c0ac4821773eb335e36ad6cd35830b8d97c75a Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Mon, 3 Sep 2012 00:21:46 -0400 Subject: [PATCH 08/11] [server] Strong access.conf validation Fernando Arnaboldi from IOActive found several conditions in which the server did not properly throw out maliciously constructed variables in the access.conf file. This has been fixed along with new fuzzing tests in the test suite. --- CREDITS | 6 ++ ChangeLog | 4 ++ Makefile.am | 3 + server/access.c | 114 ++++++++++++++++++++----------- test/conf/open_ports_access.conf | 4 +- test/test-fwknop.pl | 42 ++++++++++++ 6 files changed, 133 insertions(+), 40 deletions(-) diff --git a/CREDITS b/CREDITS index 14753ea4..4a3dcf7a 100644 --- a/CREDITS +++ b/CREDITS @@ -60,3 +60,9 @@ Fernando Arnaboldi (IOActive) developed along with a new fuzzing capability in the test suite. - Found a condition in which an overly long IP from malicious authenticated clients is not properly validated by the fwknopd server (pre-2.0.3). + - 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. + - Found several conditions in which the server did not properly throw out + maliciously constructed variables in the access.conf file. This has been + fixed along with new fuzzing tests in the test suite. diff --git a/ChangeLog b/ChangeLog index 5e800b03..f42600b0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -21,6 +21,10 @@ fwknop-2.0.3 (08//2012): - [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. + - [server] Fernando Arnaboldi from IOActive found several conditions in + which the server did not properly throw out maliciously constructed + variables in the access.conf file. This has been fixed along with new + fuzzing tests in the test suite. - [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/Makefile.am b/Makefile.am index 1d83d5db..cbd39d2e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -150,6 +150,9 @@ EXTRA_DIST = \ test/conf/subnet_source_match_access.conf \ test/conf/local_nat_fwknopd.conf \ test/conf/disable_aging_fwknopd.conf \ + test/conf/fuzzing_source_access.conf \ + test/conf/fuzzing_open_ports_access.conf \ + test/conf/fuzzing_restrict_ports_access.conf \ test/hardening-check \ test/local_spa.key \ test/test-fwknop.pl \ diff --git a/server/access.c b/server/access.c index 5778c014..46090044 100644 --- a/server/access.c +++ b/server/access.c @@ -187,24 +187,6 @@ add_source_mask(acc_stanza_t *acc, const char *ip) exit(EXIT_FAILURE); } - /* If this is not the first entry, we walk our pointer to the - * end of the list. - */ - if(acc->source_list == NULL) - { - acc->source_list = new_sle; - } - else - { - tmp_sle = acc->source_list; - - do { - last_sle = tmp_sle; - } while((tmp_sle = tmp_sle->next)); - - last_sle->next = new_sle; - } - /* Convert the IP data into the appropriate mask */ if(strcasecmp(ip, "ANY") == 0) @@ -219,12 +201,27 @@ add_source_mask(acc_stanza_t *acc, const char *ip) */ if((ndx = strchr(ip, '/')) != NULL) { + if(((ndx-ip)) >= MAX_IPV4_STR_LEN) + { + log_msg(LOG_ERR, "Error parsing string to IP"); + free(new_sle); + new_sle = NULL; + return 0; + } + mask = atoi(ndx+1); strlcpy(ip_str, ip, (ndx-ip)+1); } else { mask = 32; + if(strnlen(ip, MAX_IPV4_STR_LEN+1) >= MAX_IPV4_STR_LEN) + { + log_msg(LOG_ERR, "Error parsing string to IP"); + free(new_sle); + new_sle = NULL; + return 0; + } strlcpy(ip_str, ip, strlen(ip)+1); } @@ -249,6 +246,25 @@ add_source_mask(acc_stanza_t *acc, const char *ip) */ new_sle->maddr = ntohl(in.s_addr) & new_sle->mask; } + + /* If this is not the first entry, we walk our pointer to the + * end of the list. + */ + if(acc->source_list == NULL) + { + acc->source_list = new_sle; + } + else + { + tmp_sle = acc->source_list; + + do { + last_sle = tmp_sle; + } while((tmp_sle = tmp_sle->next)); + + last_sle->next = new_sle; + } + return 1; } @@ -348,7 +364,7 @@ parse_proto_and_port(char *pstr, int *proto, int *port) /* Take a proto/port string and convert it to appropriate integer values * for comparisons of incoming SPA requests. */ -static void +static int add_port_list_ent(acc_port_list_t **plist, char *port_str) { int proto_int, port; @@ -359,7 +375,7 @@ add_port_list_ent(acc_port_list_t **plist, char *port_str) * are no problems with the incoming string. */ if(parse_proto_and_port(port_str, &proto_int, &port) != 0) - return; + return 0; if((new_plist = calloc(1, sizeof(acc_port_list_t))) == NULL) { @@ -389,6 +405,8 @@ add_port_list_ent(acc_port_list_t **plist, char *port_str) new_plist->proto = proto_int; new_plist->port = port; + + return 1; } /* Add a string list entry to the given acc_string_list. @@ -462,7 +480,10 @@ expand_acc_port_list(acc_port_list_t **plist, char *plist_str) return 0; strlcpy(buf, start, (ndx-start)+1); - add_port_list_ent(plist, buf); + + if(add_port_list_ent(plist, buf) == 0) + return 0; + start = ndx+1; } } @@ -477,14 +498,15 @@ expand_acc_port_list(acc_port_list_t **plist, char *plist_str) strlcpy(buf, start, (ndx-start)+1); - add_port_list_ent(plist, buf); + if(add_port_list_ent(plist, buf) == 0) + return 0; return 1; } /* Expand a comma-separated string into a simple acc_string_list. */ -static void +static int expand_acc_string_list(acc_string_list_t **stlist, char *stlist_str) { char *ndx, *start; @@ -502,10 +524,7 @@ expand_acc_string_list(acc_string_list_t **stlist, char *stlist_str) start++; if(((ndx-start)+1) >= 1024) - { - fprintf(stderr, "Fatal str->list too long"); - exit(EXIT_FAILURE); - } + return 0; strlcpy(buf, start, (ndx-start)+1); add_string_list_ent(stlist, buf); @@ -519,14 +538,13 @@ expand_acc_string_list(acc_string_list_t **stlist, char *stlist_str) start++; if(((ndx-start)+1) >= 1024) - { - fprintf(stderr, "Fatal str->list too long"); - exit(EXIT_FAILURE); - } + return 0; strlcpy(buf, start, (ndx-start)+1); add_string_list_ent(stlist, buf); + + return 1; } /* Free the acc source_list @@ -649,17 +667,29 @@ expand_acc_ent_lists(fko_srv_options_t *opts) */ if(expand_acc_source(acc) == 0) { - acc = acc->next; - continue; + log_msg(LOG_ERR, "Fatal invalid SOURCE in access stanza"); + clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); } /* Now expand the open_ports string. */ if(acc->open_ports != NULL && strlen(acc->open_ports)) - expand_acc_port_list(&(acc->oport_list), acc->open_ports); + { + if(expand_acc_port_list(&(acc->oport_list), acc->open_ports) == 0) + { + log_msg(LOG_ERR, "Fatal invalid OPEN_PORTS in access stanza"); + clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); + } + } if(acc->restrict_ports != NULL && strlen(acc->restrict_ports)) - expand_acc_port_list(&(acc->rport_list), acc->restrict_ports); + { + if(expand_acc_port_list(&(acc->rport_list), acc->restrict_ports) == 0) + { + log_msg(LOG_ERR, "Fatal invalid RESTRICT_PORTS in access stanza"); + clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); + } + } /* Expand the GPG_REMOTE_ID string. */ @@ -1072,7 +1102,6 @@ parse_access_file(fko_srv_options_t *opts) /* Expand our the expandable fields into their respective data buckets. */ - expand_acc_ent_lists(opts); /* Make sure default values are set where needed. @@ -1173,7 +1202,12 @@ acc_check_port_access(acc_stanza_t *acc, char *port_str) return(0); } strlcpy(buf, start, (ndx-start)+1); - add_port_list_ent(&in_pl, buf); + if(add_port_list_ent(&in_pl, buf) == 0) + { + log_msg(LOG_ERR, "Invalid proto/port string"); + return 0; + } + start = ndx+1; ctr = 0; } @@ -1189,7 +1223,11 @@ acc_check_port_access(acc_stanza_t *acc, char *port_str) return(0); } strlcpy(buf, start, (ndx-start)+1); - add_port_list_ent(&in_pl, buf); + if(add_port_list_ent(&in_pl, buf) == 0) + { + log_msg(LOG_ERR, "Invalid proto/port string"); + return 0; + } if(in_pl == NULL) { diff --git a/test/conf/open_ports_access.conf b/test/conf/open_ports_access.conf index 24966359..52150f75 100644 --- a/test/conf/open_ports_access.conf +++ b/test/conf/open_ports_access.conf @@ -1,4 +1,4 @@ -SOURCE: 4.3.2.0/24, 127.0.0.0/24, 23.43.0.0/16, 10.10.10.10; -OPEN_PORTS: udp/6001, tcp/22, tcp/80; +SOURCE: 4.3.2.0/24, 127.0.0.0/24, 123.123.123.123/24, 23.43.0.0/16, 10.10.10.10; +OPEN_PORTS: udp/6001, tcp/22, tcp/80, tcp/12345; KEY: fwknoptest; FW_ACCESS_TIMEOUT: 3; diff --git a/test/test-fwknop.pl b/test/test-fwknop.pl index bfe7dd4f..b678d316 100755 --- a/test/test-fwknop.pl +++ b/test/test-fwknop.pl @@ -49,6 +49,9 @@ my %cf = ( 'ip_src_match' => "$conf_dir/ip_source_match_access.conf", 'subnet_src_match' => "$conf_dir/ip_source_match_access.conf", 'disable_aging' => "$conf_dir/disable_aging_fwknopd.conf", + 'fuzz_source' => "$conf_dir/fuzzing_source_access.conf", + 'fuzz_open_ports' => "$conf_dir/fuzzing_open_ports_access.conf", + 'fuzz_restrict_ports' => "$conf_dir/fuzzing_restrict_ports_access.conf", ); my $default_digest_file = "$run_dir/digest.cache"; @@ -1448,6 +1451,45 @@ my @tests = ( "-d $default_digest_file -p $default_pid_file $intf_str", 'fatal' => $NO }, + { + 'category' => 'FUZZING', + 'subcategory' => 'server', + 'detail' => 'invalid SOURCE access.conf', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&generic_exec, + 'cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'fuzz_source'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'positive_output_matches' => [qr/Fatal\sinvalid/], + 'exec_err' => $YES, + 'fatal' => $NO + }, + { + 'category' => 'FUZZING', + 'subcategory' => 'server', + 'detail' => 'invalid OPEN_PORTS access.conf', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&generic_exec, + 'cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'fuzz_open_ports'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'positive_output_matches' => [qr/Fatal\sinvalid/], + 'exec_err' => $YES, + 'fatal' => $NO + }, + { + 'category' => 'FUZZING', + 'subcategory' => 'server', + 'detail' => 'invalid RESTRICT_PORTS access.conf', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&generic_exec, + 'cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'fuzz_restrict_ports'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'positive_output_matches' => [qr/Fatal\sinvalid/], + 'exec_err' => $YES, + 'fatal' => $NO + }, { 'category' => 'Rijndael SPA', From b05d229bb15cb77a17a28a146b8b0dc61afa4aa9 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Mon, 3 Sep 2012 09:09:35 -0400 Subject: [PATCH 09/11] sprintf() -> snprintf() calls --- lib/fko_encryption.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/fko_encryption.c b/lib/fko_encryption.c index bc2a80a4..e75a2222 100644 --- a/lib/fko_encryption.c +++ b/lib/fko_encryption.c @@ -57,7 +57,8 @@ _rijndael_encrypt(fko_ctx_t ctx, const char *enc_key) if(plain == NULL) return(FKO_ERROR_MEMORY_ALLOCATION); - sprintf(plain, "%s:%s", ctx->encoded_msg, ctx->digest); + snprintf(plain, strlen(ctx->encoded_msg) + strlen(ctx->digest) + 2, + "%s:%s", ctx->encoded_msg, ctx->digest); /* Make a bucket for the encrypted version and populate it. */ @@ -199,7 +200,8 @@ gpg_encrypt(fko_ctx_t ctx, const char *enc_key) if(plain == NULL) return(FKO_ERROR_MEMORY_ALLOCATION); - sprintf(plain, "%s:%s", ctx->encoded_msg, ctx->digest); + snprintf(plain, strlen(ctx->encoded_msg) + strlen(ctx->digest) + 2, + "%s:%s", ctx->encoded_msg, ctx->digest); res = gpgme_encrypt(ctx, (unsigned char*)plain, strlen(plain), From 8d26cc90ee76ba95d58ee18d90431a9883a2a89a Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Mon, 3 Sep 2012 22:18:59 -0400 Subject: [PATCH 10/11] include file compilation fix for OpenBSD relative to inet_aton() IP verification --- lib/fko_message.c | 2 +- lib/fko_message.h | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/fko_message.c b/lib/fko_message.c index d74d58a5..d7361f94 100644 --- a/lib/fko_message.c +++ b/lib/fko_message.c @@ -29,8 +29,8 @@ * ***************************************************************************** */ -#include "fko_message.h" #include "fko_common.h" +#include "fko_message.h" #include "fko.h" /* Set the SPA message type. diff --git a/lib/fko_message.h b/lib/fko_message.h index 7be313aa..cb61ed11 100644 --- a/lib/fko_message.h +++ b/lib/fko_message.h @@ -32,8 +32,13 @@ #ifndef FKO_MESSAGE_H #define FKO_MESSAGE_H 1 -#if HAVE_SYS_SOCKET_H - #include +#if PLATFORM_OPENBSD + #include + #include +#else + #if HAVE_SYS_SOCKET_H + #include + #endif #endif #include From 40ac28df21fab384f1389607eed78f6d35159206 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Mon, 3 Sep 2012 22:23:48 -0400 Subject: [PATCH 11/11] bump version to 2.0.3 --- ChangeLog | 6 +++--- VERSION | 2 +- android/project/jni/config.h | 6 +++--- configure.ac | 2 +- fwknop.spec | 2 +- iphone/Classes/config.h | 6 +++--- lib/fko.h | 2 +- todo.org | 3 +++ 8 files changed, 16 insertions(+), 13 deletions(-) diff --git a/ChangeLog b/ChangeLog index f42600b0..446103b1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,6 +1,4 @@ -fwknop-2.0.3 (08//2012): - - Fixed RPM builds by including the $(DESTDIR) prefix for uninstall-local - and install-exec-hook stages in Makefile.am. +fwknop-2.0.3 (09/03/2012): - [server] Fernando Arnaboldi from IOActive found several DoS/code execution vulnerabilities for malicious fwknop clients that manage to get past the authentication stage (so a such a client must be in @@ -28,6 +26,8 @@ fwknop-2.0.3 (08//2012): - [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. + - Fixed RPM builds by including the $(DESTDIR) prefix for uninstall-local + and install-exec-hook stages in Makefile.am. fwknop-2.0.2 (08/18/2012): - [server] For GPG mode, added a new access.conf variable diff --git a/VERSION b/VERSION index 50a5a980..3103e619 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -fwknop-2.0.2 +fwknop-2.0.3 diff --git a/android/project/jni/config.h b/android/project/jni/config.h index e578588b..85727474 100644 --- a/android/project/jni/config.h +++ b/android/project/jni/config.h @@ -207,13 +207,13 @@ #define PACKAGE_NAME "fwknop" /* Define to the full name and version of this package. */ -#define PACKAGE_STRING "fwknop 2.0.2" +#define PACKAGE_STRING "fwknop 2.0.3" /* Define to the one symbol short name of this package. */ #define PACKAGE_TARNAME "fwknop" /* Define to the version of this package. */ -#define PACKAGE_VERSION "2.0.2" +#define PACKAGE_VERSION "2.0.3" /* The size of `unsigned int', as computed by sizeof. */ #define SIZEOF_UNSIGNED_INT 4 @@ -247,7 +247,7 @@ /* Version number of package */ -#define VERSION "2.0.2" +#define VERSION "2.0.3" /* Define WORDS_BIGENDIAN to 1 if your processor stores words with the most significant byte first (like Motorola and SPARC, unlike Intel). */ diff --git a/configure.ac b/configure.ac index 8690d216..1a90a175 100644 --- a/configure.ac +++ b/configure.ac @@ -11,7 +11,7 @@ AC_PREREQ(2.62) dnl Define our name, version and email. m4_define(my_package, [fwknop]) -m4_define(my_version, [2.0.2]) +m4_define(my_version, [2.0.3]) m4_define(my_bug_email, [dstuart@dstuart.org]) AC_INIT(my_package, my_version, my_bug_email) diff --git a/fwknop.spec b/fwknop.spec index 09942efb..87dc5d97 100644 --- a/fwknop.spec +++ b/fwknop.spec @@ -13,7 +13,7 @@ %define _mandir /usr/share/man Name: fwknop -Version: 2.0.2 +Version: 2.0.3 Epoch: 1 Release: 1%{?dist} Summary: Firewall Knock Operator client. An implementation of Single Packet Authorization. diff --git a/iphone/Classes/config.h b/iphone/Classes/config.h index f241fbb5..1c817868 100644 --- a/iphone/Classes/config.h +++ b/iphone/Classes/config.h @@ -203,13 +203,13 @@ Copyright (C) Max Kastanas 2010 #define PACKAGE_NAME "fwknop" /* Define to the full name and version of this package. */ -#define PACKAGE_STRING "fwknop 2.0.2" +#define PACKAGE_STRING "fwknop 2.0.3" /* Define to the one symbol short name of this package. */ #define PACKAGE_TARNAME "fwknop" /* Define to the version of this package. */ -#define PACKAGE_VERSION "2.0.2" +#define PACKAGE_VERSION "2.0.3" /* The size of `unsigned int', as computed by sizeof. */ #define SIZEOF_UNSIGNED_INT 4 @@ -243,7 +243,7 @@ Copyright (C) Max Kastanas 2010 /* Version number of package */ -#define VERSION "2.0.2" +#define VERSION "2.0.3" /* Define WORDS_BIGENDIAN to 1 if your processor stores words with the most significant byte first (like Motorola and SPARC, unlike Intel). */ diff --git a/lib/fko.h b/lib/fko.h index 08c652e3..87389c72 100644 --- a/lib/fko.h +++ b/lib/fko.h @@ -55,7 +55,7 @@ extern "C" { /* General params */ -#define FKO_PROTOCOL_VERSION "2.0.2" /* The fwknop protocol version */ +#define FKO_PROTOCOL_VERSION "2.0.3" /* The fwknop protocol version */ /* Supported FKO Message types... */ diff --git a/todo.org b/todo.org index 33b75e52..541e84d1 100644 --- a/todo.org +++ b/todo.org @@ -9,6 +9,9 @@ *** Release fwknop-2.0.2 :CLOSED: <2012-08-18 Sat> Make the fwknop-2.0.2 release. +*** Release fwknop-2.0.3 + :CLOSED: <2012-09-03 Mon> + Make the fwknop-2.0.3 release. *** Update fwknopd man page for GPG_ALLOW_NO_PW :CLOSED: <2012-08-14 Tue> *** Preserve existing configs under 'make install'