posix_spawn_file_actions_addopen needs to copy the path argument (BZ 17048)

POSIX requires that we make a copy, so we allocate a new string
and free it in posix_spawn_file_actions_destroy.

Reported by David Reid, Alex Gaynor, and Glyph Lefkowitz.  This bug
may have security implications.

6 files changed, 54 insertions, 8 deletions

diff --git a/ChangeLog b/ChangeLog
index d23e631778..e24271c45d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2014-06-11  Florian Weimer  <fweimer@redhat.com>
+
+	[BZ #17048]
+	* posix/spawn_int.h (struct __spawn_action): Make the path string
+	non-const to support deallocation.
+	* posix/spawn_faction_addopen.c
+	(posix_spawn_file_actions_addopen): Make a copy of the pathname.
+	* posix/spawn_faction_destroy.c
+	(posix_spawn_file_actions_destroy): Adjust comment.  Deallocate
+	path in all spawn_do_open actions.
+	* posix/tst-spawn.c (do_test): Exercise the copy operation in
+	posix_spawn_file_actions_addopen.
+
 2014-08-26  Florian Weimer  <fweimer@redhat.com>
 
 	[BZ #17187]
diff --git a/NEWS b/NEWS
index 3832c7c52c..fa6caeb75a 100644
--- a/NEWS
+++ b/NEWS
@@ -11,7 +11,7 @@ Version 2.18.1
 
   14143, 14155, 14547, 14699, 15532, 15427, 15522, 15680, 15723, 15734,
   15735, 15797, 15892, 15895, 15909, 15915, 15917, 15996, 16072, 16150,
-  16414, 16430, 16431, 17137, 17187, 17325.
+  16414, 16430, 16431, 17048, 17137, 17187, 17325.
 
 * Support for powerpc64le has been added.
 
diff --git a/posix/spawn_faction_addopen.c b/posix/spawn_faction_addopen.c
index 93065b3a72..3ff6eb2011 100644
--- a/posix/spawn_faction_addopen.c
+++ b/posix/spawn_faction_addopen.c
@@ -35,17 +35,24 @@ posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions,
   if (fd < 0 || fd >= maxfd)
     return EBADF;
 
+  char *path_copy = strdup (path);
+  if (path_copy == NULL)
+    return ENOMEM;
+
   /* Allocate more memory if needed.  */
   if (file_actions->__used == file_actions->__allocated
       && __posix_spawn_file_actions_realloc (file_actions) != 0)
-    /* This can only mean we ran out of memory.  */
-    return ENOMEM;
+    {
+      /* This can only mean we ran out of memory.  */
+      free (path_copy);
+      return ENOMEM;
+    }
 
   /* Add the new value.  */
   rec = &file_actions->__actions[file_actions->__used];
   rec->tag = spawn_do_open;
   rec->action.open_action.fd = fd;
-  rec->action.open_action.path = path;
+  rec->action.open_action.path = path_copy;
   rec->action.open_action.oflag = oflag;
   rec->action.open_action.mode = mode;
 
diff --git a/posix/spawn_faction_destroy.c b/posix/spawn_faction_destroy.c
index cc1d824bd1..b59e6195a1 100644
--- a/posix/spawn_faction_destroy.c
+++ b/posix/spawn_faction_destroy.c
@@ -18,11 +18,29 @@
 #include <spawn.h>
 #include <stdlib.h>
 
-/* Initialize data structure for file attribute for `spawn' call.  */
+#include "spawn_int.h"
+
+/* Deallocate the file actions.  */
 int
 posix_spawn_file_actions_destroy (posix_spawn_file_actions_t *file_actions)
 {
-  /* Free the memory allocated.  */
+  /* Free the paths in the open actions.  */
+  for (int i = 0; i < file_actions->__used; ++i)
+    {
+      struct __spawn_action *sa = &file_actions->__actions[i];
+      switch (sa->tag)
+	{
+	case spawn_do_open:
+	  free (sa->action.open_action.path);
+	  break;
+	case spawn_do_close:
+	case spawn_do_dup2:
+	  /* No cleanup required.  */
+	  break;
+	}
+    }
+
+  /* Free the array of actions.  */
   free (file_actions->__actions);
   return 0;
 }
diff --git a/posix/spawn_int.h b/posix/spawn_int.h
index 5609e587e1..861e3b47bb 100644
--- a/posix/spawn_int.h
+++ b/posix/spawn_int.h
@@ -22,7 +22,7 @@ struct __spawn_action
     struct
     {
       int fd;
-      const char *path;
+      char *path;
       int oflag;
       mode_t mode;
     } open_action;
diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
index 66fd26bba8..36c3c12fb3 100644
--- a/posix/tst-spawn.c
+++ b/posix/tst-spawn.c
@@ -168,6 +168,7 @@ do_test (int argc, char *argv[])
   char fd2name[18];
   char fd3name[18];
   char fd4name[18];
+  char *name3_copy;
   char *spargv[12];
   int i;
 
@@ -222,9 +223,15 @@ do_test (int argc, char *argv[])
    if (posix_spawn_file_actions_addclose (&actions, fd1) != 0)
      error (EXIT_FAILURE, errno, "posix_spawn_file_actions_addclose");
    /* We want to open the third file.  */
-   if (posix_spawn_file_actions_addopen (&actions, fd3, name3,
+   name3_copy = strdup (name3);
+   if (name3_copy == NULL)
+     error (EXIT_FAILURE, errno, "strdup");
+   if (posix_spawn_file_actions_addopen (&actions, 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)
@@ -253,6 +260,7 @@ do_test (int argc, char *argv[])
    /* Cleanup.  */
    if (posix_spawn_file_actions_destroy (&actions) != 0)
      error (EXIT_FAILURE, errno, "posix_spawn_file_actions_destroy");
+   free (name3_copy);
 
   /* Wait for the child.  */
   if (waitpid (pid, &status, 0) != pid)