From 17974a1c05c4ffa3ec76c60582d407ee18c7f93a Mon Sep 17 00:00:00 2001 From: Michael Rash Date: Tue, 4 Jun 2013 22:17:59 -0400 Subject: [PATCH] [server] comment additions regarding Coverity low priority TOCTOU issues --- server/access.c | 14 ++++++++++++++ server/config_init.c | 6 ++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/server/access.c b/server/access.c index c619fe40..01619de3 100644 --- a/server/access.c +++ b/server/access.c @@ -935,6 +935,20 @@ parse_access_file(fko_srv_options_t *opts) verify_file_perms_ownership(opts->config[CONF_ACCESS_FILE]); + /* 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(opts->config[CONF_ACCESS_FILE], "r")) == NULL) { fprintf(stderr, "[*] Could not open access file: %s\n", diff --git a/server/config_init.c b/server/config_init.c index 6aebf6b9..e2ee5b15 100644 --- a/server/config_init.c +++ b/server/config_init.c @@ -206,8 +206,7 @@ parse_config_file(fko_srv_options_t *opts, const char *config_file) struct stat st; - /* First see if the config file exists. If it doesn't, complain - * and go on with program defaults. + /* Make sure the config file exists. */ if(stat(config_file, &st) != 0) { @@ -218,6 +217,9 @@ parse_config_file(fko_srv_options_t *opts, const char *config_file) verify_file_perms_ownership(config_file); + /* See the comment in the parse_access_file() function regarding security + * here relative to a TOCTOU bug flagged by Coverity. + */ if ((cfile_ptr = fopen(config_file, "r")) == NULL) { fprintf(stderr, "[*] Could not open config file: %s\n",