DeforaNetworks/fwknop
3
0
Fork 0
Code Releases Activity

[server] Strong access.conf validation

Browse Source 
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.
This commit is contained in:
Michael Rash
2012-09-03 00:21:46 -04:00
parent 263fa01f2a
commit e2c0ac4821
6 changed files with 133 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CREDITS
+6
View File
@@ -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.
ChangeLog
+4
View File
@@ -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.
Makefile.am
+3
View File
@@ -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 \
server/access.c
+76 -38
View File
@@ -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)
{
test/conf/open_ports_access.conf
+2 -2
View File
@@ -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;
test/test-fwknop.pl
+42
View File
@@ -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',