diff --git a/ChangeLog b/ChangeLog index e6792586..b6498ded 100644 --- a/ChangeLog +++ b/ChangeLog @@ -17,6 +17,8 @@ fwknop-2.0.4 (09/20/2012): UDP with a spoofed source IP address. This is in addition to the original 'tcpraw' and 'icmp' protocols that also support a spoofed source IP. + - [libfko] Added validation of NAT access strings in the various NAT + modes. - [server] Bug fix to accept SPA packets over ICMP if the fwknop client is executed with '-P icmp' and the user has the required privileges. - Applied patch from Franck Joncourt to have the perl FKO module link diff --git a/Makefile.am b/Makefile.am index 6504b020..71944c68 100644 --- a/Makefile.am +++ b/Makefile.am @@ -151,6 +151,7 @@ EXTRA_DIST = \ test/conf/subnet_source_match_access.conf \ test/conf/local_nat_fwknopd.conf \ test/conf/disable_aging_fwknopd.conf \ + test/conf/disable_aging_nat_fwknopd.conf \ test/conf/fuzzing_source_access.conf \ test/conf/fuzzing_open_ports_access.conf \ test/conf/fuzzing_restrict_ports_access.conf \ diff --git a/lib/fko_decode.c b/lib/fko_decode.c index b1e36a67..574c80f0 100644 --- a/lib/fko_decode.c +++ b/lib/fko_decode.c @@ -342,6 +342,12 @@ fko_decode_spa_data(fko_ctx_t ctx) } b64_decode(tbuf, (unsigned char*)ctx->nat_access); + + if(validate_nat_access_msg(ctx->nat_access) != FKO_SUCCESS) + { + free(tbuf); + return(FKO_ERROR_INVALID_DATA); + } } /* Now look for a server_auth string. diff --git a/lib/fko_message.c b/lib/fko_message.c index d7361f94..7d8237bd 100644 --- a/lib/fko_message.c +++ b/lib/fko_message.c @@ -33,6 +33,83 @@ #include "fko_message.h" #include "fko.h" +static int +have_allow_ip(const char *msg) +{ + const char *ndx = msg; + char ip_str[MAX_IPV4_STR_LEN]; + int dot_ctr = 0, char_ctr = 0; + int res = FKO_SUCCESS; +#if HAVE_SYS_SOCKET_H + struct in_addr in; +#endif + + while(*ndx != ',' && *ndx != '\0') + { + ip_str[char_ctr] = *ndx; + char_ctr++; + if(char_ctr >= MAX_IPV4_STR_LEN) + { + res = FKO_ERROR_INVALID_ALLOW_IP; + break; + } + if(*ndx == '.') + dot_ctr++; + else if(isdigit(*ndx) == 0) + { + res = FKO_ERROR_INVALID_ALLOW_IP; + break; + } + ndx++; + } + + if(char_ctr < MAX_IPV4_STR_LEN) + ip_str[char_ctr] = '\0'; + else + res = FKO_ERROR_INVALID_ALLOW_IP; + + if ((res == FKO_SUCCESS) && (char_ctr < MIN_IPV4_STR_LEN)) + res = FKO_ERROR_INVALID_ALLOW_IP; + + if((res == FKO_SUCCESS) && dot_ctr != 3) + res = FKO_ERROR_INVALID_ALLOW_IP; + +#if HAVE_SYS_SOCKET_H + /* Stronger IP validation now that we have a candidate that looks + * close enough + */ + if((res == FKO_SUCCESS) && (inet_aton(ip_str, &in) == 0)) + res = FKO_ERROR_INVALID_ALLOW_IP; +#endif + + return(res); +} + +static int +have_port(const char *msg) +{ + const char *ndx = msg; + int startlen = strnlen(msg, MAX_SPA_MESSAGE_SIZE), port_str_len = 0; + + if(startlen == MAX_SPA_MESSAGE_SIZE) + return(FKO_ERROR_INVALID_DATA); + + /* 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 != ',') + { + 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; +} + /* Set the SPA message type. */ int @@ -93,24 +170,10 @@ fko_set_spa_message(fko_ctx_t ctx, const char *msg) /* Basic message type and format checking... */ - switch(ctx->message_type) - { - case FKO_COMMAND_MSG: - res = validate_cmd_msg(msg); - break; - - case FKO_ACCESS_MSG: - case FKO_CLIENT_TIMEOUT_ACCESS_MSG: - res = validate_access_msg(msg); - break; - - case FKO_NAT_ACCESS_MSG: - case FKO_LOCAL_NAT_ACCESS_MSG: - case FKO_CLIENT_TIMEOUT_NAT_ACCESS_MSG: - case FKO_CLIENT_TIMEOUT_LOCAL_NAT_ACCESS_MSG: - res = validate_nat_access_msg(msg); - break; - } + if(ctx->message_type == FKO_COMMAND_MSG) + res = validate_cmd_msg(msg); + else + res = validate_access_msg(msg); if(res != FKO_SUCCESS) return(res); @@ -158,9 +221,9 @@ validate_cmd_msg(const char *msg) if(startlen == MAX_SPA_CMD_LEN) return(FKO_ERROR_INVALID_DATA); - /* Should have a valid allow IP. + /* Should always have a valid allow IP regardless of message type */ - if((res = got_allow_ip(msg)) != FKO_SUCCESS) + if((res = have_allow_ip(msg)) != FKO_SUCCESS) return(res); /* Commands are fairly free-form so all we can really verify is @@ -184,9 +247,9 @@ validate_access_msg(const char *msg) if(startlen == MAX_SPA_MESSAGE_SIZE) return(FKO_ERROR_INVALID_DATA); - /* Should have a valid allow IP. + /* Should always have a valid allow IP regardless of message type */ - if((res = got_allow_ip(msg)) != FKO_SUCCESS) + if((res = have_allow_ip(msg)) != FKO_SUCCESS) return(res); /* Position ourselves beyond the allow IP and make sure we are @@ -208,10 +271,43 @@ validate_access_msg(const char *msg) return(res); } +int +validate_nat_access_msg(const char *msg) +{ + const char *ndx; + int res = FKO_SUCCESS; + int startlen = strnlen(msg, MAX_SPA_MESSAGE_SIZE); + + if(startlen == MAX_SPA_MESSAGE_SIZE) + return(FKO_ERROR_INVALID_DATA); + + /* Should always have a valid allow IP regardless of message type + */ + if((res = have_allow_ip(msg)) != FKO_SUCCESS) + return(res); + + /* Position ourselves beyond the allow IP and make sure we have + * a single port value + */ + ndx = strchr(msg, ','); + if(ndx == NULL || (1+(ndx - msg)) >= startlen) + return(FKO_ERROR_INVALID_SPA_NAT_ACCESS_MSG); + + ndx++; + + if((res = have_port(ndx)) != FKO_SUCCESS) + return(res); + + if(msg[startlen-1] == ',') + return(FKO_ERROR_INVALID_SPA_NAT_ACCESS_MSG); + + return(res); +} + int validate_proto_port_spec(const char *msg) { - int startlen = strnlen(msg, MAX_SPA_MESSAGE_SIZE), port_str_len = 0; + int startlen = strnlen(msg, MAX_SPA_MESSAGE_SIZE); const char *ndx = msg; if(startlen == MAX_SPA_MESSAGE_SIZE) @@ -233,86 +329,7 @@ validate_proto_port_spec(const char *msg) */ ndx++; - /* 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 != ',') - { - 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); -} - -int -validate_nat_access_msg(const char *msg) -{ - int res = FKO_SUCCESS; - - /* Should have a valid access message. - */ - if((res = validate_access_msg(msg)) != FKO_SUCCESS) - return(res); - - // --DSS TODO: XXX: Put nat_access validation code here - - return(FKO_SUCCESS); -} - -int -got_allow_ip(const char *msg) -{ - const char *ndx = msg; - char ip_str[MAX_IPV4_STR_LEN]; - int dot_ctr = 0, char_ctr = 0; - int res = FKO_SUCCESS; -#if HAVE_SYS_SOCKET_H - struct in_addr in; -#endif - - while(*ndx != ',' && *ndx != '\0') - { - ip_str[char_ctr] = *ndx; - char_ctr++; - if(char_ctr >= MAX_IPV4_STR_LEN) - { - res = FKO_ERROR_INVALID_ALLOW_IP; - break; - } - if(*ndx == '.') - dot_ctr++; - else if(isdigit(*ndx) == 0) - { - res = FKO_ERROR_INVALID_ALLOW_IP; - break; - } - ndx++; - } - - if(char_ctr < MAX_IPV4_STR_LEN) - ip_str[char_ctr] = '\0'; - else - res = FKO_ERROR_INVALID_ALLOW_IP; - - if ((res == FKO_SUCCESS) && (char_ctr < MIN_IPV4_STR_LEN)) - res = FKO_ERROR_INVALID_ALLOW_IP; - - if((res == FKO_SUCCESS) && dot_ctr != 3) - res = FKO_ERROR_INVALID_ALLOW_IP; - -#if HAVE_SYS_SOCKET_H - /* Stronger IP validation now that we have a candidate that looks - * close enough - */ - if((res == FKO_SUCCESS) && (inet_aton(ip_str, &in) == 0)) - res = FKO_ERROR_INVALID_ALLOW_IP; -#endif - - return(res); + return have_port(ndx); } /***EOF***/ diff --git a/lib/fko_message.h b/lib/fko_message.h index cb61ed11..8c57252d 100644 --- a/lib/fko_message.h +++ b/lib/fko_message.h @@ -49,9 +49,8 @@ */ int validate_cmd_msg(const char *msg); int validate_access_msg(const char *msg); -int validate_proto_port_spec(const char *msg); int validate_nat_access_msg(const char *msg); -int got_allow_ip(const char *msg); +int validate_proto_port_spec(const char *msg); #endif /* FKO_MESSAGE_H */ diff --git a/lib/fko_nat_access.c b/lib/fko_nat_access.c index bea731a5..03a7cad5 100644 --- a/lib/fko_nat_access.c +++ b/lib/fko_nat_access.c @@ -36,6 +36,8 @@ int fko_set_spa_nat_access(fko_ctx_t ctx, const char *msg) { + int res = FKO_SUCCESS; + /* Context must be initialized. */ if(!CTX_INITIALIZED(ctx)) @@ -52,6 +54,9 @@ fko_set_spa_nat_access(fko_ctx_t ctx, const char *msg) if(strnlen(msg, MAX_SPA_NAT_ACCESS_SIZE) == MAX_SPA_NAT_ACCESS_SIZE) return(FKO_ERROR_DATA_TOO_LARGE); + if((res = validate_nat_access_msg(msg)) != FKO_SUCCESS) + return(res); + /* Just in case this is a subsquent call to this function. We * do not want to be leaking memory. */ diff --git a/test/test-fwknop.pl b/test/test-fwknop.pl index fb54c012..f0382da8 100755 --- a/test/test-fwknop.pl +++ b/test/test-fwknop.pl @@ -54,6 +54,7 @@ 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", + 'disable_aging_nat' => "$conf_dir/disable_aging_nat_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", @@ -1104,6 +1105,17 @@ my @tests = ( 'server_conf' => $cf{'nat'}, 'fatal' => $NO }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'client', + 'detail' => "NAT bogus IP validation", + 'err_msg' => "could not complete NAT SPA cycle", + 'function' => \&generic_exec, + 'exec_err' => $YES, + 'cmdline' => "$default_client_args -N 999.1.1.1:22", + 'fatal' => $NO + }, + { 'category' => 'Rijndael SPA', 'subcategory' => 'client+server', @@ -1114,8 +1126,8 @@ my @tests = ( 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . "$fwknopdCmd -c $cf{'nat'} -a $cf{'force_nat_access'} " . "-d $default_digest_file -p $default_pid_file $intf_str", - 'server_positive_output_matches' => [qr/to\:$force_nat_host\:22/i], - 'server_negative_output_matches' => [qr/to\:$internal_nat_host\:22/i], + 'server_positive_output_matches' => [qr/\sto\:$force_nat_host\:22/i], + 'server_negative_output_matches' => [qr/\sto\:$internal_nat_host\:22/i], 'fw_rule_created' => $NEW_RULE_REQUIRED, 'fw_rule_removed' => $NEW_RULE_REMOVED, 'server_conf' => $cf{'nat'}, @@ -1541,6 +1553,30 @@ my @tests = ( "-d $default_digest_file -p $default_pid_file $intf_str", 'fatal' => $NO }, + { + 'category' => 'Rijndael SPA', + 'subcategory' => 'FUZZING', + 'detail' => 'invalid NAT IP', + '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 -N 999.1.1.1:22 + 'fuzzing_pkt' => + '/v/kYVOqw+NCkg8CNEphPPvH3dOAECWjqiF+NNYnK7yKHer/Gy8wCVNa/Rr/Wnm' . + 'siApB3jrXEfyEY3yebJV+PHoYIYC3+4Trt2jxw0m+6iR231Ywhw1JetIPwsv7iQ' . + 'ATvSTpZ+qiaoN0PPfy0+7yM6KlaQIu7bfG5E2a6VJTqTZ1qYz3H7QaJfbAtOD8j' . + 'yEkDgP5+f49xrRA', + 'server_positive_output_matches' => [qr/Args\scontain\sinvalid\sdata/], + 'fwknopd_cmdline' => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " . + "$fwknopdCmd -c $cf{'disable_aging_nat'} -a $cf{'def_access'} " . + "-d $default_digest_file -p $default_pid_file $intf_str", + 'fatal' => $NO + }, + { 'category' => 'FUZZING', 'subcategory' => 'server', @@ -1741,6 +1777,14 @@ my @tests = ( 'function' => \&perl_fko_module_access_msgs, 'fatal' => $NO }, + { + 'category' => 'perl FKO module', + 'subcategory' => 'basic ops', + 'detail' => 'libfko get/set NAT access msgs', + 'err_msg' => 'could not get/set libfko NAT access msgs', + 'function' => \&perl_fko_module_nat_access_msgs, + 'fatal' => $NO + }, { 'category' => 'perl FKO module', 'subcategory' => 'basic ops', @@ -2958,6 +3002,60 @@ sub perl_fko_module_access_msgs() { return $rv; } +sub perl_fko_module_nat_access_msgs() { + my $test_hr = shift; + + my $rv = 1; + + $fko_obj = FKO->new(); + + unless ($fko_obj) { + &write_test_file("[-] error FKO->new(): " . FKO::error_str() . "\n", + $current_test_file); + return 0; + } + + $fko_obj->spa_message_type(FKO->FKO_NAT_ACCESS_MSG); + + for my $msg (@{valid_nat_access_messages()}) { + + ### set message and then see if it matches + my $status = $fko_obj->spa_nat_access($msg); + + if ($status == FKO->FKO_SUCCESS and $fko_obj->spa_nat_access() eq $msg) { + &write_test_file("[+] get/set spa_nat_access(): $msg\n", + $current_test_file); + } else { + &write_test_file("[-] could not get/set spa_nat_access(): $msg " . + FKO::error_str() . "\n", + $current_test_file); + $rv = 0; + last; + } + } + + for my $bogus_msg (@{&bogus_nat_access_messages()}, @{&valid_access_messages()}) { + + ### set message type and then see if it matches + my $status = $fko_obj->spa_nat_access($bogus_msg); + + if ($status == FKO->FKO_SUCCESS) { + &write_test_file("[-] libfko allowed bogus " . + "spa_nat_access(): $bogus_msg, got: " . $fko_obj->spa_nat_access() . ' ' . + FKO::error_str() . "\n", + $current_test_file); + $rv = 0; + } else { + &write_test_file("[+] libfko rejected bogus spa_nat_access(): $bogus_msg\n", + $current_test_file); + } + } + + $fko_obj->destroy(); + + return $rv; +} + sub perl_fko_module_cmd_msgs() { my $test_hr = shift; @@ -3025,6 +3123,14 @@ sub valid_access_messages() { return \@msgs; } +sub valid_nat_access_messages() { + my @msgs = ( + '1.2.3.4,22', + '123.123.123.123,12345', + ); + return \@msgs; +} + sub valid_cmd_messages() { my @msgs = ( '1.2.3.4,cat /etc/hosts', @@ -3038,6 +3144,15 @@ sub valid_cmd_messages() { } sub bogus_access_messages() { + my @msgs = (); + + push @msgs, @{&bogus_nat_access_messages()}; + push @msgs, '1.1.1.2,12345', + push @msgs, @{&valid_nat_access_messages()}; + return \@msgs; +} + +sub bogus_nat_access_messages() { my @msgs = ( '1.2.3.4', '1.2.3.4.', @@ -3065,7 +3180,6 @@ sub bogus_access_messages() { pack('a', ""), '1.1.1.p/12345', '1.1.1.2,,,,12345', - '1.1.1.2,12345', '1.1.1.2,icmp/123', ',,,', '----',