From f4c16bc47fc24a96b63105556b62d61c1ba7d799 Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sat, 25 Aug 2012 23:08:55 -0400 Subject: [PATCH] [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 71f0fa8..14753ea 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 00f3a37..39c5f0b 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 9148c2d..3228dfa 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 2f78bb4..2b011bc 100755 --- a/test/test-fwknop.pl +++ b/test/test-fwknop.pl @@ -1241,6 +1241,17 @@ my @tests = ( { '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', 'detail' => 'negative port value', 'err_msg' => 'server crashed or did not detect error condition', 'function' => \&negative_port, @@ -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; -- 1.7.5.4