[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.

diff --git a/CREDITS b/CREDITS
index 14753ea..4a3dcf7 100644 (file)
--- 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 5e800b0..f42600b 100644 (file)
--- 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 1d83d5d..cbd39d2 100644 (file)
--- 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 5778c01..4609004 100644 (file)
--- 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 2496635..52150f7 100644 (file)
--- 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 bfe7dd4..b678d31 100755 (executable)
--- 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',