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.

diff --git a/ChangeLog b/ChangeLog
index d86e73963dd9fb5e21b1a28326630337226812aa..3020b9ac232315df362521aeaf85f21cb9926db8 100644 (file)
--- 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-06-11  Chris Metcalf  <cmetcalf@tilera.com>
 
        * sysdeps/unix/sysv/linux/tile/pt-vfork.c: New file.

diff --git a/NEWS b/NEWS
index ca3ef633b01de6a185d33e6c26d21f5940be8cf3..655226d5d5d0726d037cb824733074f74a124bd8 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -19,7 +19,7 @@ Version 2.20
   16791, 16796, 16799, 16800, 16815, 16823, 16824, 16831, 16838, 16849,
   16854, 16876, 16877, 16878, 16882, 16885, 16888, 16890, 16912, 16915,
   16916, 16917, 16922, 16927, 16928, 16932, 16943, 16958, 16965, 16966,
-  16967, 16977, 16978, 16984, 16990, 17009, 17042.
+  16967, 16977, 16978, 16984, 16990, 17009, 17042, 17048.
 
 * The minimum Linux kernel version that this version of the GNU C Library
   can be used with is 2.6.32.

diff --git a/posix/spawn_faction_addopen.c b/posix/spawn_faction_addopen.c
index 47f62425b696a4fdd511b2a057746322eb6518db..40800b8e6e81341501c0fb8a91009529e2048dec 100644 (file)
--- 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 4d165aab01b216b82826bed3339c3fce326c5ece..1b8701071723c6b093933438e293df535bbf6a75 100644 (file)
--- 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 5609e587e1d0bafc0d49b31b2d51d22f7dae7ee0..861e3b47bb583fee26694f09c87af5a7c0bd27d7 100644 (file)
--- 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 84cecf29457b4b8e66c87cf4a1ab711bf54a30d9..6cd874a39af284eb17ce1692ad037c0d79ef0023 100644 (file)
--- 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)