DeforaNetworks/fwknop
DeforaNetworks/fwknop
3
0
Fork 0
Code Releases Activity

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

Browse Source 
khorben/coverity/1355235 TOCTOU
This commit is contained in:
Michael Rash 2018-08-28 20:36:58 -04:00 committed by GitHub
commit 7ac347fbdd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 66 additions and 88 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
client/config_init.c
View File

@ -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;

25
client/fwknop.c
View File

@ -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';

6
client/utils.c
View File

@ -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
*/

2
client/utils.h
View File

@ -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);

2
configure.ac
View File

@ -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

59
server/access.c
View File

@ -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,16 @@ parse_access_file(fko_srv_options_t *opts, char *access_filename, int *depth)
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
{
fclose(file_ptr);
return EXIT_FAILURE;
}
log_msg(LOG_DEBUG, "Opened access file: %s", access_filename);
/* Initialize the access list
@ -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)
{

24
server/config_init.c
View File

@ -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++;

13
server/fwknopd.c
View File

@ -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)
{

13
server/replay_cache.c
View File

@ -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:

6
server/utils.c
View File

@ -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
*/

2
server/utils.h
View File

@ -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);