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

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

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;

src/system.h

@@ -69,6 +69,7 @@
 
 #include <unistd.h>
 #include <signal.h>
+#include <fcntl.h>
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <monetary.h>
@@ -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 */

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

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