From 7a1bdea5140de8791d22125fca8a5b6eb50619ec Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Sun, 9 Jun 2013 14:28:17 -0400 Subject: [PATCH] [server] fix 'Use of untrusted string value' bug found by Coverity 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 | 1 + server/fw_util_iptables.c | 72 ++++++++--------------- server/fw_util_iptables.h | 4 +- test/conf/custom_input_chain_fwknopd.conf | 2 + test/conf/custom_nat_chain_fwknopd.conf | 5 ++ test/test-fwknop.pl | 2 + test/tests/rijndael_hmac.pl | 37 ++++++++++++ 7 files changed, 73 insertions(+), 50 deletions(-) create mode 100644 test/conf/custom_input_chain_fwknopd.conf create mode 100644 test/conf/custom_nat_chain_fwknopd.conf diff --git a/server/fw_util.h b/server/fw_util.h index 4e6eccd5..ceebfc7b 100644 --- a/server/fw_util.h +++ b/server/fw_util.h @@ -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 diff --git a/server/fw_util_iptables.c b/server/fw_util_iptables.c index 9e76f60a..6a3bab45 100644 --- a/server/fw_util_iptables.c +++ b/server/fw_util_iptables.c @@ -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) + if(rule_exists(opts, fwc.chain[chain_num].from_chain, rule_buf) == 1) { - log_msg(LOG_ERR, - "Got error %i trying to get rules list.\n", errno); - return(-1); + log_msg(LOG_INFO, "jump_rule_exists() jump rule found"); + exists = 1; } + else + log_msg(LOG_INFO, "jump_rule_exists() jump rule not found"); - 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(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"); - } - - 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); } diff --git a/server/fw_util_iptables.h b/server/fw_util_iptables.h index f9090db2..e34b0958 100644 --- a/server/fw_util_iptables.h +++ b/server/fw_util_iptables.h @@ -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 index 00000000..75a37d99 --- /dev/null +++ b/test/conf/custom_input_chain_fwknopd.conf @@ -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 index 00000000..39b2a3b9 --- /dev/null +++ b/test/conf/custom_nat_chain_fwknopd.conf @@ -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; diff --git a/test/test-fwknop.pl b/test/test-fwknop.pl index cea89b02..424dcf7c 100755 --- a/test/test-fwknop.pl +++ b/test/test-fwknop.pl @@ -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", diff --git a/test/tests/rijndael_hmac.pl b/test/tests/rijndael_hmac.pl index d9617d6a..65aafd8f 100644 --- a/test/tests/rijndael_hmac.pl +++ b/test/tests/rijndael_hmac.pl @@ -80,6 +80,23 @@ 'key_file' => $cf{'rc_hmac_b64_key'}, 'fatal' => $NO }, + { + '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', @@ -699,6 +716,26 @@ 'server_conf' => $cf{'nat'}, 'fatal' => $NO }, + { + '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',