DeforaNetworks/fwknop
DeforaNetworks/fwknop
3
0
Fork 0
Code Releases Activity

file permissions and client buffer overflow fix

Browse Source 
- [client+server] Fernando Arnaboldi from IOActive found that strict
filesystem permissions for various fwknop files are not verified.  Added
warnings whenever permissions are not strict enough, and ensured that
files created by the fwknop client and server are only set to user
read/write.
- [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.
This commit is contained in:
Michael Rash 2012-08-29 22:21:43 -04:00
parent 186a424353
commit a60f05ad44
13 changed files with 217 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
ChangeLog
View File

@ -13,6 +13,14 @@ fwknop-2.0.3 (08//2012):
the server did not properly validate allow IP addresses from malicious
authenticated clients. This has been fixed with stronger allow IP
validation.
- [client+server] Fernando Arnaboldi from IOActive found that strict
filesystem permissions for various fwknop files are not verified. Added
warnings whenever permissions are not strict enough, and ensured that
files created by the fwknop client and server are only set to user
read/write.
- [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.
- [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.

15
client/config_init.c
View File

@ -124,9 +124,9 @@ parse_time_offset(const char *offset_str)
static int
create_fwknoprc(const char *rcfile)
{
FILE *rc;
FILE *rc = NULL;
fprintf(stderr, "Creating initial rc file: %s.\n", rcfile);
fprintf(stdout, "[*] Creating initial rc file: %s.\n", rcfile);
if ((rc = fopen(rcfile, "w")) == NULL)
{
@ -188,7 +188,7 @@ create_fwknoprc(const char *rcfile)
"# User-provided named stanzas:\n"
"\n"
"# Example for a destination server of 192.168.1.20 to open access to \n"
"# SSH for an IP that is resoved exteranlly, and one with a NAT request\n"
"# SSH for an IP that is resolved externally, and one with a NAT request\n"
"# for a specific source IP that maps port 8088 on the server\n"
"# to port 88 on 192.168.1.55 with timeout.\n"
"#\n"
@ -210,6 +210,8 @@ create_fwknoprc(const char *rcfile)
fclose(rc);
set_file_perms(rcfile);
return(0);
}
@ -440,6 +442,13 @@ process_rc(fko_cli_options_t *options)
rcfile[rcf_offset] = PATH_SEP;
strlcat(rcfile, ".fwknoprc", MAX_PATH_LEN);
/* Check rc file permissions - if anything other than user read/write,
* then don't process it. This change was made to help ensure that the
* client consumes a proper rc file with strict permissions set (thanks
* to Fernando Arnaboldi from IOActive for pointing this out).
*/
verify_file_perms_ownership(rcfile);
/* Open the rc file for reading, if it does not exist, then create
* an initial .fwknoprc file with defaults and go on.
*/

19
client/fwknop.c
View File

@ -48,6 +48,8 @@ static int set_nat_access(fko_ctx_t ctx, fko_cli_options_t *options);
static int get_rand_port(fko_ctx_t ctx);
int resolve_ip_http(fko_cli_options_t *options);
#define MAX_CMDLINE_ARGS 50 /* should be way more than enough */
int
main(int argc, char **argv)
{
@ -556,6 +558,8 @@ show_last_command(void)
exit(EXIT_FAILURE);
#endif
verify_file_perms_ownership(args_save_file);
if (get_save_file(args_save_file)) {
if ((args_file_ptr = fopen(args_save_file, "r")) == NULL) {
fprintf(stderr, "Could not open args file: %s\n",
@ -587,7 +591,7 @@ run_last_args(fko_cli_options_t *options)
char args_save_file[MAX_PATH_LEN] = {0};
char args_str[MAX_LINE_LEN] = {0};
char arg_tmp[MAX_LINE_LEN] = {0};
char *argv_new[200]; /* should be way more than enough */
char *argv_new[MAX_CMDLINE_ARGS]; /* should be way more than enough */
#ifdef WIN32
@ -599,6 +603,8 @@ run_last_args(fko_cli_options_t *options)
if (get_save_file(args_save_file))
{
verify_file_perms_ownership(args_save_file);
if ((args_file_ptr = fopen(args_save_file, "r")) == NULL)
{
fprintf(stderr, "Could not open args file: %s\n",
@ -623,12 +629,17 @@ run_last_args(fko_cli_options_t *options)
argv_new[argc_new] = malloc(strlen(arg_tmp)+1);
if (argv_new[argc_new] == NULL)
{
fprintf(stderr, "malloc failure for cmd line arg.\n");
fprintf(stderr, "[*] malloc failure for cmd line arg.\n");
exit(EXIT_FAILURE);
}
strlcpy(argv_new[argc_new], arg_tmp, strlen(arg_tmp)+1);
current_arg_ctr = 0;
argc_new++;
if(argc_new >= MAX_CMDLINE_ARGS)
{
fprintf(stderr, "[*] max command line args exceeded.\n");
exit(EXIT_FAILURE);
}
}
}
}
@ -661,7 +672,6 @@ save_args(int argc, char **argv)
return;
#endif
if (get_save_file(args_save_file)) {
if ((args_file_ptr = fopen(args_save_file, "w")) == NULL) {
fprintf(stderr, "Could not open args file: %s\n",
@ -680,6 +690,9 @@ save_args(int argc, char **argv)
fprintf(args_file_ptr, "%s\n", args_str);
fclose(args_file_ptr);
}
set_file_perms(args_save_file);
return;
}

66
client/utils.c
View File

@ -28,8 +28,6 @@
*
*****************************************************************************
*/
#include <stdio.h>
#include <string.h>
#include "utils.h"
/* Generic hex dump function.
@ -69,5 +67,69 @@ hex_dump(const unsigned char *data, const int size)
}
}
int
set_file_perms(const char *file)
{
int res = 0;
res = chmod(file, S_IRUSR | S_IWUSR);
if(res != 0)
{
fprintf(stderr,
"[-] unable to chmod file %s to user read/write (0600, -rw-------): %s\n",
file,
strerror(errno)
);
}
return res;
}
int
verify_file_perms_ownership(const char *file)
{
#if HAVE_STAT
struct stat st;
/* Every file that the fwknop client deals with should be owned
* by the user and permissions set to 600 (user read/write)
*/
if((stat(file, &st)) != 0)
{
fprintf(stderr, "[-] unable to run stat() against file: %s: %s\n",
file, strerror(errno));
exit(EXIT_FAILURE);
}
/* Make sure it is a regular file or symbolic link
*/
if(S_ISREG(st.st_mode) != 1 && S_ISLNK(st.st_mode) != 1)
{
fprintf(stderr,
"[-] file: %s is not a regular file or symbolic link.\n",
file
);
return 0;
}
if((st.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != (S_IRUSR|S_IWUSR))
{
fprintf(stderr,
"[-] file: %s permissions should only be user read/write (0600, -rw-------)\n",
file
);
return 0;
}
if(st.st_uid != getuid())
{
fprintf(stderr, "[-] file: %s not owned by current effective user id.\n",
file);
return 0;
}
#endif
return 1;
}
/***EOF***/

13
client/utils.h
View File

@ -31,10 +31,23 @@
#ifndef UTILS_H
#define UTILS_H
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <unistd.h>
#include <errno.h>
#if HAVE_CONFIG_H
#include "config.h"
#endif
/* Prototypes
*/
void hex_dump(const unsigned char *data, const int size);
int set_file_perms(const char *file);
int verify_file_perms_ownership(const char *file);
size_t strlcat(char *dst, const char *src, size_t siz);
size_t strlcpy(char *dst, const char *src, size_t siz);

2
configure.ac
View File

@ -242,7 +242,7 @@ AC_FUNC_MALLOC
AC_FUNC_REALLOC
AC_FUNC_STAT
AC_CHECK_FUNCS([bzero gettimeofday memmove memset socket strchr strcspn strdup strncasecmp strndup strrchr strspn])
AC_CHECK_FUNCS([bzero gettimeofday memmove memset socket strchr strcspn strdup strncasecmp strndup strrchr strspn strnlen stat chmod chown])
AC_SEARCH_LIBS([socket], [socket])
AC_SEARCH_LIBS([inet_addr], [nsl])

2
server/access.c
View File

@ -806,6 +806,8 @@ parse_access_file(fko_srv_options_t *opts)
clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
}
verify_file_perms_ownership(opts->config[CONF_ACCESS_FILE]);
if ((file_ptr = fopen(opts->config[CONF_ACCESS_FILE], "r")) == NULL)
{
fprintf(stderr, "[*] Could not open access file: %s\n",

2
server/config_init.c
View File

@ -200,6 +200,8 @@ parse_config_file(fko_srv_options_t *opts, const char *config_file)
clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
}
verify_file_perms_ownership(config_file);
if ((cfile_ptr = fopen(config_file, "r")) == NULL)
{
fprintf(stderr, "[*] Could not open config file: %s\n",

2
server/fwknopd.c
View File

@ -664,6 +664,8 @@ get_running_pid(const fko_srv_options_t *opts)
char buf[PID_BUFLEN] = {0};
pid_t rpid = 0;
verify_file_perms_ownership(opts->config[CONF_FWKNOP_PID_FILE]);
op_fd = open(opts->config[CONF_FWKNOP_PID_FILE], O_RDONLY);
if(op_fd > 0)

6
server/replay_cache.c
View File

@ -261,10 +261,14 @@ replay_file_cache_init(fko_srv_options_t *opts)
fprintf(digest_file_ptr,
"# <digest> <proto> <src_ip> <src_port> <dst_ip> <dst_port> <time>\n");
fclose(digest_file_ptr);
set_file_perms(opts->config[CONF_DIGEST_FILE]);
return(0);
}
/* File exist, and we have access - create in-memory digest cache
verify_file_perms_ownership(opts->config[CONF_DIGEST_FILE]);
/* File exists, and we have access - create in-memory digest cache
*/
if ((digest_file_ptr = fopen(opts->config[CONF_DIGEST_FILE], "r")) == NULL)
{

72
server/utils.c
View File

@ -147,19 +147,87 @@ dump_ctx(fko_ctx_t ctx)
int
is_valid_dir(const char *path)
{
struct stat st;
#if HAVE_STAT
struct stat st;
/* If we are unable to stat the given dir, then return with error.
*/
if(stat(path, &st) != 0)
return(0);
{
fprintf(stderr, "[-] unable to run stat() directory: %s: %s\n",
path, strerror(errno));
exit(EXIT_FAILURE);
}
if(!S_ISDIR(st.st_mode))
return(0);
#endif /* HAVE_STAT */
return(1);
}
int
set_file_perms(const char *file)
{
int res = 0;
res = chmod(file, S_IRUSR | S_IWUSR);
if(res != 0)
{
fprintf(stderr, "[-] unable to chmod file %s to user read/write: %s\n",
file, strerror(errno));
}
return res;
}
int
verify_file_perms_ownership(const char *file)
{
#if HAVE_STAT
struct stat st;
/* Every file that the fwknop client deals with should be owned
* by the user and permissions set to 600 (user read/write)
*/
if((stat(file, &st)) != 0)
{
fprintf(stderr, "[-] unable to run stat() against file: %s: %s\n",
file, strerror(errno));
exit(EXIT_FAILURE);
}
/* Make sure it is a regular file
*/
if(S_ISREG(st.st_mode) != 1 && S_ISLNK(st.st_mode) != 1)
{
fprintf(stderr,
"[-] file: %s is not a regular file or symbolic link.\n",
file
);
return 0;
}
if((st.st_mode & (S_IRWXU|S_IRWXG|S_IRWXO)) != (S_IRUSR|S_IWUSR))
{
fprintf(stderr,
"[-] file: %s permissions should only be user read/write (0600, -rw-------)\n",
file
);
return 0;
}
if(st.st_uid != getuid())
{
fprintf(stderr, "[-] file: %s not owned by current effective user id\n",
file);
return 0;
}
#endif
return 1;
}
/* Determine if a buffer contains only characters from the base64
* encoding set
*/

2
server/utils.h
View File

@ -62,6 +62,8 @@ void hex_dump(const unsigned char *data, const int size);
char* dump_ctx(fko_ctx_t ctx);
int is_base64(const unsigned char *buf, unsigned short int len);
int is_valid_dir(const char *path);
int set_file_perms(const char *file);
int verify_file_perms_ownership(const char *file);
size_t strlcat(char *dst, const char *src, size_t siz);
size_t strlcpy(char *dst, const char *src, size_t siz);

21
test/test-fwknop.pl
View File

@ -3096,7 +3096,26 @@ sub run_cmd() {
close F;
}
my $rv = ((system "$cmd > $cmd_out 2>&1") >> 8);
### copy original file descriptors (credit: Perl Cookbook)
open OLDOUT, ">&STDOUT";
open OLDERR, ">&STDERR";
### redirect command output
open STDOUT, "> $cmd_out" or die "[*] Could not redirect stdout: $!";
open STDERR, ">&STDOUT" or die "[*] Could not dup stdout: $!";
my $rv = ((system $cmd) >> 8);
close STDOUT or die "[*] Could not close STDOUT: $!";
close STDERR or die "[*] Could not close STDERR: $!";
### restore original filehandles
open STDERR, ">&OLDERR" or die "[*] Could not restore stderr: $!";
open STDOUT, ">&OLDOUT" or die "[*] Could not restore stdout: $!";
### close the old copies
close OLDOUT or die "[*] Could not close OLDOUT: $!";
close OLDERR or die "[*] Could not close OLDERR: $!";
open C, "< $cmd_out" or die "[*] Could not open $cmd_out: $!";
my @cmd_lines = <C>;