From e2c0ac4821773eb335e36ad6cd35830b8d97c75a Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Mon, 3 Sep 2012 00:21:46 -0400 Subject: [PATCH] [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',