From 89fdb5a65e2348b1d20bab53fd4ab19afe9342d1 Mon Sep 17 00:00:00 2001 From: John Zaitseff Date: Mon, 1 Aug 2022 22:23:06 +1000 Subject: [PATCH] Eliminate a TOCTOU bug between data_directory() and xmkdir() Checking the presence of the traditional data directory using stat() leads to a time-of-check / time-of-use bug by the time it is possibly created using xmkdir(). Use file descriptors and openat() to work around this problem. --- NEWS | 6 ++- src/fileio.c | 8 ++-- src/system.h | 10 +++++ src/utils.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++----- src/utils.h | 44 +++++++++++++++++--- 5 files changed, 159 insertions(+), 21 deletions(-) diff --git a/NEWS b/NEWS index 8979d37..9ee96e0 100644 --- a/NEWS +++ b/NEWS @@ -25,8 +25,10 @@ __ https://www.zap.org.au/ Version 7.17 (not yet released) ------------------------------- -Fixed a bug when the ``HOME`` environment variable is empty: use the -current directory instead. +Fixed a TOCTOU (time-of-check / time-of-use) bug that at least in theory +arises between checking for the traditional data directory and creating a +data directory later. Also fixed a bug when the ``HOME`` environment +variable is empty: use the current directory instead. Updated the Brazilian Portuguese, Norwegian Bokmål, Swedish, French, Esperanto, Serbian, German and English translations (in that order), with diff --git a/src/fileio.c b/src/fileio.c index 251fdb0..7ff8550 100644 --- a/src/fileio.c +++ b/src/fileio.c @@ -246,7 +246,7 @@ bool load_game (int num) filename = game_filename(num); assert(filename != NULL); - file = fopen(filename, "r"); + file = game_fopen(num, "r"); if (file == NULL) { // File could not be opened @@ -438,6 +438,7 @@ bool load_game (int num) bool save_game (int num) { const char *data_dir; + int data_dir_fd; char *buf, *encbuf; char *filename; FILE *file; @@ -464,7 +465,8 @@ bool save_game (int num) // Create the data directory, if needed data_dir = data_directory(); - if (data_dir != NULL) { + data_dir_fd = data_directory_fileno(); + if (data_dir != NULL && data_dir_fd == EOF) { if (xmkdir(data_dir, S_IRWXU | S_IRWXG | S_IRWXO) != 0) { // Data directory could not be created saved_errno = errno; @@ -484,7 +486,7 @@ bool save_game (int num) filename = game_filename(num); assert(filename != NULL); - file = fopen(filename, "w"); + file = game_fopen(num, "w"); if (file == NULL) { // File could not be opened for writing saved_errno = errno; diff --git a/src/system.h b/src/system.h index 151c6d1..9ddc50a 100644 --- a/src/system.h +++ b/src/system.h @@ -69,6 +69,7 @@ #include #include +#include #include #include #include @@ -127,5 +128,14 @@ # define __attribute__(x) #endif +// Not all POSIX systems define O_SEARCH: use O_PATH or O_RDONLY instead +#ifndef O_SEARCH +# ifdef O_PATH +# define O_SEARCH O_PATH +# else +# define O_SEARCH O_RDONLY +# endif +#endif + #endif /* included_SYSTEM_H */ diff --git a/src/utils.c b/src/utils.c index 3a30080..ab51b55 100644 --- a/src/utils.c +++ b/src/utils.c @@ -231,6 +231,7 @@ static const unsigned char xor_table[] = { static char *home_directory_str = NULL; // Full pathname to home static char *data_directory_str = NULL; // Writable data dir pathname +static int data_directory_fd = EOF; // Data dir file descriptor static bool is_posix_locale = false; // Override strfmon()? @@ -252,6 +253,18 @@ static bool is_posix_locale = false; // Override strfmon()? static const char *home_directory (void); +/* + Function: game_rel_filename - Generate a relative game filename + Parameters: gamenum - Game number (1-9) as an integer + Returns: char * - Pointer to game filename string + + This function returns a game filename, as a malloc()ed string, relative + to some current directory. If gamenum is outside the range 1 to 9 + (inclusive), NULL is returned. +*/ +extern char *game_rel_filename (int gamenum); + + /* Function: apply_xor - Scramble a buffer using xor_table Parameters: dest - Location of destination buffer @@ -386,8 +399,6 @@ const char *data_directory (void) const char *dirsep_hiddenpath = DIRSEP HIDDEN_PATH; const char *datahome_part = DIRSEP XDG_DATA_DEFAULT DIRSEP; - struct stat statbuf; - assert(strlen(dirsep) == 1); @@ -406,7 +417,8 @@ const char *data_directory (void) strcat(p, dirsep_hiddenpath); strcat(p, program_name); - if (stat(p, &statbuf) == 0 && S_ISDIR(statbuf.st_mode)) { + data_directory_fd = open(p, O_DIRECTORY | O_SEARCH); + if (data_directory_fd != EOF) { // Directory "$HOME/.trader" exists: use that data_directory_str = p; } else { @@ -448,10 +460,48 @@ const char *data_directory (void) } } + if (data_directory_str != NULL && data_directory_fd == EOF) { + data_directory_fd = open(data_directory_str, O_DIRECTORY | O_SEARCH); + } + return data_directory_str; } +/***********************************************************************/ +// data_directory_fileno: Return file descriptor of data directory + +int data_directory_fileno (void) +{ + if (data_directory_fd == EOF) { + (void) data_directory(); + } + + return data_directory_fd; +} + + +/***********************************************************************/ +// game_rel_filename: Generate a relative game filename + +char *game_rel_filename (int gamenum) +{ + /* This implementation assumes a POSIX environment and an ASCII-safe + character encoding. */ + + char *p; + + + if (gamenum < 1 || gamenum > 9) { + return NULL; + } + + p = xmalloc(GAME_FILENAME_BUFSIZE); + snprintf(p, GAME_FILENAME_BUFSIZE, GAME_FILENAME_PROTO, gamenum); + return p; +} + + /***********************************************************************/ // game_filename: Convert an integer to a game filename @@ -462,30 +512,70 @@ char *game_filename (int gamenum) const char *dirsep = DIRSEP; - char buf[GAME_FILENAME_BUFSIZE]; // Buffer for part of filename + char *gfn; // Relative game filename const char *dd; // Data directory - - if (gamenum < 1 || gamenum > 9) { + gfn = game_rel_filename(gamenum); + if (gfn == NULL) { return NULL; } dd = data_directory(); - snprintf(buf, GAME_FILENAME_BUFSIZE, GAME_FILENAME_PROTO, gamenum); - if (dd == NULL) { - return xstrdup(buf); + return gfn; } else { - char *p = xmalloc(strlen(dd) + strlen(dirsep) + strlen(buf) + 1); + char *p = xmalloc(strlen(dd) + strlen(dirsep) + strlen(gfn) + 1); strcpy(p, dd); strcat(p, dirsep); - strcat(p, buf); + strcat(p, gfn); + free(gfn); return p; } } +/***********************************************************************/ +// game_fopen: Open a game file for reading or writing + +FILE *game_fopen (int gamenum, const char *mode) +{ + char *gfn; + int ddfd; + int fd; + int oflag; + mode_t omode; + + + gfn = game_rel_filename(gamenum); + if (gfn == NULL) { + return NULL; + } + + ddfd = data_directory_fileno(); + if (ddfd == EOF) { + ddfd = AT_FDCWD; + } + + if (strcmp(mode, "r") == 0) { + oflag = O_RDONLY; + omode = 0; + } else if (strcmp(mode, "w") == 0) { + oflag = O_WRONLY | O_CREAT | O_TRUNC; + omode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH; + } else { + free(gfn); + errno = EINVAL; + return NULL; + } + + fd = openat(ddfd, gfn, oflag, omode); + free(gfn); + + return (fd == EOF) ? NULL : fdopen(fd, mode); +} + + /************************************************************************ * Error-reporting function definitions * ************************************************************************/ diff --git a/src/utils.h b/src/utils.h index cd7b006..d44825e 100644 --- a/src/utils.h +++ b/src/utils.h @@ -87,7 +87,8 @@ extern void init_program_name (const char *argv0); Returns: const char * - Pointer to data directory This function returns the full pathname to a potentially-writable data - directory, usually within the user's home directory. + directory, usually within the user's home directory. The pointer so + returned must NOT be passed to free(). Assuming program_name is set to "trader", if "$HOME/.trader" exists, that directory is returned as the data directory. Otherwise, if the @@ -96,12 +97,27 @@ extern void init_program_name (const char *argv0); "$HOME/.local/share/trader" is returned. Note that the returned path is NOT created by this function, nor is the - ability to read from or write to this path checked. NULL is returned - if the path cannot be determined. + ability to read from or write to this path checked. However, if the + path already exists, it is opened and used for game_fopen(); this may + be checked by running data_directory_fileno(). NULL is returned if the + path cannot be determined. */ extern const char *data_directory (void); +/* + Function: data_directory_fileno - Return file descriptor of data directory + Parameters: (none) + Returns: int - File descriptor, or EOF on failure + + This function returns the file descriptor of the potentially-writable + data directory returned by data_directory(), or EOF (-1) if that + directory does not exist. Note that this function does NOT create the + data directory itself. +*/ +extern int data_directory_fileno (void); + + /* Function: game_filename - Convert an integer to a game filename Parameters: gamenum - Game number (1-9) as an integer @@ -109,14 +125,32 @@ extern const char *data_directory (void); This function returns the full game filename as a malloc()ed string (ie, a string that must be freed at a later time by calling free()). + This string is to be used for display ONLY, not as a parameter to + fopen(), to avoid issues of TOCTOU (time-of-check / time-of-use). If gamenum is between 1 and 9 inclusive, the string returned is in the - form data_directory() + "/" + GAME_FILENAME(gamenum). If gamenum is - any other integer, NULL is returned. + form DATA_DIRECTORY + "/" + GAME_FILENAME(gamenum). If gamenum is any + other integer, NULL is returned. */ extern char *game_filename (int gamenum); +/* + Function: game_fopen - Open a game file for reading or writing + Parameters: gamenum - Game number (1-9) as an integer + mode - File mode: "r" for reading, "w" for writing + Returns: FILE * - Pointer to open file structure + + This function opens a game file for reading or writing, depending on + the contents of mode, returning a pointer to the open file structure. + Note that the data directory must be created before calling this + function, by checking whether data_directory_fileno() returns EOF: if + so, the directory must be created. NULL is returned (and errno set) if + the file cannot be opened successfully. +*/ +extern FILE *game_fopen (int gamenum, const char *mode); + + /************************************************************************ * Error-reporting function prototypes * ************************************************************************/