DeforaNetworks/fwknop
3
0
Fork 0
Code Releases Activity

[server] fix 'Use of untrusted string value' bug found by Coverity

Browse Source 
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).
This commit is contained in:
Michael Rash
2013-06-09 14:28:17 -04:00
parent f491c41697
commit 7a1bdea514
7 changed files with 73 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/fw_util.h
+1
View 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
server/fw_util_iptables.c
+23 -49
View 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)
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);
}
server/fw_util_iptables.h
+3 -1
View 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"
test/conf/custom_input_chain_fwknopd.conf
+2
View File
@@ -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;
test/conf/custom_nat_chain_fwknopd.conf
+5
View File
@@ -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;
test/test-fwknop.pl
+2
View File
@@ -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",
test/tests/rijndael_hmac.pl
+37
View File
@@ -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',