Permalink
Browse files

[server] Stronger IP validation based on a bug found by Fernando Arna…

…boldi 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.
  • Loading branch information...
1 parent d46ba1c commit f4c16bc47fc24a96b63105556b62d61c1ba7d799 @mrash committed Aug 26, 2012
Showing with 85 additions and 4 deletions.
  1. +2 −0 CREDITS
  2. +4 −0 ChangeLog
  3. +12 −4 lib/fko_message.c
  4. +67 −0 test/test-fwknop.pl
View
@@ -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).
View
@@ -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.
View
@@ -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);
View
@@ -1241,6 +1241,17 @@
{
'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 @@ ()
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;

0 comments on commit f4c16bc

Please sign in to comment.