[server] fix 'Use of untrusted string value' bug found by Coverity
authorMichael Rash <mbr@cipherdyne.org>
Sun, 9 Jun 2013 18:28:17 +0000 (14:28 -0400)
committerMichael Rash <mbr@cipherdyne.org>
Sun, 9 Jun 2013 18:28:17 +0000 (14:28 -0400)
This commit changes iptables policy parsing to re-use rule_exists() for fwknop
jump rule detection instead of using sscanf() against iptables policy list
output.  Also, fwknop jump rules are now deleted from iptables policies in a
loop to ensure all are removed even if there are duplicates (even though this
should not happen under normal circumstances anyway).

server/fw_util.h
server/fw_util_iptables.c
server/fw_util_iptables.h
test/conf/custom_input_chain_fwknopd.conf [new file with mode: 0644]
test/conf/custom_nat_chain_fwknopd.conf [new file with mode: 0644]
test/test-fwknop.pl
test/tests/rijndael_hmac.pl

index 4e6eccd..ceebfc7 100644 (file)
@@ -33,6 +33,7 @@
 
 #define CMD_BUFSIZE                 256
 #define MAX_FW_COMMAND_ARGS_LEN     256
+#define CMD_LOOP_TRIES              10   /* for repeated command executions */
 
 #define STANDARD_CMD_OUT_BUFSIZE    4096
 
index 9e76f60..6a3bab4 100644 (file)
@@ -44,6 +44,9 @@ static char   cmd_buf[CMD_BUFSIZE];
 static char   err_buf[CMD_BUFSIZE];
 static char   cmd_out[STANDARD_CMD_OUT_BUFSIZE];
 
+static int rule_exists(const fko_srv_options_t * const opts,
+        const char * const fw_chain, const char * const fw_rule);
+
 static void
 zero_cmd_buffers(void)
 {
@@ -190,56 +193,25 @@ chain_exists(const fko_srv_options_t * const opts, const int chain_num)
 static int
 jump_rule_exists(const fko_srv_options_t * const opts, const int chain_num)
 {
-    int     num, pos = 0;
-    char    cmd_buf[CMD_BUFSIZE]  = {0};
-    char    target[CMD_BUFSIZE]   = {0};
-    char    line_buf[CMD_BUFSIZE] = {0};
-    FILE   *ipt;
+    int    exists = 0;
+    char   rule_buf[CMD_BUFSIZE] = {0};
 
-    snprintf(cmd_buf, CMD_BUFSIZE-1, "%s " IPT_LIST_RULES_ARGS,
-        fwc.fw_command,
+    memset(rule_buf, 0, CMD_BUFSIZE);
+
+    snprintf(rule_buf, CMD_BUFSIZE-1, IPT_CHK_JUMP_RULE_ARGS,
         fwc.chain[chain_num].table,
-        fwc.chain[chain_num].from_chain
+        fwc.chain[chain_num].to_chain
     );
 
-    ipt = popen(cmd_buf, "r");
-
-    if(ipt == NULL)
-    {
-        log_msg(LOG_ERR,
-            "Got error %i trying to get rules list.\n", errno);
-        return(-1);
-    }
-
-    while((fgets(line_buf, CMD_BUFSIZE-1, ipt)) != NULL)
-    {
-        /* Get past comments and empty lines (note: we only look at the
-         * first character).
-        */
-        if(IS_EMPTY_LINE(line_buf[0]))
-            continue;
-
-        if(sscanf(line_buf, "%i %s ", &num, target) == 2)
-        {
-            if(strcmp(target, fwc.chain[chain_num].to_chain) == 0)
-            {
-                pos = num;
-                break;
-            }
-        }
-    }
-
-    pclose(ipt);
-
-    if (opts->verbose)
+    if(rule_exists(opts, fwc.chain[chain_num].from_chain, rule_buf) == 1)
     {
-        if(pos > 0)
-            log_msg(LOG_INFO, "jump_rule_exists() jump rule position: %d", pos);
-        else
-            log_msg(LOG_INFO, "jump_rule_exists() jump rule not found");
+        log_msg(LOG_INFO, "jump_rule_exists() jump rule found");
+        exists = 1;
     }
+    else
+        log_msg(LOG_INFO, "jump_rule_exists() jump rule not found");
 
-    return(pos);
+    return exists;
 }
 
 /* Print all firewall rules currently instantiated by the running fwknopd
@@ -333,8 +305,7 @@ fw_dump_rules(const fko_srv_options_t * const opts)
 static void
 delete_all_chains(const fko_srv_options_t * const opts)
 {
-    int     i, res;
-    int     jump_rule_num;
+    int     i, res, cmd_ctr = 0;
 
     for(i=0; i<(NUM_FWKNOP_ACCESS_TYPES); i++)
     {
@@ -344,15 +315,16 @@ delete_all_chains(const fko_srv_options_t * const opts)
         /* First look for a jump rule to this chain and remove it if it
          * is there.
         */
-        if((jump_rule_num = jump_rule_exists(opts, i)) > 0)
+        cmd_ctr = 0;
+        while(cmd_ctr < CMD_LOOP_TRIES && (jump_rule_exists(opts, i) == 1))
         {
             zero_cmd_buffers();
 
-            snprintf(cmd_buf, CMD_BUFSIZE-1, "%s " IPT_DEL_RULE_ARGS,
+            snprintf(cmd_buf, CMD_BUFSIZE-1, "%s " IPT_DEL_JUMP_RULE_ARGS,
                 fwc.fw_command,
                 fwc.chain[i].table,
                 fwc.chain[i].from_chain,
-                jump_rule_num
+                fwc.chain[i].to_chain
             );
 
             res = run_extcmd(cmd_buf, err_buf, CMD_BUFSIZE, 0);
@@ -365,6 +337,8 @@ delete_all_chains(const fko_srv_options_t * const opts)
             /* Expect full success on this */
             if(! EXTCMD_IS_SUCCESS(res))
                 log_msg(LOG_ERR, "Error %i from cmd:'%s': %s", res, cmd_buf, err_buf);
+
+            cmd_ctr++;
         }
 
         zero_cmd_buffers();
@@ -664,7 +638,7 @@ rule_exists(const fko_srv_options_t * const opts,
     {
         rule_exists = 1;
         if (opts->verbose)
-            log_msg(LOG_INFO, "rule_exists() Rule : '%s' in %s already exist.",
+            log_msg(LOG_INFO, "rule_exists() Rule : '%s' in %s already exists.",
                     fw_rule, fw_chain);
     }
 
index f9090db..e34b095 100644 (file)
@@ -36,7 +36,7 @@
 /* iptables command args
 */
 #define IPT_RULE_ARGS           "-t %s -p %i -s %s --dport %i -m comment --comment " EXPIRE_COMMENT_PREFIX "%u -j %s 2>&1"
-#define IPT_CHK_RULE_ARGS       "-C %s %s 2>&1"
+#define IPT_CHK_RULE_ARGS       "-C %s %s"
 #define IPT_OUT_RULE_ARGS       "-t %s -p %i -d %s --sport %i -m comment --comment " EXPIRE_COMMENT_PREFIX "%u -j %s 2>&1"
 #define IPT_FWD_RULE_ARGS       "-t %s -p %i -s %s -d %s --dport %i -m comment --comment " EXPIRE_COMMENT_PREFIX "%u -j %s 2>&1"
 #define IPT_DNAT_RULE_ARGS      "-t %s -p %i -s %s --dport %i -m comment --comment " EXPIRE_COMMENT_PREFIX "%u -j %s --to-destination %s:%i 2>&1"
@@ -47,7 +47,9 @@
 #define IPT_FLUSH_CHAIN_ARGS    "-t %s -F %s 2>&1"
 #define IPT_CHAIN_EXISTS_ARGS   "-t %s -L %s -n 2>&1"
 #define IPT_DEL_CHAIN_ARGS      "-t %s -X %s 2>&1"
+#define IPT_CHK_JUMP_RULE_ARGS  "-t %s -j %s 2>&1"
 #define IPT_ADD_JUMP_RULE_ARGS  "-t %s -I %s %i -j %s 2>&1"
+#define IPT_DEL_JUMP_RULE_ARGS  "-t %s -D %s -j %s 2>&1"  /* let iptables work out the rule number */
 #define IPT_LIST_RULES_ARGS     "-t %s -L %s --line-numbers -n 2>&1"
 #define IPT_LIST_ALL_RULES_ARGS "-t %s -v -n -L --line-numbers 2>&1"
 
diff --git a/test/conf/custom_input_chain_fwknopd.conf b/test/conf/custom_input_chain_fwknopd.conf
new file mode 100644 (file)
index 0000000..75a37d9
--- /dev/null
@@ -0,0 +1,2 @@
+# default config - no variables set to allow defaults to be preserved
+IPT_INPUT_ACCESS        ACCEPT, filter, INPUT, 1, FWKNOP_INPUT_TEST, 1;
diff --git a/test/conf/custom_nat_chain_fwknopd.conf b/test/conf/custom_nat_chain_fwknopd.conf
new file mode 100644 (file)
index 0000000..39b2a3b
--- /dev/null
@@ -0,0 +1,5 @@
+ENABLE_IPT_FORWARDING           Y;
+
+IPT_INPUT_ACCESS        ACCEPT, filter, INPUT, 1, FWKNOP_INPUT_TEST, 1;
+IPT_FORWARD_ACCESS      ACCEPT, filter, FORWARD, 1, FWKNOP_FORWARD_TEST, 1;
+IPT_DNAT_ACCESS         DNAT, nat, PREROUTING, 1, FWKNOP_PREROUTING_TEST, 1;
index cea89b0..424dcf7 100755 (executable)
@@ -135,6 +135,8 @@ our %cf = (
     'rc_hmac_sha512_short_key'     => "$conf_dir/fwknoprc_hmac_sha512_short_key",
     'rc_hmac_sha512_long_key'      => "$conf_dir/fwknoprc_hmac_sha512_long_key",
     'base64_key_access'            => "$conf_dir/base64_key_access.conf",
+    'custom_input_chain'           => "$conf_dir/custom_input_chain_fwknopd.conf",
+    'custom_nat_chain'             => "$conf_dir/custom_nat_chain_fwknopd.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",
index d9617d6..65aafd8 100644 (file)
     {
         'category' => 'Rijndael+HMAC',
         'subcategory' => 'client+server',
+        'detail'   => 'custom input chain (tcp/22)',
+        'function' => \&spa_cycle,
+        'cmdline'  => $default_client_hmac_args,
+        'fwknopd_cmdline'  => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " .
+            "$fwknopdCmd -c $cf{'custom_input_chain'} -a $cf{'hmac_access'} " .
+            "-d $default_digest_file -p $default_pid_file $intf_str",
+        'server_positive_output_matches' => [qr/FWKNOP_INPUT_TEST\s\(1\sreferences/],
+        'fw_rule_created' => $NEW_RULE_REQUIRED,
+        'fw_rule_removed' => $NEW_RULE_REMOVED,
+        'server_conf'     => $cf{'custom_input_chain'},
+        'key_file' => $cf{'rc_hmac_b64_key'},
+        'fatal'    => $NO
+    },
+
+    {
+        'category' => 'Rijndael+HMAC',
+        'subcategory' => 'client+server',
         'detail'   => '--get-hmac-key (tcp/22 ssh)',
         'function' => \&spa_cycle,
         'cmdline'  => $default_client_args .
     {
         'category' => 'Rijndael+HMAC',
         'subcategory' => 'client+server',
+        'detail'   => "NAT to $internal_nat_host custom chain",
+        'function' => \&spa_cycle,
+        'cmdline'  => "$default_client_args_no_get_key --rc-file " .
+            "$cf{'rc_hmac_b64_key'} -N $internal_nat_host:22",
+        'fwknopd_cmdline'  => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " .
+            "$fwknopdCmd -c $cf{'custom_nat_chain'} -a $cf{'hmac_open_ports_access'} " .
+            "-d $default_digest_file -p $default_pid_file $intf_str",
+        'server_positive_output_matches' => [
+            qr/FWKNOP_FORWARD_TEST\s.*dport\s22\s/,
+            qr/to\:$internal_nat_host\:22/i],
+        'fw_rule_created' => $NEW_RULE_REQUIRED,
+        'fw_rule_removed' => $NEW_RULE_REMOVED,
+        'key_file' => $cf{'rc_hmac_b64_key'},
+        'server_conf' => $cf{'custom_nat_chain'},
+        'fatal'    => $NO
+    },
+
+    {
+        'category' => 'Rijndael+HMAC',
+        'subcategory' => 'client+server',
         'detail'   => "NAT tcp/80 to $internal_nat_host tcp/22",
         'function' => \&spa_cycle,
         'cmdline'  => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " .