Merge pull request #286 from DeforaNetworks/khorben/coverity/1355235-…

…toctou

khorben/coverity/1355235 TOCTOU

client/config_init.c

@@ -696,7 +696,7 @@ set_rc_file(char *rcfile, fko_cli_options_t *options)
      * client consumes a proper rc file with strict permissions set (thanks
      * to Fernando Arnaboldi from IOActive for pointing this out).
     */
-    if(verify_file_perms_ownership(rcfile) != 1)
+    if(verify_file_perms_ownership(rcfile, -1) != 1)
         exit(EXIT_FAILURE);
 
     return;

client/fwknop.c

@@ -893,15 +893,22 @@ show_last_command(const char * const args_save_file)
     char args_str[MAX_LINE_LEN] = {0};
     FILE *args_file_ptr = NULL;
 
-    if(verify_file_perms_ownership(args_save_file) != 1)
-        return 0;
-
     if ((args_file_ptr = fopen(args_save_file, "r")) == NULL) {
         log_msg(LOG_VERBOSITY_ERROR, "Could not open args file: %s",
             args_save_file);
         return 0;
     }
 
+#if HAVE_FILENO
+    if(verify_file_perms_ownership(args_save_file, fileno(args_file_ptr)) != 1)
+#else
+    if(verify_file_perms_ownership(args_save_file, -1) != 1)
+#endif
+    {
+        fclose(args_file_ptr);
+        return 0;
+    }
+
     if ((fgets(args_str, MAX_LINE_LEN, args_file_ptr)) != NULL) {
         log_msg(LOG_VERBOSITY_NORMAL,
                 "Last fwknop client command line: %s", args_str);
@@ -928,15 +935,21 @@ run_last_args(fko_cli_options_t *options, const char * const args_save_file)
 
     memset(argv_new, 0x0, sizeof(argv_new));
 
-    if(verify_file_perms_ownership(args_save_file) != 1)
-        return 0;
-
     if ((args_file_ptr = fopen(args_save_file, "r")) == NULL)
     {
         log_msg(LOG_VERBOSITY_ERROR, "Could not open args file: %s",
                 args_save_file);
         return 0;
     }
+#if HAVE_FILENO
+    if(verify_file_perms_ownership(args_save_file, fileno(args_file_ptr)) != 1)
+#else
+    if(verify_file_perms_ownership(args_save_file, -1) != 1)
+#endif
+    {
+        fclose(args_file_ptr);
+        return 0;
+    }
     if ((fgets(args_str, MAX_LINE_LEN, args_file_ptr)) != NULL)
     {
         args_str[MAX_LINE_LEN-1] = '\0';

client/utils.c

@@ -56,17 +56,17 @@ static fko_protocol_t fko_protocol_array[] =
 };
 
 int
-verify_file_perms_ownership(const char *file)
+verify_file_perms_ownership(const char *file, int fd)
 {
     int res = 1;
 
-#if HAVE_STAT
+#if HAVE_FSTAT && 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)
+    if((fd >= 0 && fstat(fd, &st) == 0) || stat(file, &st) == 0)
     {
         /* Make sure it is a regular file
         */

client/utils.h

@@ -52,7 +52,7 @@
 
 /* Prototypes
 */
-int     verify_file_perms_ownership(const char *file);
+int     verify_file_perms_ownership(const char *file, int fd);
 int     resolve_dst_addr(const char *dns_str, struct addrinfo *hints,
             char *ip_str, size_t ip_bufsize, fko_cli_options_t *opts);
 short   proto_inttostr(int proto, char *proto_str, size_t proto_size);

configure.ac

@@ -400,7 +400,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 strnlen stat lstat chmod chown strlcat strlcpy])
+AC_CHECK_FUNCS([bzero fileno gettimeofday memmove memset socket strchr strcspn strdup strncasecmp strndup strrchr strspn strnlen fstat stat lstat chmod chown strlcat strlcpy])
 
 dnl Decide whether or not to check for the execvp() function
 dnl

server/access.c

@@ -27,8 +27,6 @@
  *
  ******************************************************************************
 */
-#include <sys/stat.h>
-
 #if HAVE_SYS_SOCKET_H
   #include <sys/socket.h>
 #endif
@@ -1494,58 +1492,29 @@ parse_access_file(fko_srv_options_t *opts, char *access_filename, int *depth)
 
     struct passwd  *user_pw = NULL;
     struct passwd  *sudo_user_pw = NULL;
-    struct stat     st;
 
     acc_stanza_t   *curr_acc = NULL;
 
     /* This allows us to limit include depth, and also tracks when we've returned to the root access.conf file.
     */
     (*depth)++;
 
-    /* First see if the access file exists.  If it doesn't, complain
-     * and bail.
-    */
-#if HAVE_LSTAT
-    if(lstat(access_filename, &st) != 0)
+    if ((file_ptr = fopen(access_filename, "r")) == NULL)
     {
-        log_msg(LOG_ERR, "[*] Access file: '%s' was not found.",
+        log_msg(LOG_ERR, "[*] Could not open access file: %s",
             access_filename);
+        perror(NULL);
 
         return EXIT_FAILURE;
     }
-#elif HAVE_STAT
-    if(stat(access_filename, &st) != 0)
-    {
-        log_msg(LOG_ERR, "[*] Access file: '%s' was not found.",
-            access_filename);
 
-        return EXIT_FAILURE;
-    }
+#if HAVE_FILENO
+    if(verify_file_perms_ownership(access_filename, fileno(file_ptr)) != 1)
+#else
+    if(verify_file_perms_ownership(access_filename, -1) != 1)
 #endif
-
-    if(verify_file_perms_ownership(access_filename) != 1)
-        return EXIT_FAILURE;
-
-    /* A note on security here: Coverity flags the following fopen() as a
-     * Time of check time of use (TOCTOU) bug with a low priority due to the
-     * previous stat() call above.  I.e., the access.conf file on disk could
-     * have been changed between the stat() and the fopen() causing a TOCTOU
-     * bug.  While technically this is true, the return value of fopen() is
-     * also checked below so stat() success does not imply we assume fopen()
-     * success.  Also, we could just remove the stat() and
-     * verify_file_perms_ownership() calls above to "fix" the bug, but this
-     * would actually make things easier for an attacker that has already
-     * compromised the local system since access.conf could be changed to, say,
-     * a symbolic link (for which verify_file_perms_ownership() throws a
-     * warning), and then there is no race at all before the fopen().  I.e.
-     * forcing an attacker to do the race makes things harder for them.
-    */
-    if ((file_ptr = fopen(access_filename, "r")) == NULL)
     {
-        log_msg(LOG_ERR, "[*] Could not open access file: %s",
-            access_filename);
-        perror(NULL);
-
+        fclose(file_ptr);
         return EXIT_FAILURE;
     }
 
@@ -2343,7 +2312,6 @@ int
 include_keys_file(acc_stanza_t *curr_acc, const char *access_filename, fko_srv_options_t *opts)
 {
     FILE           *file_ptr;
-    struct stat     st;
     unsigned int    num_lines = 0;
 
     char            access_line_buf[MAX_LINE_LEN] = {0};
@@ -2352,13 +2320,6 @@ include_keys_file(acc_stanza_t *curr_acc, const char *access_filename, fko_srv_o
     char           *ndx;
 
     log_msg(LOG_INFO, "Including key file: '%s'", access_filename);
-    if(stat(access_filename, &st) != 0)
-    {
-        log_msg(LOG_ERR, "[*] Access file: '%s' was not found.",
-            access_filename);
-
-        return EXIT_FAILURE;
-    }
 
     if ((file_ptr = fopen(access_filename, "r")) == NULL)
     {

server/config_init.c

@@ -291,20 +291,6 @@ parse_config_file(fko_srv_options_t *opts, const char *config_file)
     char            tmp1[MAX_LINE_LEN] = {0};
     char            tmp2[MAX_LINE_LEN] = {0};
 
-    struct stat     st;
-
-    /* Make sure the config file exists.
-    */
-    if(stat(config_file, &st) != 0)
-    {
-        log_msg(LOG_ERR, "[*] Config file: '%s' was not found.",
-            config_file);
-        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
-    }
-
-    if(verify_file_perms_ownership(config_file) != 1)
-        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
-
     /* See the comment in the parse_access_file() function regarding security
      * here relative to a TOCTOU bug flagged by Coverity.
     */
@@ -317,6 +303,16 @@ parse_config_file(fko_srv_options_t *opts, const char *config_file)
         clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
+#if HAVE_FILENO
+    if(verify_file_perms_ownership(config_file, fileno(cfile_ptr)) != 1)
+#else
+    if(verify_file_perms_ownership(config_file, -1) != 1)
+#endif
+    {
+        fclose(cfile_ptr);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
+    }
+
     while ((fgets(conf_line_buf, MAX_LINE_LEN, cfile_ptr)) != NULL)
     {
         numLines++;

server/fwknopd.c

@@ -1015,12 +1015,6 @@ get_running_pid(const fko_srv_options_t *opts)
     pid_t   rpid            = 0;
 
 
-    if(verify_file_perms_ownership(opts->config[CONF_FWKNOP_PID_FILE]) != 1)
-    {
-        fprintf(stderr, "verify_file_perms_ownership() error\n");
-        return(rpid);
-    }
-
     op_fd = open(opts->config[CONF_FWKNOP_PID_FILE], O_RDONLY);
 
     if(op_fd == -1)
@@ -1030,6 +1024,13 @@ get_running_pid(const fko_srv_options_t *opts)
         return(rpid);
     }
 
+    if(verify_file_perms_ownership(opts->config[CONF_FWKNOP_PID_FILE], op_fd) != 1)
+    {
+        fprintf(stderr, "verify_file_perms_ownership() error\n");
+        close(op_fd);
+        return(rpid);
+    }
+
     bytes_read = read(op_fd, buf, PID_BUFLEN);
     if (bytes_read > 0)
     {

server/replay_cache.c

@@ -265,9 +265,6 @@ replay_file_cache_init(fko_srv_options_t *opts)
         }
     }
 
-    if(verify_file_perms_ownership(opts->config[CONF_DIGEST_FILE]) != 1)
-        return(-1);
-
     /* File exists, and we have access - create in-memory digest cache
     */
     if ((digest_file_ptr = fopen(opts->config[CONF_DIGEST_FILE], "r")) == NULL)
@@ -277,6 +274,16 @@ replay_file_cache_init(fko_srv_options_t *opts)
         return(-1);
     }
 
+#if HAVE_FILENO
+    if(verify_file_perms_ownership(opts->config[CONF_DIGEST_FILE], fileno(digest_file_ptr)) != 1)
+#else
+    if(verify_file_perms_ownership(opts->config[CONF_DIGEST_FILE], -1) != 1)
+#endif
+    {
+        fclose(digest_file_ptr);
+        return(-1);
+    }
+
     /* Line format:
      * <digest> <proto> <src_ip> <src_port> <dst_ip> <dst_port> <time>
      * Example:

server/utils.c

@@ -109,15 +109,15 @@ is_valid_file(const char *path)
 }
 
 int
-verify_file_perms_ownership(const char *file)
+verify_file_perms_ownership(const char *file, int fd)
 {
-#if HAVE_STAT
+#if HAVE_FSTAT && HAVE_STAT
     struct stat st;
 
     /* Every file that fwknopd deals with should be owned
      * by the user and permissions set to 600 (user read/write)
     */
-    if((stat(file, &st)) == 0)
+    if((fd >= 0 && fstat(fd, &st) == 0) || stat(file, &st) == 0)
     {
         /* Make sure it is a regular file
         */

server/utils.h

@@ -63,7 +63,7 @@ char* dump_ctx(fko_ctx_t ctx);
 int   is_valid_dir(const char *path);
 int   is_valid_exe(const char *path);
 int   is_valid_file(const char *path);
-int   verify_file_perms_ownership(const char *file);
+int   verify_file_perms_ownership(const char *file, int fd);
 void  truncate_partial_line(char *str);
 int   is_digits(const char * const str);