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.
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)
{
"# 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"
fclose(rc);
+ set_file_perms(rcfile);
+
return(0);
}
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.
*/
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)
{
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",
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
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",
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);
+ }
}
}
}
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",
fprintf(args_file_ptr, "%s\n", args_str);
fclose(args_file_ptr);
}
+
+ set_file_perms(args_save_file);
+
return;
}
*
*****************************************************************************
*/
-#include <stdio.h>
-#include <string.h>
#include "utils.h"
/* Generic hex dump function.
}
}
+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***/
#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);
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])
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",
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",
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)
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)
{
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
*/
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);
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>;
projects / fwknop.git / commitdiff