Avoid TOCTOU by using fstat() after open()

This also needs fileno() after fopen().

This is the second part of three for Coverity issue 1355235.

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,7 +1492,6 @@ 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;
 
@@ -1502,44 +1499,6 @@ parse_access_file(fko_srv_options_t *opts, char *access_filename, int *depth)
     */
     (*depth)++;
 
-    /* First see if the access file exists.  If it doesn't, complain
-     * and bail.
-    */
-#if HAVE_LSTAT
-    if(lstat(access_filename, &st) != 0)
-    {
-        log_msg(LOG_ERR, "[*] Access file: '%s' was not found.",
-            access_filename);
-
-        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;
-    }
-#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",
@@ -1549,6 +1508,12 @@ parse_access_file(fko_srv_options_t *opts, char *access_filename, int *depth)
         return EXIT_FAILURE;
     }
 
+    if(verify_file_perms_ownership(access_filename, fileno(file_ptr)) != 1)
+    {
+        fclose(file_ptr);
+        return EXIT_FAILURE;
+    }
+
     log_msg(LOG_DEBUG, "Opened access file: %s", access_filename);
 
     /* Initialize the access list

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,12 @@ parse_config_file(fko_srv_options_t *opts, const char *config_file)
         clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
+    if(verify_file_perms_ownership(config_file, fileno(cfile_ptr)) != 1)
+    {
+        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,12 @@ replay_file_cache_init(fko_srv_options_t *opts)
         return(-1);
     }
 
+    if(verify_file_perms_ownership(opts->config[CONF_DIGEST_FILE], fileno(digest_file_ptr)) != 1)
+    {
+        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
     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(fstat(fd, &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);