From b0bf7f369918989bae364730c8952258aac693c6 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sat, 18 Aug 2012 16:30:34 -0400 Subject: [PATCH 01/14] minor paren's syntax bug fix --- server/incoming_spa.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/incoming_spa.c b/server/incoming_spa.c index 726cc069..a7be5f6e 100644 --- a/server/incoming_spa.c +++ b/server/incoming_spa.c @@ -285,6 +285,7 @@ incoming_spa(fko_srv_options_t *opts) if (is_src_match(opts->acc_stanzas, ntohl(spa_pkt->packet_src_ip))) { if(strncasecmp(opts->config[CONF_ENABLE_DIGEST_PERSISTENCE], "Y", 1) == 0) + { /* Check for a replay attack */ res = get_raw_digest(&raw_digest, (char *)spa_pkt->packet_data); @@ -302,6 +303,7 @@ incoming_spa(fko_srv_options_t *opts) free(raw_digest); return; } + } } else { From d46ba1c027a11e45821ba897a4928819bccc8f22 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Fri, 24 Aug 2012 22:12:19 -0400 Subject: [PATCH 02/14] (Fernando Arnaboldi, IOActive) Found and fixed several DoS/code execution vulns for authenticated clients - [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 possession of a valid access.conf encryption key). These vulnerbilities manifested themselves in the handling of malformed access requests, and both the fwknopd server code along with libfko now perform stronger input validation of access request data. These vulnerabilities affect pre-2.0.3 fwknop releases. - [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. --- CREDITS | 5 + ChangeLog | 13 + Makefile.am | 1 + lib/fko_message.c | 23 +- lib/fko_message.h | 3 + server/access.c | 89 ++++-- server/access.h | 4 +- server/fw_util_iptables.c | 3 +- test/conf/disable_aging_fwknopd.conf | 5 + test/test-fwknop.pl | 413 ++++++++++++++++++++++++++- 10 files changed, 531 insertions(+), 28 deletions(-) create mode 100644 test/conf/disable_aging_fwknopd.conf diff --git a/CREDITS b/CREDITS index ab6edd06..71f0fa84 100644 --- a/CREDITS +++ b/CREDITS @@ -53,3 +53,8 @@ Hank Leininger - For iptables firewalls, suggested a check for the 'comment' match to ensure the local environment will properly support fwknopd operations. The result is the new ENABLE_IPT_COMMENT_CHECK functionality. + +Fernando Arnaboldi (IOActive) + - Found important buffer overflow conditions for authenticated SPA clients + in the fwknopd server (pre-2.0.3). These findings enabled fixes to be + developed along with a new fuzzing capability in the test suite. diff --git a/ChangeLog b/ChangeLog index 1568a1aa..00f3a372 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,16 @@ +fwknop-2.0.3 (08//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 + possession of a valid access.conf encryption key). These vulnerbilities + manifested themselves in the handling of malformed access requests, and + both the fwknopd server code along with libfko now perform stronger input + validation of access request data. These vulnerabilities affect + pre-2.0.3 fwknop releases. + - [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. + fwknop-2.0.2 (08/18/2012): - [server] For GPG mode, added a new access.conf variable "GPG_ALLOW_NO_PW" to make it possible to leverage a server-side GPG key diff --git a/Makefile.am b/Makefile.am index 9df6222e..7306bbc5 100644 --- a/Makefile.am +++ b/Makefile.am @@ -149,6 +149,7 @@ EXTRA_DIST = \ test/conf/require_user_access.conf \ test/conf/subnet_source_match_access.conf \ test/conf/local_nat_fwknopd.conf \ + test/conf/disable_aging_fwknopd.conf \ test/hardening-check \ test/local_spa.key \ test/test-fwknop.pl \ diff --git a/lib/fko_message.c b/lib/fko_message.c index 061bb738..9148c2d3 100644 --- a/lib/fko_message.c +++ b/lib/fko_message.c @@ -200,6 +200,8 @@ validate_access_msg(const char *msg) do { ndx++; res = validate_proto_port_spec(ndx); + if(res != FKO_SUCCESS) + break; } while((ndx = strchr(ndx, ','))); return(res); @@ -208,14 +210,13 @@ validate_access_msg(const char *msg) int validate_proto_port_spec(const char *msg) { - int startlen = strnlen(msg, MAX_SPA_MESSAGE_SIZE); + int startlen = strnlen(msg, MAX_SPA_MESSAGE_SIZE), port_str_len = 0; const char *ndx = msg; if(startlen == MAX_SPA_MESSAGE_SIZE) return(FKO_ERROR_INVALID_DATA); - /* Now check for proto/port string. Currenly we only allow protos - * 'tcp', 'udp', and 'icmp'. + /* Now check for proto/port string. */ if(strncmp(ndx, "tcp", 3) && strncmp(ndx, "udp", 3) @@ -224,19 +225,25 @@ validate_proto_port_spec(const char *msg) return(FKO_ERROR_INVALID_SPA_ACCESS_MSG); ndx = strchr(ndx, '/'); - if(ndx == NULL || (1+(ndx - msg)) >= startlen) + if(ndx == NULL || ((1+(ndx - msg)) > MAX_PROTO_STR_LEN)) return(FKO_ERROR_INVALID_SPA_ACCESS_MSG); - /* Skip over the ',' and make sure we only have digits. + /* Skip over the '/' and make sure we only have digits. */ ndx++; - while(*ndx != '\0') + + /* Must have at least one digit for the port number + */ + if(isdigit(*ndx) == 0) + return(FKO_ERROR_INVALID_SPA_ACCESS_MSG); + + while(*ndx != '\0' && *ndx != ',') { - if(isdigit(*ndx) == 0) + port_str_len++; + if((isdigit(*ndx) == 0) || (port_str_len > MAX_PORT_STR_LEN)) return(FKO_ERROR_INVALID_SPA_ACCESS_MSG); ndx++; } - return(FKO_SUCCESS); } diff --git a/lib/fko_message.h b/lib/fko_message.h index f56e33ff..8350460e 100644 --- a/lib/fko_message.h +++ b/lib/fko_message.h @@ -32,6 +32,9 @@ #ifndef FKO_MESSAGE_H #define FKO_MESSAGE_H 1 +#define MAX_PROTO_STR_LEN 4 /* tcp, udp, icmp for now */ +#define MAX_PORT_STR_LEN 5 + /* SPA message format validation functions. */ int validate_cmd_msg(const char *msg); diff --git a/server/access.c b/server/access.c index c81fb936..91feb0b7 100644 --- a/server/access.c +++ b/server/access.c @@ -168,7 +168,7 @@ add_acc_force_nat(fko_srv_options_t *opts, acc_stanza_t *curr_acc, const char *v /* Take an IP or Subnet/Mask and convert it to mask for later * comparisons of incoming source IPs against this mask. */ -static void +static int add_source_mask(acc_stanza_t *acc, const char *ip) { char *ndx; @@ -237,7 +237,7 @@ add_source_mask(acc_stanza_t *acc, const char *ip) free(new_sle); new_sle = NULL; - return; + return 0; } /* Store our mask converted from CIDR to a 32-bit value. @@ -249,15 +249,17 @@ add_source_mask(acc_stanza_t *acc, const char *ip) */ new_sle->maddr = ntohl(in.s_addr) & new_sle->mask; } + return 1; } /* Expand the access SOURCE string to a list of masks. */ -void +static int expand_acc_source(acc_stanza_t *acc) { char *ndx, *start; - char buf[32]; + char buf[ACCESS_BUF_LEN]; + int res = 1; start = acc->source; @@ -270,8 +272,13 @@ expand_acc_source(acc_stanza_t *acc) while(isspace(*start)) start++; + if(((ndx-start)+1) >= ACCESS_BUF_LEN) + return 0; + strlcpy(buf, start, (ndx-start)+1); - add_source_mask(acc, buf); + res = add_source_mask(acc, buf); + if(res == 0) + return res; start = ndx+1; } } @@ -281,15 +288,20 @@ expand_acc_source(acc_stanza_t *acc) while(isspace(*start)) start++; + if(((ndx-start)+1) >= ACCESS_BUF_LEN) + return 0; + strlcpy(buf, start, (ndx-start)+1); - add_source_mask(acc, buf); + res = add_source_mask(acc, buf); + + return res; } static int parse_proto_and_port(char *pstr, int *proto, int *port) { char *ndx; - char proto_str[32]; + char proto_str[ACCESS_BUF_LEN]; /* Parse the string into its components. */ @@ -301,10 +313,24 @@ parse_proto_and_port(char *pstr, int *proto, int *port) return(-1); } - strlcpy(proto_str, pstr, (ndx - pstr)+1); + if(((ndx - pstr)+1) >= ACCESS_BUF_LEN) + { + log_msg(LOG_ERR, + "Parse error on access port entry: %s", pstr); + return(-1); + } + + strlcpy(proto_str, pstr, (ndx - pstr)+1); *port = atoi(ndx+1); + if((*port < 0) || (*port > MAX_PORT)) + { + log_msg(LOG_ERR, + "Invalid port in access request: %s", pstr); + return(-1); + } + if(strcasecmp(proto_str, "tcp") == 0) *proto = PROTO_TCP; else if(strcasecmp(proto_str, "udp") == 0) @@ -313,7 +339,6 @@ parse_proto_and_port(char *pstr, int *proto, int *port) { log_msg(LOG_ERR, "Invalid protocol in access port entry: %s", pstr); - return(-1); } @@ -416,15 +441,15 @@ add_string_list_ent(acc_string_list_t **stlist, const char *str_str) /* Expand a proto/port access string to a list of access proto-port struct. */ -void +int expand_acc_port_list(acc_port_list_t **plist, char *plist_str) { char *ndx, *start; - char buf[32]; + char buf[ACCESS_BUF_LEN]; start = plist_str; - for(ndx = start; *ndx; ndx++) + for(ndx = start; *ndx != '\0'; ndx++) { if(*ndx == ',') { @@ -433,6 +458,9 @@ expand_acc_port_list(acc_port_list_t **plist, char *plist_str) while(isspace(*start)) start++; + if(((ndx-start)+1) >= ACCESS_BUF_LEN) + return 0; + strlcpy(buf, start, (ndx-start)+1); add_port_list_ent(plist, buf); start = ndx+1; @@ -444,9 +472,14 @@ expand_acc_port_list(acc_port_list_t **plist, char *plist_str) while(isspace(*start)) start++; + if(((ndx-start)+1) >= ACCESS_BUF_LEN) + return 0; + strlcpy(buf, start, (ndx-start)+1); add_port_list_ent(plist, buf); + + return 1; } /* Expand a comma-separated string into a simple acc_string_list. @@ -602,7 +635,11 @@ expand_acc_ent_lists(fko_srv_options_t *opts) { /* Expand the source string to 32-bit integer masks foreach entry. */ - expand_acc_source(acc); + if(expand_acc_source(acc) == 0) + { + acc = acc->next; + continue; + } /* Now expand the open_ports string. */ @@ -1086,9 +1123,9 @@ compare_port_list(acc_port_list_t *in, acc_port_list_t *ac, const int match_any) int acc_check_port_access(acc_stanza_t *acc, char *port_str) { - int res = 1; + int res = 1, ctr = 0; - char buf[32]; + char buf[ACCESS_BUF_LEN]; char *ndx, *start; acc_port_list_t *o_pl = acc->oport_list; @@ -1101,14 +1138,34 @@ acc_check_port_access(acc_stanza_t *acc, char *port_str) /* Create our own internal port_list from the incoming SPA data * for comparison. */ - for(ndx = start; *ndx; ndx++) + for(ndx = start; *ndx != '\0'; ndx++) { if(*ndx == ',') { + if((ctr >= ACCESS_BUF_LEN) + || (((ndx-start)+1) >= ACCESS_BUF_LEN)) + { + log_msg(LOG_ERR, + "Unable to create acc_port_list from incoming data: %s", + port_str + ); + return(0); + } strlcpy(buf, start, (ndx-start)+1); add_port_list_ent(&in_pl, buf); start = ndx+1; + ctr = 0; } + ctr++; + } + if((ctr >= ACCESS_BUF_LEN) + || (((ndx-start)+1) >= ACCESS_BUF_LEN)) + { + log_msg(LOG_ERR, + "Unable to create acc_port_list from incoming data: %s", + port_str + ); + return(0); } strlcpy(buf, start, (ndx-start)+1); add_port_list_ent(&in_pl, buf); diff --git a/server/access.h b/server/access.h index b6501014..4ecae19d 100644 --- a/server/access.h +++ b/server/access.h @@ -34,6 +34,8 @@ #define PROTO_TCP 6 #define PROTO_UDP 17 +#define ACCESS_BUF_LEN 32 + /* Function Prototypes */ void parse_access_file(fko_srv_options_t *opts); @@ -41,7 +43,7 @@ int compare_addr_list(acc_int_list_t *source_list, const uint32_t ip); int acc_check_port_access(acc_stanza_t *acc, char *port_str); int acc_check_gpg_remote_id(acc_stanza_t *acc, const char *gpg_id); void dump_access_list(const fko_srv_options_t *opts); -void expand_acc_port_list(acc_port_list_t **plist, char *plist_str); +int expand_acc_port_list(acc_port_list_t **plist, char *plist_str); void free_acc_stanzas(fko_srv_options_t *opts); void free_acc_port_list(acc_port_list_t *plist); diff --git a/server/fw_util_iptables.c b/server/fw_util_iptables.c index ad1648a5..e1b39888 100644 --- a/server/fw_util_iptables.c +++ b/server/fw_util_iptables.c @@ -582,7 +582,8 @@ process_spa_request(const fko_srv_options_t *opts, const acc_stanza_t *acc, spa_ /* Parse and expand our access message. */ - expand_acc_port_list(&port_list, spadat->spa_message_remain); + if(expand_acc_port_list(&port_list, spadat->spa_message_remain) != 1) + return res; /* Start at the top of the proto-port list... */ diff --git a/test/conf/disable_aging_fwknopd.conf b/test/conf/disable_aging_fwknopd.conf new file mode 100644 index 00000000..195a85be --- /dev/null +++ b/test/conf/disable_aging_fwknopd.conf @@ -0,0 +1,5 @@ +# +# The default fwknopd.conf contains only comments since defaults are defined in +# code and modified via the config file +# +ENABLE_SPA_PACKET_AGING N; diff --git a/test/test-fwknop.pl b/test/test-fwknop.pl index 0fd8adf0..2f78bb44 100755 --- a/test/test-fwknop.pl +++ b/test/test-fwknop.pl @@ -48,6 +48,7 @@ my %cf = ( '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"; @@ -508,7 +509,7 @@ my @tests = ( { 'category' => 'basic operations', 'subcategory' => 'client', - 'detail' => '-A / specification', + 'detail' => '-A / specification (proto)', 'err_msg' => 'permitted invalid -A /', 'function' => \&generic_exec, 'positive_output_matches' => [qr/Invalid\sSPA\saccess\smessage/i], @@ -517,6 +518,19 @@ my @tests = ( "$fwknopCmd -A invalid/22 -a $fake_ip -D $loopback_ip", 'fatal' => $NO }, + { + 'category' => 'basic operations', + 'subcategory' => 'client', + 'detail' => '-A / specification (port)', + 'err_msg' => 'permitted invalid -A /', + 'function' => \&generic_exec, + 'positive_output_matches' => [qr/Invalid\sSPA\saccess\smessage/i], + 'exec_err' => $YES, + 'cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopCmd -A tcp/600001 -a $fake_ip -D $loopback_ip", + 'fatal' => $NO + }, + { 'category' => 'basic operations', 'subcategory' => 'client', @@ -1069,6 +1083,52 @@ my @tests = ( 'fw_rule_removed' => $NEW_RULE_REMOVED, 'fatal' => $NO }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'client+server', + 'detail' => 'complete cycle (tcp/60001)', + 'err_msg' => 'could not complete SPA cycle', + 'function' => \&spa_cycle, + 'cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopCmd -A tcp/60001 -a $fake_ip -D $loopback_ip --get-key " . + "$local_key_file --verbose --verbose", + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd $default_server_conf_args $intf_str", + 'fw_rule_created' => $NEW_RULE_REQUIRED, + 'fw_rule_removed' => $NEW_RULE_REMOVED, + 'fatal' => $NO + }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'client+server', + 'detail' => 'multi port (tcp/60001,udp/60001)', + 'err_msg' => 'could not complete SPA cycle', + 'function' => \&spa_cycle, + 'cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopCmd -A tcp/60001,udp/60001 -a $fake_ip -D $loopback_ip --get-key " . + "$local_key_file --verbose --verbose", + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd $default_server_conf_args $intf_str", + 'fw_rule_created' => $NEW_RULE_REQUIRED, + 'fw_rule_removed' => $NEW_RULE_REMOVED, + 'fatal' => $NO + }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'client+server', + 'detail' => 'multi port (tcp/22,udp/53,tcp/1234)', + 'err_msg' => 'could not complete SPA cycle', + 'function' => \&spa_cycle, + 'cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopCmd -A tcp/22,udp/53,tcp/1234 -a $fake_ip -D $loopback_ip --get-key " . + "$local_key_file --verbose --verbose", + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd $default_server_conf_args $intf_str", + 'fw_rule_created' => $NEW_RULE_REQUIRED, + 'fw_rule_removed' => $NEW_RULE_REMOVED, + 'fatal' => $NO + }, + { 'category' => 'Rijndael SPA', 'subcategory' => 'client+server', @@ -1154,6 +1214,64 @@ my @tests = ( 'replay_positive_output_matches' => [qr/Data\sis\snot\sa\svalid\sSPA\smessage\sformat/], 'fatal' => $NO }, + + ### fuzzing tests + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'FUZZING', + 'detail' => 'overly long port value', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&overly_long_port, + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'fatal' => $NO + }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'FUZZING', + 'detail' => 'overly long proto value', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&overly_long_proto, + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'fatal' => $NO + }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'FUZZING', + 'detail' => 'negative port value', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&negative_port, + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'fatal' => $NO + }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'FUZZING', + 'detail' => 'null port value', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&null_port, + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'fatal' => $NO + }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'FUZZING', + 'detail' => 'null proto value', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&null_proto, + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'fatal' => $NO + }, + { 'category' => 'Rijndael SPA', 'subcategory' => 'server', @@ -1287,6 +1405,24 @@ my @tests = ( 'fw_rule_removed' => $NEW_RULE_REMOVED, 'fatal' => $NO }, + { + 'category' => 'GPG (no pw) SPA', + 'subcategory' => 'client+server', + 'detail' => 'complete cycle (tcp/60001)', + 'err_msg' => 'could not complete SPA cycle', + 'function' => \&spa_cycle, + 'cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopCmd -A tcp/60001 -a $fake_ip -D $loopback_ip --get-key " . + "$local_key_file --verbose --verbose " . + "--gpg-recipient-key $gpg_server_key " . + "--gpg-signer-key $gpg_client_key " . + "--gpg-home-dir $gpg_client_home_dir_no_pw", + 'fwknopd_cmdline' => $default_server_gpg_args_no_pw, + 'fw_rule_created' => $NEW_RULE_REQUIRED, + 'fw_rule_removed' => $NEW_RULE_REMOVED, + 'fatal' => $NO + }, + { 'category' => 'GPG (no pw) SPA', 'subcategory' => 'client+server', @@ -1450,6 +1586,24 @@ my @tests = ( 'fw_rule_removed' => $NEW_RULE_REMOVED, 'fatal' => $NO }, + { + 'category' => 'GnuPG (GPG) SPA', + 'subcategory' => 'client+server', + 'detail' => 'complete cycle (tcp/60001)', + 'err_msg' => 'could not complete SPA cycle', + 'function' => \&spa_cycle, + 'cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopCmd -A tcp/60001 -a $fake_ip -D $loopback_ip --get-key " . + "$local_key_file --verbose --verbose " . + "--gpg-recipient-key $gpg_server_key " . + "--gpg-signer-key $gpg_client_key " . + "--gpg-home-dir $gpg_client_home_dir", + 'fwknopd_cmdline' => $default_server_gpg_args, + 'fw_rule_created' => $NEW_RULE_REQUIRED, + 'fw_rule_removed' => $NEW_RULE_REMOVED, + 'fatal' => $NO + }, + { 'category' => 'GnuPG (GPG) SPA', 'subcategory' => 'client+server', @@ -2112,6 +2266,261 @@ sub altered_non_base64_spa_data() { return $rv; } +sub null_proto() { + my $test_hr = shift; + + my $rv = 1; + my $server_was_stopped = 0; + my $fw_rule_created = 0; + my $fw_rule_removed = 0; + + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A /22 \ + # -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key \ + # --verbose --verbose + # + my $spa_pkt = + '/JT14qxh9P4iy+CuUZahThaQjoEuL2zd46a+jL6sTrBZJSa6faUX4dH5fte/4ZJv+9f' . + 'd/diWYKAUvdQ4DydPGlR7mwQa2W+obKpqrsTBz7D4054z6ATAOGpCtifakEVl1XRc2+' . + 'hW04WpY8mdUNu9i+PrfPr7/KxqU'; + + my @packets = ( + { + 'proto' => 'udp', + 'port' => $default_spa_port, + 'dst_ip' => $loopback_ip, + 'data' => $spa_pkt, + }, + ); + + ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) + = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); + + $rv = 0 unless $server_was_stopped; + + if ($fw_rule_created) { + &write_test_file("[-] new fw rule created.\n", $current_test_file); + $rv = 0; + } else { + &write_test_file("[+] new fw rule not created.\n", $current_test_file); + } + + unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], + $MATCH_ALL, $server_test_file)) { + $rv = 0; + } + + return $rv; +} + +sub null_port() { + my $test_hr = shift; + + my $rv = 1; + my $server_was_stopped = 0; + my $fw_rule_created = 0; + my $fw_rule_removed = 0; + + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A tcp/ \ + # -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key \ + # --verbose --verbose + # + my $spa_pkt = + '94nu7hvq6V/3A27GzjHwfPnPCQfs44ySlraIFYHOAqy5YqjkrBS67nH35tX55N1BrYZ' . + '07zvcT03keUhLE1Uo7Wme1nE7BfTOG5stmIK1UQI85sL52//lDHu+xCqNcL7GUKbVRz' . + 'ekw+EUscVvUkrsRcVtSvOm+fCNo'; + + my @packets = ( + { + 'proto' => 'udp', + 'port' => $default_spa_port, + 'dst_ip' => $loopback_ip, + 'data' => $spa_pkt, + }, + ); + + ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) + = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); + + $rv = 0 unless $server_was_stopped; + + if ($fw_rule_created) { + &write_test_file("[-] new fw rule created.\n", $current_test_file); + $rv = 0; + } else { + &write_test_file("[+] new fw rule not created.\n", $current_test_file); + } + + unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], + $MATCH_ALL, $server_test_file)) { + $rv = 0; + } + + return $rv; +} + +sub negative_port() { + my $test_hr = shift; + + my $rv = 1; + my $server_was_stopped = 0; + my $fw_rule_created = 0; + my $fw_rule_removed = 0; + + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A \ + # tcp/-33 -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key \ + # --verbose --verbose + # + my $spa_pkt = + '/weoc+pEuQknZo8ImWTQBB+/PwSJ2/TcrmFoSkxpRXX4+jlUxoJakHrioxh8rhLmAD9' . + '8E4lMnq+EbM2XYdhs2alpZ5bovAFojMsYRWwr/BvRO4Um4Fmo9z9sY3DR477TXNYXBR' . + 'iGXWxSL4u+AWSSePK3qiiYoRQVw'; + + my @packets = ( + { + 'proto' => 'udp', + 'port' => $default_spa_port, + 'dst_ip' => $loopback_ip, + 'data' => $spa_pkt, + }, + ); + + ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) + = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); + + $rv = 0 unless $server_was_stopped; + + if ($fw_rule_created) { + &write_test_file("[-] new fw rule created.\n", $current_test_file); + $rv = 0; + } else { + &write_test_file("[+] new fw rule not created.\n", $current_test_file); + } + + unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], + $MATCH_ALL, $server_test_file)) { + $rv = 0; + } + + return $rv; +} + +sub overly_long_proto() { + my $test_hr = shift; + + my $rv = 1; + my $server_was_stopped = 0; + my $fw_rule_created = 0; + my $fw_rule_removed = 0; + + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A \ + # "tcp`perl -e '{print "A"x"28"}'`/1" -a 127.0.0.2 -D 127.0.0.1 \ + # --get-key local_spa.key --verbose --verbose + # + # This problem was found by Fernando Arnaboldi of IOActive and exploits + # a buffer overflow in the fwknopd servers prior to 2.0.3 from + # authenticated clients. + # + my $spa_pkt = + '/im5MiJQmOdzqrdWXv+AjEtAm/HsLrdaTFcSw3ZskqpGOdDIrSCz3VXbFfv7qDkc5Y4' . + 'q/k1mRXl9SGzpug87U5dZSyCdAr30z7/2kUFEPTGOQBi/x+L1t1pvdkm4xg13t09ldm' . + '5OD8KiV6qzqLOvN4ULJjvvJJWBZ9qvo/f2Q9Wf67g2KHiwS6EeCINAuMoUw/mNRQMa4' . + 'oGnOXu3/DeWHJAwtSeh7EAr4'; + + my @packets = ( + { + 'proto' => 'udp', + 'port' => $default_spa_port, + 'dst_ip' => $loopback_ip, + 'data' => $spa_pkt, + }, + ); + + ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) + = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); + + $rv = 0 unless $server_was_stopped; + + if ($fw_rule_created) { + &write_test_file("[-] new fw rule created.\n", $current_test_file); + $rv = 0; + } else { + &write_test_file("[+] new fw rule not created.\n", $current_test_file); + } + + unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], + $MATCH_ALL, $server_test_file)) { + $rv = 0; + } + + return $rv; +} + +sub overly_long_port() { + my $test_hr = shift; + + my $rv = 1; + my $server_was_stopped = 0; + my $fw_rule_created = 0; + my $fw_rule_removed = 0; + + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A \ + # "tcp/`perl -e '{print "1"x"40"}'`" -a 127.0.0.2 -D 127.0.0.1 \ + # --get-key local_spa.key --verbose --verbose + # + # This problem was found by Fernando Arnaboldi of IOActive and exploits + # a buffer overflow in the fwknopd servers prior to 2.0.3 from + # authenticated clients. + # + my $spa_pkt = + '+JzxeTGlc6lwwzbJSrYChKx8bonWBIPajwGfEtGOaoglcMLbTY/GGXo/nxqiN1LykFS' . + 'lDFXgrkyx2emJ7NGzYqQPUYZxLdZRocR9aRIptvXLLIPBcIpJASi/TUiJlw7CDFMcj0' . + 'ptSBJJUZi0tozpKHETp3AgqfzyOy5FNs38aZsV5/sDl3Pt+kF7fTZJ+YLbmYY4yCUz2' . + 'ZUYoCaJ7X78ULyJTi5eT7nug'; + + my @packets = ( + { + 'proto' => 'udp', + 'port' => $default_spa_port, + 'dst_ip' => $loopback_ip, + 'data' => $spa_pkt, + }, + ); + + ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) + = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); + + $rv = 0 unless $server_was_stopped; + + if ($fw_rule_created) { + &write_test_file("[-] new fw rule created.\n", $current_test_file); + $rv = 0; + } else { + &write_test_file("[+] new fw rule not created.\n", $current_test_file); + } + + unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], + $MATCH_ALL, $server_test_file)) { + $rv = 0; + } + + return $rv; +} + sub altered_base64_spa_data() { my $test_hr = shift; @@ -2357,7 +2766,7 @@ sub server_ignore_small_packets() { } sub client_server_interaction() { - my ($test_hr, $pkts_hr, $spa_client_flag, $fw_rules_flag) = @_; + my ($test_hr, $pkts_hr, $spa_client_flag) = @_; my $rv = 1; my $server_was_stopped = 1; From f4c16bc47fc24a96b63105556b62d61c1ba7d799 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sat, 25 Aug 2012 23:08:55 -0400 Subject: [PATCH 03/14] [server] Stronger IP validation based on a bug found by Fernando Arnaboldi from IOActive This commit fixes a condition in which the server did not properly validate allow IP addresses from malicious authenticated clients. This has been fixed with stronger allow IP validation. --- CREDITS | 2 ++ ChangeLog | 4 +++ lib/fko_message.c | 16 ++++++++--- test/test-fwknop.pl | 67 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 4 deletions(-) diff --git a/CREDITS b/CREDITS index 71f0fa84..14753ea4 100644 --- a/CREDITS +++ b/CREDITS @@ -58,3 +58,5 @@ Fernando Arnaboldi (IOActive) - Found important buffer overflow conditions for authenticated SPA clients in the fwknopd server (pre-2.0.3). These findings enabled fixes to be 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). diff --git a/ChangeLog b/ChangeLog index 00f3a372..39c5f0b7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,10 @@ fwknop-2.0.3 (08//2012): both the fwknopd server code along with libfko now perform stronger input validation of access request data. These vulnerabilities affect pre-2.0.3 fwknop releases. + - [server] Fernando Arnaboldi from IOActive found a condition in which + the server did not properly validate allow IP addresses from malicious + authenticated clients. This has been fixed with stronger allow IP + validation. - [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/lib/fko_message.c b/lib/fko_message.c index 9148c2d3..3228dfad 100644 --- a/lib/fko_message.c +++ b/lib/fko_message.c @@ -266,23 +266,31 @@ int got_allow_ip(const char *msg) { const char *ndx = msg; - int dot_cnt = 0; + int dot_ctr = 0, char_ctr = 0; int res = FKO_SUCCESS; while(*ndx != ',' && *ndx != '\0') { + char_ctr++; + if(char_ctr >= MAX_IPV4_STR_LEN) + { + res = FKO_ERROR_INVALID_ALLOW_IP; + break; + } if(*ndx == '.') - dot_cnt++; + dot_ctr++; else if(isdigit(*ndx) == 0) { res = FKO_ERROR_INVALID_ALLOW_IP; break; } - ndx++; } - if(dot_cnt != 3) + if (char_ctr < MIN_IPV4_STR_LEN) + res = FKO_ERROR_INVALID_ALLOW_IP; + + if(dot_ctr != 3) res = FKO_ERROR_INVALID_ALLOW_IP; return(res); diff --git a/test/test-fwknop.pl b/test/test-fwknop.pl index 2f78bb44..2b011bc7 100755 --- a/test/test-fwknop.pl +++ b/test/test-fwknop.pl @@ -1238,6 +1238,17 @@ my @tests = ( "-d $default_digest_file -p $default_pid_file $intf_str", 'fatal' => $NO }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'FUZZING', + 'detail' => 'overly long IP value', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&overly_long_ip, + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'fatal' => $NO + }, { 'category' => 'Rijndael SPA', 'subcategory' => 'FUZZING', @@ -2413,6 +2424,62 @@ sub negative_port() { return $rv; } +sub overly_long_ip() { + my $test_hr = shift; + + my $rv = 1; + my $server_was_stopped = 0; + my $fw_rule_created = 0; + my $fw_rule_removed = 0; + + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A tcp/22 \ + # -a `perl -e '{print "1"x"136"}'`.0.0.1 -D 127.0.0.1 \ + # --get-key local_spa.key --verbose --verbose + # + # This problem was found by Fernando Arnaboldi of IOActive and exploits + # a condition in which pre-2.0.3 fwknopd servers fail to properly validate + # allow IP addresses from malicious authenticated clients. + # + my $spa_pkt = + '93f2rhsXLmBoPicWvYTqrbp+6lNqvWDc8dzmX2s3settwjBGRAXm33TB9agibEphrBu' . + '3d+7DEsivZLDS6Kz0JwdjX7t0J9c8es+DVNjlLnPtVNcxhs+2kUzimNrgysIXQRJ+GF' . + 'GbhdxiXCqdy1vWxWpdoaZmY/CeGIkpoFJFPbJhCRLLX25UMvMF2wXj02MpI4d3t1/6W' . + 'DM3taM3kZsiFv6HxFjAhIEuQ1oAg2OgRGXkDmT3jDNZMHUm0d4Ahm9LonG7RbOxq/B0' . + 'qUvY8lkymbwvjelVok7Lvlc06cRhN4zm32D4V05g0vQS3PlX9C+mgph9DeAPVX+D8iZ' . + '8lGrxcPSfbCOW61k0MP+q1EhLZkc1qAm5g2+2cLNZcoBNEdh3yj8OTPZJyBVw'; + + my @packets = ( + { + 'proto' => 'udp', + 'port' => $default_spa_port, + 'dst_ip' => $loopback_ip, + 'data' => $spa_pkt, + }, + ); + + ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) + = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); + + $rv = 0 unless $server_was_stopped; + + if ($fw_rule_created) { + &write_test_file("[-] new fw rule created.\n", $current_test_file); + $rv = 0; + } else { + &write_test_file("[+] new fw rule not created.\n", $current_test_file); + } + + unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], + $MATCH_ALL, $server_test_file)) { + $rv = 0; + } + + return $rv; +} + sub overly_long_proto() { my $test_hr = shift; From 557cd6615b9cab21a9208390f5af070c66fd257d Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sun, 26 Aug 2012 15:46:54 -0400 Subject: [PATCH 04/14] consolidatd fuzzing functions within a single 'fuzzer' function --- test/test-fwknop.pl | 449 ++++++++++++++++---------------------------- 1 file changed, 164 insertions(+), 285 deletions(-) diff --git a/test/test-fwknop.pl b/test/test-fwknop.pl index 2b011bc7..5cad5ab4 100755 --- a/test/test-fwknop.pl +++ b/test/test-fwknop.pl @@ -1221,7 +1221,24 @@ my @tests = ( 'subcategory' => 'FUZZING', 'detail' => 'overly long port value', 'err_msg' => 'server crashed or did not detect error condition', - 'function' => \&overly_long_port, + 'function' => \&fuzzer, + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A \ + # "tcp/`perl -e '{print "1"x"40"}'`" -a 127.0.0.2 -D 127.0.0.1 \ + # --get-key local_spa.key --verbose --verbose + # + # This problem was found by Fernando Arnaboldi of IOActive and exploits + # a buffer overflow in the fwknopd servers prior to 2.0.3 from + # authenticated clients. + # + 'fuzzing_pkt' => + '+JzxeTGlc6lwwzbJSrYChKx8bonWBIPajwGfEtGOaoglcMLbTY/GGXo/nxqiN1LykFS' . + 'lDFXgrkyx2emJ7NGzYqQPUYZxLdZRocR9aRIptvXLLIPBcIpJASi/TUiJlw7CDFMcj0' . + 'ptSBJJUZi0tozpKHETp3AgqfzyOy5FNs38aZsV5/sDl3Pt+kF7fTZJ+YLbmYY4yCUz2' . + 'ZUYoCaJ7X78ULyJTi5eT7nug', + 'server_positive_output_matches' => [qr/Args\scontain\sinvalid\sdata/], 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . "-d $default_digest_file -p $default_pid_file $intf_str", @@ -1232,7 +1249,24 @@ my @tests = ( 'subcategory' => 'FUZZING', 'detail' => 'overly long proto value', 'err_msg' => 'server crashed or did not detect error condition', - 'function' => \&overly_long_proto, + 'function' => \&fuzzer, + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A \ + # "tcp`perl -e '{print "A"x"28"}'`/1" -a 127.0.0.2 -D 127.0.0.1 \ + # --get-key local_spa.key --verbose --verbose + # + # This problem was found by Fernando Arnaboldi of IOActive and exploits + # a buffer overflow in the fwknopd servers prior to 2.0.3 from + # authenticated clients. + # + 'fuzzing_pkt' => + '/im5MiJQmOdzqrdWXv+AjEtAm/HsLrdaTFcSw3ZskqpGOdDIrSCz3VXbFfv7qDkc5Y4' . + 'q/k1mRXl9SGzpug87U5dZSyCdAr30z7/2kUFEPTGOQBi/x+L1t1pvdkm4xg13t09ldm' . + '5OD8KiV6qzqLOvN4ULJjvvJJWBZ9qvo/f2Q9Wf67g2KHiwS6EeCINAuMoUw/mNRQMa4' . + 'oGnOXu3/DeWHJAwtSeh7EAr4', + 'server_positive_output_matches' => [qr/Args\scontain\sinvalid\sdata/], 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . "-d $default_digest_file -p $default_pid_file $intf_str", @@ -1243,7 +1277,26 @@ my @tests = ( 'subcategory' => 'FUZZING', 'detail' => 'overly long IP value', 'err_msg' => 'server crashed or did not detect error condition', - 'function' => \&overly_long_ip, + 'function' => \&fuzzer, + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A tcp/22 \ + # -a `perl -e '{print "1"x"136"}'`.0.0.1 -D 127.0.0.1 \ + # --get-key local_spa.key --verbose --verbose + # + # This problem was found by Fernando Arnaboldi of IOActive and exploits + # a condition in which pre-2.0.3 fwknopd servers fail to properly validate + # allow IP addresses from malicious authenticated clients. + # + 'fuzzing_pkt' => + '93f2rhsXLmBoPicWvYTqrbp+6lNqvWDc8dzmX2s3settwjBGRAXm33TB9agibEphrBu' . + '3d+7DEsivZLDS6Kz0JwdjX7t0J9c8es+DVNjlLnPtVNcxhs+2kUzimNrgysIXQRJ+GF' . + 'GbhdxiXCqdy1vWxWpdoaZmY/CeGIkpoFJFPbJhCRLLX25UMvMF2wXj02MpI4d3t1/6W' . + 'DM3taM3kZsiFv6HxFjAhIEuQ1oAg2OgRGXkDmT3jDNZMHUm0d4Ahm9LonG7RbOxq/B0' . + 'qUvY8lkymbwvjelVok7Lvlc06cRhN4zm32D4V05g0vQS3PlX9C+mgph9DeAPVX+D8iZ' . + '8lGrxcPSfbCOW61k0MP+q1EhLZkc1qAm5g2+2cLNZcoBNEdh3yj8OTPZJyBVw', + 'server_positive_output_matches' => [qr/Args\scontain\sinvalid\sdata/], 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . "-d $default_digest_file -p $default_pid_file $intf_str", @@ -1254,7 +1307,19 @@ my @tests = ( 'subcategory' => 'FUZZING', 'detail' => 'negative port value', 'err_msg' => 'server crashed or did not detect error condition', - 'function' => \&negative_port, + 'function' => \&fuzzer, + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A \ + # tcp/-33 -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key \ + # --verbose --verbose + # + 'fuzzing_pkt' => + '/weoc+pEuQknZo8ImWTQBB+/PwSJ2/TcrmFoSkxpRXX4+jlUxoJakHrioxh8rhLmAD9' . + '8E4lMnq+EbM2XYdhs2alpZ5bovAFojMsYRWwr/BvRO4Um4Fmo9z9sY3DR477TXNYXBR' . + 'iGXWxSL4u+AWSSePK3qiiYoRQVw', + 'server_positive_output_matches' => [qr/Args\scontain\sinvalid\sdata/], 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . "-d $default_digest_file -p $default_pid_file $intf_str", @@ -1265,18 +1330,105 @@ my @tests = ( 'subcategory' => 'FUZZING', 'detail' => 'null port value', 'err_msg' => 'server crashed or did not detect error condition', - 'function' => \&null_port, + 'function' => \&fuzzer, + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A tcp/ \ + # -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key \ + # --verbose --verbose + # + 'fuzzing_pkt' => + '94nu7hvq6V/3A27GzjHwfPnPCQfs44ySlraIFYHOAqy5YqjkrBS67nH35tX55N1BrYZ' . + '07zvcT03keUhLE1Uo7Wme1nE7BfTOG5stmIK1UQI85sL52//lDHu+xCqNcL7GUKbVRz' . + 'ekw+EUscVvUkrsRcVtSvOm+fCNo', + 'server_positive_output_matches' => [qr/Args\scontain\sinvalid\sdata/], 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . "-d $default_digest_file -p $default_pid_file $intf_str", 'fatal' => $NO }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'FUZZING', + 'detail' => 'long FKO protocol value (enc mode trigger)', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&fuzzer, + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A tcp/22 \ + # -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key --verbose --verbose + # + # This problem was found by Fernando Arnaboldi of IOActive and is designed + # to have fwknopd look for a mode decryption mode for a long Rijndael- + # encrypted SPA packet + # + 'fuzzing_pkt' => + '/ewH/k1XsDX+VQ8NlNvCZ4P2QOl/4IpJYXkq4TtAe3899OtApXJiTtPCuYW70XPuxge' . + 'MtFjc4UfslK/r9v+FYfyd3fIIHCz0Q0M4+nM3agTLmJj8nOxk6ZeBj82SDQWhHAxGdJ' . + 'IQALPve0ug4cuGxS3b4M+2Q/Av9i2tU3Lzlogw3sY0tk6wGf4zZk4UsviVXYpINniGT' . + 'RhYSIQ1dfdkng7hKiHMDaObYY1GFp4nxEt/QjasAwvE+7/iFyoKN+IRpGG4v4hGEPh2' . + 'vTDqmvfRuIHtgFD7NxZjt+m/jjcu0gkdWEoD4fenwGU35FlvchyM2AiAEw7yRzSABfn' . + 'R9d3sYZGMtyASw2O1vSluwIxUUnDop3gxEIhJEj8h+01pA3K+klSpALeY9EZgHqYC7E' . + 'ETuPS6dZ3764nWohtCY67JvNUX7TtNDNc2qrhrapdRP17+PT2Vh4s9m38V3WwVWC3uH' . + 'X/klLZcHIt+aRDV+uekw9GOKSgwFL2ekPpr3gXxigc3zrxel5hcsqLOpVUa4CP/0HkG' . + 'F0NPQvOT3ZvpeIJnirKP1ZX9gDFinqhuzL7oqktW61e1iwe7KZEdrZV0k2KZwyb8qU5' . + 'rPAEnw', + 'server_positive_output_matches' => [qr/No\sstanza\sencryption\smode\smatch/], + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'fatal' => $NO + }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'FUZZING', + 'detail' => 'long FKO protocol value (Rijndael trigger)', + 'err_msg' => 'server crashed or did not detect error condition', + 'function' => \&fuzzer, + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A tcp/22 \ + # -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key --verbose --verbose + # + # This problem was found by Fernando Arnaboldi of IOActive and is designed + # to have fwknopd look for a mode decryption mode for a long Rijndael- + # encrypted SPA packet + # + 'fuzzing_pkt' => + '+YQNu4BFgiNeu8HeiBiNKriqCFSseALt9vJaKzkzK/OF4pjkJcvhGEOi7fEVXqn3VIdlGR' . + 'DmBul2I7H3z18U9E97bWGgT9NexKgEPCuekL18ZEPf5xR3JleNsNWatqYgAOkgN8ZWE69Q' . + 'qQUYYhxTvJHS6R+5JqFKB3A44hMXoICdYNkn9MAktHxk3PbbpQ+nA+jESwVCra2doAiLiM' . + 'ucvGIZZiTv0Mc1blFYIE2zqZ/C7ct1V+ukwSkUv0r87eA7uJhmlpThRsL0dN6iekJ6i87B' . + 'tE8QyuOXzOMftI11SUn/LwqD4RMdR21rvLrzR6ZB5eUX2UBpODyzX6n+PJJkTWCuFVT4z1' . + 'MKY', + 'server_positive_output_matches' => [qr/Args\scontain\sinvalid\sdata/], + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'fatal' => $NO + }, + { 'category' => 'Rijndael SPA', 'subcategory' => 'FUZZING', 'detail' => 'null proto value', 'err_msg' => 'server crashed or did not detect error condition', - 'function' => \&null_proto, + 'function' => \&fuzzer, + ### this packet was generated with a modified fwknop client via the + ### following command line: + # + # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A /22 \ + # -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key \ + # --verbose --verbose + # + 'fuzzing_pkt' => + '/JT14qxh9P4iy+CuUZahThaQjoEuL2zd46a+jL6sTrBZJSa6faUX4dH5fte/4ZJv+9f' . + 'd/diWYKAUvdQ4DydPGlR7mwQa2W+obKpqrsTBz7D4054z6ATAOGpCtifakEVl1XRc2+' . + 'hW04WpY8mdUNu9i+PrfPr7/KxqU', + 'server_positive_output_matches' => [qr/Args\scontain\sinvalid\sdata/], 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . "$fwknopdCmd -c $cf{'disable_aging'} -a $cf{'def_access'} " . "-d $default_digest_file -p $default_pid_file $intf_str", @@ -2277,7 +2429,7 @@ sub altered_non_base64_spa_data() { return $rv; } -sub null_proto() { +sub fuzzer() { my $test_hr = shift; my $rv = 1; @@ -2285,24 +2437,12 @@ sub null_proto() { my $fw_rule_created = 0; my $fw_rule_removed = 0; - ### this packet was generated with a modified fwknop client via the - ### following command line: - # - # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A /22 \ - # -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key \ - # --verbose --verbose - # - my $spa_pkt = - '/JT14qxh9P4iy+CuUZahThaQjoEuL2zd46a+jL6sTrBZJSa6faUX4dH5fte/4ZJv+9f' . - 'd/diWYKAUvdQ4DydPGlR7mwQa2W+obKpqrsTBz7D4054z6ATAOGpCtifakEVl1XRc2+' . - 'hW04WpY8mdUNu9i+PrfPr7/KxqU'; - my @packets = ( { 'proto' => 'udp', 'port' => $default_spa_port, 'dst_ip' => $loopback_ip, - 'data' => $spa_pkt, + 'data' => $test_hr->{'fuzzing_pkt'}, }, ); @@ -2318,271 +2458,10 @@ sub null_proto() { &write_test_file("[+] new fw rule not created.\n", $current_test_file); } - unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], - $MATCH_ALL, $server_test_file)) { - $rv = 0; - } - - return $rv; -} - -sub null_port() { - my $test_hr = shift; - - my $rv = 1; - my $server_was_stopped = 0; - my $fw_rule_created = 0; - my $fw_rule_removed = 0; - - ### this packet was generated with a modified fwknop client via the - ### following command line: - # - # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A tcp/ \ - # -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key \ - # --verbose --verbose - # - my $spa_pkt = - '94nu7hvq6V/3A27GzjHwfPnPCQfs44ySlraIFYHOAqy5YqjkrBS67nH35tX55N1BrYZ' . - '07zvcT03keUhLE1Uo7Wme1nE7BfTOG5stmIK1UQI85sL52//lDHu+xCqNcL7GUKbVRz' . - 'ekw+EUscVvUkrsRcVtSvOm+fCNo'; - - my @packets = ( - { - 'proto' => 'udp', - 'port' => $default_spa_port, - 'dst_ip' => $loopback_ip, - 'data' => $spa_pkt, - }, - ); - - ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) - = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); - - $rv = 0 unless $server_was_stopped; - - if ($fw_rule_created) { - &write_test_file("[-] new fw rule created.\n", $current_test_file); - $rv = 0; - } else { - &write_test_file("[+] new fw rule not created.\n", $current_test_file); - } - - unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], - $MATCH_ALL, $server_test_file)) { - $rv = 0; - } - - return $rv; -} - -sub negative_port() { - my $test_hr = shift; - - my $rv = 1; - my $server_was_stopped = 0; - my $fw_rule_created = 0; - my $fw_rule_removed = 0; - - ### this packet was generated with a modified fwknop client via the - ### following command line: - # - # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A \ - # tcp/-33 -a 127.0.0.2 -D 127.0.0.1 --get-key local_spa.key \ - # --verbose --verbose - # - my $spa_pkt = - '/weoc+pEuQknZo8ImWTQBB+/PwSJ2/TcrmFoSkxpRXX4+jlUxoJakHrioxh8rhLmAD9' . - '8E4lMnq+EbM2XYdhs2alpZ5bovAFojMsYRWwr/BvRO4Um4Fmo9z9sY3DR477TXNYXBR' . - 'iGXWxSL4u+AWSSePK3qiiYoRQVw'; - - my @packets = ( - { - 'proto' => 'udp', - 'port' => $default_spa_port, - 'dst_ip' => $loopback_ip, - 'data' => $spa_pkt, - }, - ); - - ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) - = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); - - $rv = 0 unless $server_was_stopped; - - if ($fw_rule_created) { - &write_test_file("[-] new fw rule created.\n", $current_test_file); - $rv = 0; - } else { - &write_test_file("[+] new fw rule not created.\n", $current_test_file); - } - - unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], - $MATCH_ALL, $server_test_file)) { - $rv = 0; - } - - return $rv; -} - -sub overly_long_ip() { - my $test_hr = shift; - - my $rv = 1; - my $server_was_stopped = 0; - my $fw_rule_created = 0; - my $fw_rule_removed = 0; - - ### this packet was generated with a modified fwknop client via the - ### following command line: - # - # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A tcp/22 \ - # -a `perl -e '{print "1"x"136"}'`.0.0.1 -D 127.0.0.1 \ - # --get-key local_spa.key --verbose --verbose - # - # This problem was found by Fernando Arnaboldi of IOActive and exploits - # a condition in which pre-2.0.3 fwknopd servers fail to properly validate - # allow IP addresses from malicious authenticated clients. - # - my $spa_pkt = - '93f2rhsXLmBoPicWvYTqrbp+6lNqvWDc8dzmX2s3settwjBGRAXm33TB9agibEphrBu' . - '3d+7DEsivZLDS6Kz0JwdjX7t0J9c8es+DVNjlLnPtVNcxhs+2kUzimNrgysIXQRJ+GF' . - 'GbhdxiXCqdy1vWxWpdoaZmY/CeGIkpoFJFPbJhCRLLX25UMvMF2wXj02MpI4d3t1/6W' . - 'DM3taM3kZsiFv6HxFjAhIEuQ1oAg2OgRGXkDmT3jDNZMHUm0d4Ahm9LonG7RbOxq/B0' . - 'qUvY8lkymbwvjelVok7Lvlc06cRhN4zm32D4V05g0vQS3PlX9C+mgph9DeAPVX+D8iZ' . - '8lGrxcPSfbCOW61k0MP+q1EhLZkc1qAm5g2+2cLNZcoBNEdh3yj8OTPZJyBVw'; - - my @packets = ( - { - 'proto' => 'udp', - 'port' => $default_spa_port, - 'dst_ip' => $loopback_ip, - 'data' => $spa_pkt, - }, - ); - - ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) - = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); - - $rv = 0 unless $server_was_stopped; - - if ($fw_rule_created) { - &write_test_file("[-] new fw rule created.\n", $current_test_file); - $rv = 0; - } else { - &write_test_file("[+] new fw rule not created.\n", $current_test_file); - } - - unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], - $MATCH_ALL, $server_test_file)) { - $rv = 0; - } - - return $rv; -} - -sub overly_long_proto() { - my $test_hr = shift; - - my $rv = 1; - my $server_was_stopped = 0; - my $fw_rule_created = 0; - my $fw_rule_removed = 0; - - ### this packet was generated with a modified fwknop client via the - ### following command line: - # - # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A \ - # "tcp`perl -e '{print "A"x"28"}'`/1" -a 127.0.0.2 -D 127.0.0.1 \ - # --get-key local_spa.key --verbose --verbose - # - # This problem was found by Fernando Arnaboldi of IOActive and exploits - # a buffer overflow in the fwknopd servers prior to 2.0.3 from - # authenticated clients. - # - my $spa_pkt = - '/im5MiJQmOdzqrdWXv+AjEtAm/HsLrdaTFcSw3ZskqpGOdDIrSCz3VXbFfv7qDkc5Y4' . - 'q/k1mRXl9SGzpug87U5dZSyCdAr30z7/2kUFEPTGOQBi/x+L1t1pvdkm4xg13t09ldm' . - '5OD8KiV6qzqLOvN4ULJjvvJJWBZ9qvo/f2Q9Wf67g2KHiwS6EeCINAuMoUw/mNRQMa4' . - 'oGnOXu3/DeWHJAwtSeh7EAr4'; - - my @packets = ( - { - 'proto' => 'udp', - 'port' => $default_spa_port, - 'dst_ip' => $loopback_ip, - 'data' => $spa_pkt, - }, - ); - - ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) - = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); - - $rv = 0 unless $server_was_stopped; - - if ($fw_rule_created) { - &write_test_file("[-] new fw rule created.\n", $current_test_file); - $rv = 0; - } else { - &write_test_file("[+] new fw rule not created.\n", $current_test_file); - } - - unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], - $MATCH_ALL, $server_test_file)) { - $rv = 0; - } - - return $rv; -} - -sub overly_long_port() { - my $test_hr = shift; - - my $rv = 1; - my $server_was_stopped = 0; - my $fw_rule_created = 0; - my $fw_rule_removed = 0; - - ### this packet was generated with a modified fwknop client via the - ### following command line: - # - # LD_LIBRARY_PATH=../lib/.libs ../client/.libs/fwknop -A \ - # "tcp/`perl -e '{print "1"x"40"}'`" -a 127.0.0.2 -D 127.0.0.1 \ - # --get-key local_spa.key --verbose --verbose - # - # This problem was found by Fernando Arnaboldi of IOActive and exploits - # a buffer overflow in the fwknopd servers prior to 2.0.3 from - # authenticated clients. - # - my $spa_pkt = - '+JzxeTGlc6lwwzbJSrYChKx8bonWBIPajwGfEtGOaoglcMLbTY/GGXo/nxqiN1LykFS' . - 'lDFXgrkyx2emJ7NGzYqQPUYZxLdZRocR9aRIptvXLLIPBcIpJASi/TUiJlw7CDFMcj0' . - 'ptSBJJUZi0tozpKHETp3AgqfzyOy5FNs38aZsV5/sDl3Pt+kF7fTZJ+YLbmYY4yCUz2' . - 'ZUYoCaJ7X78ULyJTi5eT7nug'; - - my @packets = ( - { - 'proto' => 'udp', - 'port' => $default_spa_port, - 'dst_ip' => $loopback_ip, - 'data' => $spa_pkt, - }, - ); - - ($rv, $server_was_stopped, $fw_rule_created, $fw_rule_removed) - = &client_server_interaction($test_hr, \@packets, $USE_PREDEF_PKTS); - - $rv = 0 unless $server_was_stopped; - - if ($fw_rule_created) { - &write_test_file("[-] new fw rule created.\n", $current_test_file); - $rv = 0; - } else { - &write_test_file("[+] new fw rule not created.\n", $current_test_file); - } - - unless (&file_find_regex([qr/Args\scontain\sinvalid\sdata/], - $MATCH_ALL, $server_test_file)) { - $rv = 0; + if ($test_hr->{'server_positive_output_matches'}) { + $rv = 0 unless &file_find_regex( + $test_hr->{'server_positive_output_matches'}, + $MATCH_ALL, $server_test_file); } return $rv; From e8386dbe6c959365da5c08396e09c27901faed56 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sun, 26 Aug 2012 15:47:24 -0400 Subject: [PATCH 05/14] added encryption mode flags for each access stanza --- server/access.c | 17 ++++++++++++----- server/fwknopd_common.h | 3 +++ server/incoming_spa.c | 18 +++++------------- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/server/access.c b/server/access.c index 91feb0b7..d1057f89 100644 --- a/server/access.c +++ b/server/access.c @@ -763,8 +763,9 @@ set_acc_defaults(fko_srv_options_t *opts) static int acc_data_is_valid(const acc_stanza_t *acc) { - if((acc->key == NULL || !strlen(acc->key)) + if(((acc->key == NULL || !strlen(acc->key)) && (acc->gpg_decrypt_pw == NULL || !strlen(acc->gpg_decrypt_pw))) + || (acc->use_rijndael == 0 && acc->use_gpg == 0 && acc->gpg_allow_no_pw == 0)) { fprintf(stderr, "[*] No keys found for access stanza source: '%s'\n", acc->source @@ -907,6 +908,7 @@ parse_access_file(fko_srv_options_t *opts) clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); } add_acc_string(&(curr_acc->key), val); + add_acc_bool(&(curr_acc->use_rijndael), "Y"); } else if(CONF_VAR_IS(var, "FW_ACCESS_TIMEOUT")) { @@ -972,13 +974,18 @@ parse_access_file(fko_srv_options_t *opts) clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE); } add_acc_string(&(curr_acc->gpg_decrypt_pw), val); + add_acc_bool(&(curr_acc->use_gpg), "Y"); } else if(CONF_VAR_IS(var, "GPG_ALLOW_NO_PW")) { - if(curr_acc->gpg_decrypt_pw != NULL && curr_acc->gpg_decrypt_pw[0] != '\0') - free(curr_acc->gpg_decrypt_pw); - - add_acc_string(&(curr_acc->gpg_decrypt_pw), ""); + add_acc_bool(&(curr_acc->gpg_allow_no_pw), val); + if(curr_acc->gpg_allow_no_pw == 1) + { + add_acc_bool(&(curr_acc->use_gpg), "Y"); + if(curr_acc->gpg_decrypt_pw != NULL && curr_acc->gpg_decrypt_pw[0] != '\0') + free(curr_acc->gpg_decrypt_pw); + add_acc_string(&(curr_acc->gpg_decrypt_pw), ""); + } } else if(CONF_VAR_IS(var, "GPG_REQUIRE_SIG")) { diff --git a/server/fwknopd_common.h b/server/fwknopd_common.h index a6f0655c..cd4d42e9 100644 --- a/server/fwknopd_common.h +++ b/server/fwknopd_common.h @@ -273,6 +273,7 @@ typedef struct acc_stanza char *restrict_ports; acc_port_list_t *rport_list; char *key; + unsigned char use_rijndael; int fw_access_timeout; unsigned char enable_cmd_exec; char *cmd_exec_user; @@ -284,6 +285,8 @@ typedef struct acc_stanza char *gpg_decrypt_pw; unsigned char gpg_require_sig; unsigned char gpg_ignore_sig_error; + unsigned char use_gpg; + unsigned char gpg_allow_no_pw; char *gpg_remote_id; acc_string_list_t *gpg_remote_id_list; time_t access_expire_time; diff --git a/server/incoming_spa.c b/server/incoming_spa.c index a7be5f6e..67929c20 100644 --- a/server/incoming_spa.c +++ b/server/incoming_spa.c @@ -362,7 +362,7 @@ incoming_spa(fko_srv_options_t *opts) */ enc_type = fko_encryption_type((char *)spa_pkt->packet_data); - if(enc_type == FKO_ENCRYPTION_RIJNDAEL) + if(acc->use_rijndael && enc_type == FKO_ENCRYPTION_RIJNDAEL) { if(acc->key != NULL) res = fko_new_with_data(&ctx, (char *)spa_pkt->packet_data, acc->key); @@ -376,12 +376,12 @@ incoming_spa(fko_srv_options_t *opts) continue; } } - else if(enc_type == FKO_ENCRYPTION_GPG) + else if(acc->use_gpg && enc_type == FKO_ENCRYPTION_GPG) { /* For GPG we create the new context without decrypting on the fly * so we can set some GPG parameters first. */ - if(acc->gpg_decrypt_pw != NULL) + if(acc->gpg_decrypt_pw != NULL || acc->gpg_allow_no_pw) { res = fko_new_with_data(&ctx, (char *)spa_pkt->packet_data, NULL); if(res != FKO_SUCCESS) @@ -439,19 +439,11 @@ incoming_spa(fko_srv_options_t *opts) res = fko_decrypt_spa_data(ctx, acc->gpg_decrypt_pw); } - else - { - log_msg(LOG_ERR, - "(stanza #%d) No GPG_DECRYPT_PW for GPG encrypted messages, set GPG_ALLOW_NO_PW", - stanza_num - ); - acc = acc->next; - continue; - } } else { - log_msg(LOG_ERR, "(stanza #%d) Unable to determing encryption type. Got type=%i.", + log_msg(LOG_ERR, + "(stanza #%d) No stanza encryption mode match for encryption type: %i.", stanza_num, enc_type); acc = acc->next; continue; From c5b229c5c87657197b0c814ff22127d870b55753 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Mon, 27 Aug 2012 21:16:59 -0400 Subject: [PATCH 06/14] Added $DESTDIR prefix in uninstall-local and install-exec-hook to fix RPM builds --- Makefile.am | 43 +++++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/Makefile.am b/Makefile.am index 7306bbc5..1d83d5db 100644 --- a/Makefile.am +++ b/Makefile.am @@ -13,7 +13,7 @@ SUBDIRS = \ common \ $(CLIENT_DIR) \ $(SERVER_DIR) \ - doc + doc EXTRA_DIST = \ android/COPYING \ @@ -174,39 +174,38 @@ dist-hook: rm -f $(distdir)/server/fwknopd.8 uninstall-local: - if test -f $(sysconfdir)/fwknop/fwknopd.conf; then \ - rm -f $(sysconfdir)/fwknop/fwknopd.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; then \ + rm -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; \ fi - if test -f $(sysconfdir)/fwknop/fwknopd.conf.inst; then \ - rm -f $(sysconfdir)/fwknop/fwknopd.conf.inst; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf.inst; then \ + rm -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf.inst; \ fi - if test -f $(sysconfdir)/fwknop/access.conf; then \ - rm -f $(sysconfdir)/fwknop/access.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf; then \ + rm -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf; \ fi - if test -f $(sysconfdir)/fwknop/access.conf.inst; then \ - rm -f $(sysconfdir)/fwknop/access.conf.inst; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf.inst; then \ + rm -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf.inst; \ fi install-exec-hook: - if test -d $(sysconfdir)/fwknop; then \ - chmod 700 $(sysconfdir)/fwknop; \ + if test -d $(DESTDIR)$(sysconfdir)/fwknop; then \ + chmod 700 $(DESTDIR)$(sysconfdir)/fwknop; \ fi - if test -f $(sysconfdir)/fwknop/fwknopd.conf; then :; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; then :; \ else \ - if test -f $(sysconfdir)/fwknop/fwknopd.conf.inst; then \ - mv $(sysconfdir)/fwknop/fwknopd.conf.inst $(sysconfdir)/fwknop/fwknopd.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf.inst; then \ + mv $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf.inst $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; \ fi \ fi - if test -f $(sysconfdir)/fwknop/access.conf; then :; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf; then :; \ else \ - if test -f $(sysconfdir)/fwknop/access.conf.inst; then \ - mv $(sysconfdir)/fwknop/access.conf.inst $(sysconfdir)/fwknop/access.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf.inst; then \ + mv $(DESTDIR)$(sysconfdir)/fwknop/access.conf.inst $(DESTDIR)$(sysconfdir)/fwknop/access.conf; \ fi \ fi - if test -f $(sysconfdir)/fwknop/fwknopd.conf; then \ - chmod 600 $(sysconfdir)/fwknop/fwknopd.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; then \ + chmod 600 $(DESTDIR)$(sysconfdir)/fwknop/fwknopd.conf; \ fi - if test -f $(sysconfdir)/fwknop/access.conf; then \ - chmod 600 $(sysconfdir)/fwknop/access.conf; \ + if test -f $(DESTDIR)$(sysconfdir)/fwknop/access.conf; then \ + chmod 600 $(DESTDIR)$(sysconfdir)/fwknop/access.conf; \ fi - From 89dfa2c1fb06776646f99f722f21d47620f66695 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Mon, 27 Aug 2012 21:20:02 -0400 Subject: [PATCH 07/14] minor ChangeLog update for the RPM build change --- ChangeLog | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog b/ChangeLog index 39c5f0b7..056d1efe 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,4 +1,6 @@ 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. - [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 From 098ae417fe91aefe501e9268aacd228374d0906d Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Mon, 27 Aug 2012 22:30:27 -0400 Subject: [PATCH 08/14] migrated TODO tasks to the todo.org file --- TODO | 41 ----------------------------------------- todo.org | 10 ++++++++++ 2 files changed, 10 insertions(+), 41 deletions(-) delete mode 100644 TODO diff --git a/TODO b/TODO deleted file mode 100644 index d73d2413..00000000 --- a/TODO +++ /dev/null @@ -1,41 +0,0 @@ -To whom it may concern, this is -*- outline -*- mode. - -* The library - libfko: -** C-based test suite. -** Get it out there and get feedback. - -* The fwknop C client: -** Config file support (~/.fwknoprc ?). -** Server Auth support (maybe). - -* The fwknopd server: -** Sniffer support to acquire SPA packet data ala the fwknopd Perl server: -** SPA packet decryption: -*** Add support for ipfw and pf support eventually. -** Test on embedded platforms - especially OpenWRT on a Linksys router. - -* Nice to haves: -** Binary packages: -*** Redhat RPMS -*** Debian .deb -** Linux/Unix platform: -*** A GNOME or KDE GUI app for the client. -** Windows platform: -*** VB and/or C# class wrappers around libfko.dll. -*** A Windows GUI app that uses the dll or wrapper classes. -** Misc: -*** Python module wrapping libfko. -*** Ruby module wrapping libfko. -*** PHP module wrapping libfko. - - -Copyright 2009-2010 - Damien Stuart - -This file is free software; as a special exception the author gives -unlimited permission to copy and/or distribute it, with or without -modifications, as long as this notice is preserved. - -This file is distributed in the hope that it will be useful, but -WITHOUT ANY WARRANTY, to the extent permitted by law; without even the -implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR -PURPOSE. diff --git a/todo.org b/todo.org index f619582c..ee4d6ad3 100644 --- a/todo.org +++ b/todo.org @@ -50,3 +50,13 @@ ** [test suite] backwards compatibility tests The test suite should have the ability to test backwards compatibility between fwknop versions. +** For Linux/Unix - a GNOME or KDE GUI app for the fwknop client. + Although there is currently a functioning web proxy that can serve as a + UI via a browser, it would be nice to have native GNOME and KDE GUI + wrappers for the fwknop client. +** For Windows - VB and/or C# class wrappers around libfko.dll + Extend Windows support with VB and/or C# class wrappers around the + libfko.dll +** Ruby bindings to libfko + Perl and Python bindings already exist for libfko, so add Ruby to this list + as well. From 186a424353a2e795e69f399f079a901e7dc8f24b Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Tue, 28 Aug 2012 21:28:57 -0400 Subject: [PATCH 09/14] Added Ctrl-C and --disable-gpg notes --- todo.org | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/todo.org b/todo.org index ee4d6ad3..33b75e52 100644 --- a/todo.org +++ b/todo.org @@ -60,3 +60,10 @@ ** Ruby bindings to libfko Perl and Python bindings already exist for libfko, so add Ruby to this list as well. +** [client] Update to not send SPA packet if Ctrl-C is used + The client currently sends an SPA packet when an encryption key is + requested but the user tries to exit out with Ctrl-C. +** Add --disable-gpg arg to the autoconf configure script + There needs to be a way to easily disable libgpgme usage even if it is + installed - this could be done with a new --disablegpg argument to the + configure script. From a60f05ad44e824f6230b22f8976399340cb535dc Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Wed, 29 Aug 2012 22:21:43 -0400 Subject: [PATCH 10/14] 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 056d1efe..5e800b03 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 19ec13ec..9e04bcc9 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 97192b6c..3c518da5 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 5a60528b..1e5ee2fe 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 df618aec..672b8f35 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 4a2ae3dc..8690d216 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 d1057f89..280e7024 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 70c3e7c2..a728cdae 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 5a780b39..ec49d322 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 2b164e94..227d64c6 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, "#