file permissions and client buffer overflow fix
authorMichael Rash <mbr@cipherdyne.org>
Thu, 30 Aug 2012 02:21:43 +0000 (22:21 -0400)
committerMichael Rash <mbr@cipherdyne.org>
Thu, 30 Aug 2012 02:21:43 +0000 (22:21 -0400)
- [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.

13 files changed:
ChangeLog
client/config_init.c
client/fwknop.c
client/utils.c
client/utils.h
configure.ac
server/access.c
server/config_init.c
server/fwknopd.c
server/replay_cache.c
server/utils.c
server/utils.h
test/test-fwknop.pl

index 056d1ef..5e800b0 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -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.
index 19ec13e..9e04bcc 100644 (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.
     */
index 97192b6..3c518da 100644 (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;
 }
 
index 5a60528..1e5ee2f 100644 (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***/
index df618ae..672b8f3 100644 (file)
 #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);
 
index 4a2ae3d..8690d21 100644 (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])
index d1057f8..280e702 100644 (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",
index 70c3e7c..a728cda 100644 (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",
index 5a780b3..ec49d32 100644 (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)
index 2b164e9..227d64c 100644 (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)
     {
index e145ba8..a25bdcf 100644 (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
 */
index 09584e1..03ad62e 100644 (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);
index 5cad5ab..75d150b 100755 (executable)
@@ -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>;