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',