[PATCH] support: Prevent multiple deletion of temporary files

[fweimer at redhat dot com (Florian Weimer)]

Otherwise, another user might recreate these files after the first
deletion.  Particularly with temporary directories, this could result
in the removal of unintended files through symbol link attacks.

2017-05-08  Florian Weimer  <fweimer@redhat.com>

	Prevent multiple deletion of temporary files.
	* support/temp_file.c (struct temp_name_list): Add owner member.
	(add_temp_file): Record owner.
	(support_delete_temp_files): Delete file only if owner matches.
	* posix/tst-exec.c (temp_fd1, temp_fd2): Define.
	(do_prepare): Use create_temp_file instead of add_temp_file.
	Initialize temp_fd1, temp_fd2.
	(do_test): Use global temp_fd1, temp_fd2 variables.  Let the test
	framework remove the temporary files.
	* posix/tst-exec.c (temp_fd1, temp_fd2, temp_fd3): Define.
	(do_prepare): Use create_temp_file instead of add_temp_file.
	Initialize temp_fd1, temp_fd2, temp_fd3.
	(do_test): Use global temp_fd1, temp_fd2, temp_fd3 variables.  Let
	the test framework remove the temporary files.
	* posix/tst-vfork3.c (do_prepare): Adjust for LIFO order of file
	deletion.
	* posix/tst-pathconf.c (do_test): Do not call rmdir on the
	temporary directory.  It is removed by the test framework.
	* dirent/tst-scandir.c (do_test): Likewise.

diff --git a/dirent/tst-scandir.c b/dirent/tst-scandir.c
index 09d19ab..4ecfd7b 100644
--- a/dirent/tst-scandir.c
+++ b/dirent/tst-scandir.c
@@ -293,7 +293,6 @@ do_test (void)
   remove_file (scandir_test_dir, "aa");
   remove_file (scandir_test_dir, "b");
   remove_file (scandir_test_dir, "a");
-  rmdir (scandir_test_dir);
 
   return 0;
 }
diff --git a/posix/tst-exec.c b/posix/tst-exec.c
index be0114e..12a0f57 100644
--- a/posix/tst-exec.c
+++ b/posix/tst-exec.c
@@ -47,6 +47,10 @@ extern int do_test (int argc, char *argv[]);
 static char *name1;
 static char *name2;
 
+/* File descriptors for these temporary files.  */
+static int temp_fd1 = -1;
+static int temp_fd2 = -1;
+
 /* The contents of our files.  */
 static const char fd1string[] = "This file should get closed";
 static const char fd2string[] = "This file should stay opened";
@@ -56,18 +60,14 @@ static const char fd2string[] = "This file should stay opened";
 void
 do_prepare (int argc, char *argv[])
 {
-   size_t name_len;
+  /* We must not open any files in the restart case.  */
+  if (restart)
+    return;
 
-   name_len = strlen (test_dir);
-   name1 = xmalloc (name_len + sizeof ("/execXXXXXX"));
-   mempcpy (mempcpy (name1, test_dir, name_len),
-	    "/execXXXXXX", sizeof ("/execXXXXXX"));
-   add_temp_file (name1);
-
-   name2 = xmalloc (name_len + sizeof ("/execXXXXXX"));
-   mempcpy (mempcpy (name2, test_dir, name_len),
-	    "/execXXXXXX", sizeof ("/execXXXXXX"));
-   add_temp_file (name2);
+  temp_fd1 = create_temp_file ("exec", &name1);
+  temp_fd2 = create_temp_file ("exec", &name2);
+  if (temp_fd1 < 0 || temp_fd2 < 0)
+    exit (1);
 }
 
 
@@ -120,8 +120,6 @@ int
 do_test (int argc, char *argv[])
 {
   pid_t pid;
-  int fd1;
-  int fd2;
   int flags;
   int status;
 
@@ -151,26 +149,18 @@ do_test (int argc, char *argv[])
   /* Prepare the test.  We are creating two files: one which file descriptor
      will be marked with FD_CLOEXEC, another which is not.  */
 
-   /* Open our test files.   */
-   fd1 = mkstemp (name1);
-   if (fd1 == -1)
-     error (EXIT_FAILURE, errno, "cannot open test file `%s'", name1);
-   fd2 = mkstemp (name2);
-   if (fd2 == -1)
-     error (EXIT_FAILURE, errno, "cannot open test file `%s'", name2);
-
    /* Set the bit.  */
-   flags = fcntl (fd1, F_GETFD, 0);
+   flags = fcntl (temp_fd1, F_GETFD, 0);
    if (flags < 0)
      error (EXIT_FAILURE, errno, "cannot get flags");
    flags |= FD_CLOEXEC;
-   if (fcntl (fd1, F_SETFD, flags) < 0)
+   if (fcntl (temp_fd1, F_SETFD, flags) < 0)
      error (EXIT_FAILURE, errno, "cannot set flags");
 
    /* Write something in the files.  */
-   if (write (fd1, fd1string, strlen (fd1string)) != strlen (fd1string))
+   if (write (temp_fd1, fd1string, strlen (fd1string)) != strlen (fd1string))
      error (EXIT_FAILURE, errno, "cannot write to first file");
-   if (write (fd2, fd2string, strlen (fd2string)) != strlen (fd2string))
+   if (write (temp_fd2, fd2string, strlen (fd2string)) != strlen (fd2string))
      error (EXIT_FAILURE, errno, "cannot write to second file");
 
   /* We want to test the `exec' function.  To do this we restart the program
@@ -181,8 +171,8 @@ do_test (int argc, char *argv[])
       char fd1name[18];
       char fd2name[18];
 
-      snprintf (fd1name, sizeof fd1name, "%d", fd1);
-      snprintf (fd2name, sizeof fd2name, "%d", fd2);
+      snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
+      snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
 
       /* This is the child.  Construct the command line.  */
       if (argc == 5)
@@ -205,9 +195,5 @@ do_test (int argc, char *argv[])
     error (EXIT_FAILURE, 0, "Child terminated incorrectly");
   status = WEXITSTATUS (status);
 
-  /* Remove the test files.  */
-  unlink (name1);
-  unlink (name2);
-
   return status;
 }
diff --git a/posix/tst-pathconf.c b/posix/tst-pathconf.c
index cce59e6..88df792 100644
--- a/posix/tst-pathconf.c
+++ b/posix/tst-pathconf.c
@@ -162,11 +162,5 @@ out_nofifo:
       ret = 1;
     }
 
-  if (rmdir (dirbuf) != 0)
-    {
-      printf ("Could not remove directory (%s)\n", strerror (errno));
-      ret = 1;
-    }
-
   return ret;
 }
diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
index 2f960ba..08d92bd 100644
--- a/posix/tst-spawn.c
+++ b/posix/tst-spawn.c
@@ -50,6 +50,11 @@ static char *name1;
 static char *name2;
 static char *name3;
 
+/* Descriptors for the temporary files.  */
+static int temp_fd1 = -1;
+static int temp_fd2 = -1;
+static int temp_fd3 = -1;
+
 /* The contents of our files.  */
 static const char fd1string[] = "This file should get closed";
 static const char fd2string[] = "This file should stay opened";
@@ -60,23 +65,15 @@ static const char fd3string[] = "This file will be opened";
 void
 do_prepare (int argc, char *argv[])
 {
-   size_t name_len;
+  /* We must not open any files in the restart case.  */
+  if (restart)
+    return;
 
-   name_len = strlen (test_dir);
-   name1 = (char *) xmalloc (name_len + sizeof ("/spawnXXXXXX"));
-   mempcpy (mempcpy (name1, test_dir, name_len),
-	    "/spawnXXXXXX", sizeof ("/spawnXXXXXX"));
-   add_temp_file (name1);
-
-   name2 = (char *) xmalloc (name_len + sizeof ("/spawnXXXXXX"));
-   mempcpy (mempcpy (name2, test_dir, name_len),
-	    "/spawnXXXXXX", sizeof ("/spawnXXXXXX"));
-   add_temp_file (name2);
-
-   name3 = (char *) xmalloc (name_len + sizeof ("/spawnXXXXXX"));
-   mempcpy (mempcpy (name3, test_dir, name_len),
-	    "/spawnXXXXXX", sizeof ("/spawnXXXXXX"));
-   add_temp_file (name3);
+  temp_fd1 = create_temp_file ("spawn", &name1);
+  temp_fd2 = create_temp_file ("spawn", &name2);
+  temp_fd3 = create_temp_file ("spawn", &name3);
+  if (temp_fd1 < 0 || temp_fd2 < 0 || temp_fd3 < 0)
+    exit (1);
 }
 
 
@@ -158,9 +155,6 @@ int
 do_test (int argc, char *argv[])
 {
   pid_t pid;
-  int fd1;
-  int fd2;
-  int fd3;
   int fd4;
   int status;
   posix_spawn_file_actions_t actions;
@@ -194,53 +188,42 @@ do_test (int argc, char *argv[])
   /* Prepare the test.  We are creating two files: one which file descriptor
      will be marked with FD_CLOEXEC, another which is not.  */
 
-   /* Open our test files.   */
-   fd1 = mkstemp (name1);
-   if (fd1 == -1)
-     error (EXIT_FAILURE, errno, "cannot open test file `%s'", name1);
-   fd2 = mkstemp (name2);
-   if (fd2 == -1)
-     error (EXIT_FAILURE, errno, "cannot open test file `%s'", name2);
-   fd3 = mkstemp (name3);
-   if (fd3 == -1)
-     error (EXIT_FAILURE, errno, "cannot open test file `%s'", name3);
-
    /* Write something in the files.  */
-   if (write (fd1, fd1string, strlen (fd1string)) != strlen (fd1string))
+   if (write (temp_fd1, fd1string, strlen (fd1string)) != strlen (fd1string))
      error (EXIT_FAILURE, errno, "cannot write to first file");
-   if (write (fd2, fd2string, strlen (fd2string)) != strlen (fd2string))
+   if (write (temp_fd2, fd2string, strlen (fd2string)) != strlen (fd2string))
      error (EXIT_FAILURE, errno, "cannot write to second file");
-   if (write (fd3, fd3string, strlen (fd3string)) != strlen (fd3string))
+   if (write (temp_fd3, fd3string, strlen (fd3string)) != strlen (fd3string))
      error (EXIT_FAILURE, errno, "cannot write to third file");
 
    /* Close the third file.  It'll be opened by `spawn'.  */
-   close (fd3);
+   close (temp_fd3);
 
    /* Tell `spawn' what to do.  */
    if (posix_spawn_file_actions_init (&actions) != 0)
      error (EXIT_FAILURE, errno, "posix_spawn_file_actions_init");
-   /* Close `fd1'.  */
-   if (posix_spawn_file_actions_addclose (&actions, fd1) != 0)
+   /* Close `temp_fd1'.  */
+   if (posix_spawn_file_actions_addclose (&actions, temp_fd1) != 0)
      error (EXIT_FAILURE, errno, "posix_spawn_file_actions_addclose");
    /* We want to open the third file.  */
    name3_copy = strdup (name3);
    if (name3_copy == NULL)
      error (EXIT_FAILURE, errno, "strdup");
-   if (posix_spawn_file_actions_addopen (&actions, fd3, name3_copy,
+   if (posix_spawn_file_actions_addopen (&actions, temp_fd3, name3_copy,
 					 O_RDONLY, 0666) != 0)
      error (EXIT_FAILURE, errno, "posix_spawn_file_actions_addopen");
    /* Overwrite the name to check that a copy has been made.  */
    memset (name3_copy, 'X', strlen (name3_copy));
 
    /* We dup the second descriptor.  */
-   fd4 = MAX (2, MAX (fd1, MAX (fd2, fd3))) + 1;
-   if (posix_spawn_file_actions_adddup2 (&actions, fd2, fd4) != 0)
+   fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
+   if (posix_spawn_file_actions_adddup2 (&actions, temp_fd2, fd4) != 0)
      error (EXIT_FAILURE, errno, "posix_spawn_file_actions_adddup2");
 
    /* Now spawn the process.  */
-   snprintf (fd1name, sizeof fd1name, "%d", fd1);
-   snprintf (fd2name, sizeof fd2name, "%d", fd2);
-   snprintf (fd3name, sizeof fd3name, "%d", fd3);
+   snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
+   snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
+   snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
    snprintf (fd4name, sizeof fd4name, "%d", fd4);
 
    for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
@@ -274,10 +257,5 @@ do_test (int argc, char *argv[])
     error (EXIT_FAILURE, 0, "Child terminated incorrectly");
   status = WEXITSTATUS (status);
 
-  /* Remove the test files.  */
-  unlink (name1);
-  unlink (name2);
-  unlink (name3);
-
   return status;
 }
diff --git a/posix/tst-vfork3.c b/posix/tst-vfork3.c
index c104271..80898b3 100644
--- a/posix/tst-vfork3.c
+++ b/posix/tst-vfork3.c
@@ -159,11 +159,10 @@ do_prepare (void)
   strcpy (stpcpy (script1, tmpdirname), "/script1.sh");
   strcpy (stpcpy (script2, tmpdirname), "/script2.sh");
 
+  add_temp_file (tmpdirname);
   add_temp_file (script0);
   add_temp_file (script1);
   add_temp_file (script2);
-  /* Need to make sure tmpdirname is at the end of the linked list.  */
-  add_temp_file (tmpdirname);
 
   const char content0[] = "#!/bin/sh\necho empty\n";
   create_script (script0, content0, sizeof content0);
diff --git a/support/temp_file.c b/support/temp_file.c
index 50cbae6..fdb2477 100644
--- a/support/temp_file.c
+++ b/support/temp_file.c
@@ -28,12 +28,14 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 
 /* List of temporary files.  */
 static struct temp_name_list
 {
   struct temp_name_list *next;
   char *name;
+  pid_t owner;
 } *temp_name_list;
 
 /* Location of the temporary files.  Set by the test skeleton via
@@ -50,6 +52,7 @@ add_temp_file (const char *name)
     {
       newp->name = newname;
       newp->next = temp_name_list;
+      newp->owner = getpid ();
       temp_name_list = newp;
     }
   else
@@ -94,12 +97,19 @@ support_set_test_dir (const char *path)
 void
 support_delete_temp_files (void)
 {
+  pid_t pid = getpid ();
   while (temp_name_list != NULL)
     {
-      /* For some tests, the temporary file removal runs multiple
-	 times (in the parent processes and the subprocess), so do not
-	 report a failed removal attempt.  */
-      (void) remove (temp_name_list->name);
+      /* Only perform the removal if the path was registed in the same
+	 process, as identified by the PID.  (This assumes that the
+	 parent process which registered the temporary file sticks
+	 around, to prevent PID reuse.)  */
+      if (temp_name_list->owner == pid)
+	{
+	  if (remove (temp_name_list->name) != 0)
+	    printf ("warning: could not remove temporary file: %s: %m\n",
+		    temp_name_list->name);
+	}
       free (temp_name_list->name);
 
       struct temp_name_list *next = temp_name_list->next;