[libfko] validation of NAT access strings
authorMichael Rash <mbr@cipherdyne.org>
Tue, 16 Oct 2012 00:52:23 +0000 (20:52 -0400)
committerMichael Rash <mbr@cipherdyne.org>
Tue, 16 Oct 2012 00:52:23 +0000 (20:52 -0400)
Added validation of NAT access strings in the various NAT modes in libfko.
This applies to both the client and server, and test suite support was added
as well.

ChangeLog
Makefile.am
lib/fko_decode.c
lib/fko_message.c
lib/fko_message.h
lib/fko_nat_access.c
test/test-fwknop.pl

index e679258..b6498de 100644 (file)
--- 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
index 6504b02..71944c6 100644 (file)
@@ -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 \
index b1e36a6..574c80f 100644 (file)
@@ -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.
index d7361f9..7d8237b 100644 (file)
 #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
@@ -209,110 +272,64 @@ validate_access_msg(const char *msg)
 }
 
 int
-validate_proto_port_spec(const char *msg)
+validate_nat_access_msg(const char *msg)
 {
-    int     startlen    = strnlen(msg, MAX_SPA_MESSAGE_SIZE), port_str_len = 0;
-    const char   *ndx   = 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);
 
-    /* Now check for proto/port string.
+    /* Should always have a valid allow IP regardless of message type
     */
-    if(strncmp(ndx, "tcp", 3)
-      && strncmp(ndx, "udp", 3)
-      && strncmp(ndx, "icmp", 4)
-      && strncmp(ndx, "none", 4))
-        return(FKO_ERROR_INVALID_SPA_ACCESS_MSG);
-
-    ndx = strchr(ndx, '/');
-    if(ndx == NULL || ((1+(ndx - msg)) > MAX_PROTO_STR_LEN))
-        return(FKO_ERROR_INVALID_SPA_ACCESS_MSG);
-
-    /* Skip over the '/' and make sure we only have digits.
-    */
-    ndx++;
+    if((res = have_allow_ip(msg)) != FKO_SUCCESS)
+        return(res);
 
-    /* Must have at least one digit for the port number
+    /* Position ourselves beyond the allow IP and make sure we have
+     * a single port value
     */
-    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);
-}
+    ndx = strchr(msg, ',');
+    if(ndx == NULL || (1+(ndx - msg)) >= startlen)
+        return(FKO_ERROR_INVALID_SPA_NAT_ACCESS_MSG);
 
-int
-validate_nat_access_msg(const char *msg)
-{
-    int res = FKO_SUCCESS;
+    ndx++;
 
-    /* Should have a valid access message.
-    */
-    if((res = validate_access_msg(msg)) != FKO_SUCCESS)
+    if((res = have_port(ndx)) != FKO_SUCCESS)
         return(res);
 
-    // --DSS TODO: XXX: Put nat_access validation code here
+    if(msg[startlen-1] == ',')
+        return(FKO_ERROR_INVALID_SPA_NAT_ACCESS_MSG);
 
-    return(FKO_SUCCESS);
+    return(res);
 }
 
 int
-got_allow_ip(const char *msg)
+validate_proto_port_spec(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++;
-    }
+    int     startlen    = strnlen(msg, MAX_SPA_MESSAGE_SIZE);
+    const char   *ndx   = msg;
 
-    if(char_ctr < MAX_IPV4_STR_LEN)
-        ip_str[char_ctr] = '\0';
-    else
-        res = FKO_ERROR_INVALID_ALLOW_IP;
+    if(startlen == MAX_SPA_MESSAGE_SIZE)
+        return(FKO_ERROR_INVALID_DATA);
 
-    if ((res == FKO_SUCCESS) && (char_ctr < MIN_IPV4_STR_LEN))
-        res = FKO_ERROR_INVALID_ALLOW_IP;
+    /* Now check for proto/port string.
+    */
+    if(strncmp(ndx, "tcp", 3)
+      && strncmp(ndx, "udp", 3)
+      && strncmp(ndx, "icmp", 4)
+      && strncmp(ndx, "none", 4))
+        return(FKO_ERROR_INVALID_SPA_ACCESS_MSG);
 
-    if((res == FKO_SUCCESS) && dot_ctr != 3)
-        res = FKO_ERROR_INVALID_ALLOW_IP;
+    ndx = strchr(ndx, '/');
+    if(ndx == NULL || ((1+(ndx - msg)) > MAX_PROTO_STR_LEN))
+        return(FKO_ERROR_INVALID_SPA_ACCESS_MSG);
 
-#if HAVE_SYS_SOCKET_H
-    /* Stronger IP validation now that we have a candidate that looks
-     * close enough
+    /* Skip over the '/' and make sure we only have digits.
     */
-    if((res == FKO_SUCCESS) && (inet_aton(ip_str, &in) == 0))
-        res = FKO_ERROR_INVALID_ALLOW_IP;
-#endif
+    ndx++;
 
-    return(res);
+    return have_port(ndx);
 }
 
 /***EOF***/
index cb61ed1..8c57252 100644 (file)
@@ -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 */
 
index bea731a..03a7cad 100644 (file)
@@ -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.
     */
index fb54c01..f0382da 100755 (executable)
@@ -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",
@@ -1106,6 +1107,17 @@ my @tests = (
     },
     {
         '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',
         'detail'   => "force NAT $force_nat_host (tcp/22 ssh)",
         'err_msg'  => "could not complete NAT SPA cycle",
@@ -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'},
@@ -1542,6 +1554,30 @@ my @tests = (
         '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',
         'detail'   => 'invalid SOURCE access.conf',
@@ -1744,6 +1780,14 @@ my @tests = (
     {
         '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',
         'detail'   => 'libfko get/set cmd msgs',
         'err_msg'  => 'could not get/set libfko cmd msgs',
         'function' => \&perl_fko_module_cmd_msgs,
@@ -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',
         ',,,',
         '----',