From 7eadce33d03775916d7519eea31a21104bc8794a Mon Sep 17 00:00:00 2001 From: Pierre Pronchery Date: Mon, 27 Aug 2018 18:53:37 +0200 Subject: [PATCH 1/4] Avoid TOCTOU when calling stat() just before fopen() This is the first part of three for Coverity issue 1355235. --- server/access.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/server/access.c b/server/access.c index d2abee91..4b549e5e 100644 --- a/server/access.c +++ b/server/access.c @@ -2343,7 +2343,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 +2351,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) { From cb8632f4db57bf67a56bbc5ffccf6ea5b8404436 Mon Sep 17 00:00:00 2001 From: Pierre Pronchery Date: Mon, 27 Aug 2018 19:39:21 +0200 Subject: [PATCH 2/4] 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 | 2 +- server/access.c | 47 ++++++------------------------------------- server/config_init.c | 20 ++++++------------ server/fwknopd.c | 13 ++++++------ server/replay_cache.c | 9 ++++++--- server/utils.c | 6 +++--- server/utils.h | 2 +- 7 files changed, 30 insertions(+), 69 deletions(-) diff --git a/configure.ac b/configure.ac index 238e7128..b8443c4b 100644 --- a/configure.ac +++ b/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 diff --git a/server/access.c b/server/access.c index 4b549e5e..d5612426 100644 --- a/server/access.c +++ b/server/access.c @@ -27,8 +27,6 @@ * ****************************************************************************** */ -#include - #if HAVE_SYS_SOCKET_H #include #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 diff --git a/server/config_init.c b/server/config_init.c index 58519094..0f519c31 100644 --- a/server/config_init.c +++ b/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++; diff --git a/server/fwknopd.c b/server/fwknopd.c index 2baf9781..8ca58c86 100644 --- a/server/fwknopd.c +++ b/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) { diff --git a/server/replay_cache.c b/server/replay_cache.c index 77a40e7c..834386ec 100644 --- a/server/replay_cache.c +++ b/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: *