diff mbox

posix_spawn_file_actions_addopen needs to copy the path argument (BZ 17048)

Message ID 5398C182.4040906@redhat.com
State New
Headers show

Commit Message

Florian Weimer June 11, 2014, 8:52 p.m. UTC 
POSIX requires that we make a copy, so we allocate a new string and free 
it in posix_spawn_file_actions_destroy.

The reporters (David Reid, Alex Gaynor, and Glyph Lefkowitz) are 
concerned that not the old behavior could result in security 
vulnerabilities in applications, and I agree that this cannot be ruled out.

Comments

Roland McGrath June 11, 2014, 9:01 p.m. UTC | #1 
This looks fine to me except for some trivia.  
Please put the BZ# in the subject line when you have one.

> 2014-06-11  Florian Weimer  <fweimer@redhat.com>
> 
> 	* 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.

These are all one paragraph (i.e. no blank lines in between) when they are
all part of the same change.  Put [BZ #17048] at the top.

> +/* Deallocated the file actions.  */

s/Deallocated/Deallocate/

> +      struct __spawn_action *sa = file_actions->__actions + i;

I always have a mild preference to &foo[i] when that's what you're doing.


Thanks,
Roland
Florian Weimer June 11, 2014, 9:18 p.m. UTC | #2 
On 06/11/2014 11:01 PM, Roland McGrath wrote:
> This looks fine to me except for some trivia.

Thanks, committed with the suggested changes.
Allan McRae June 15, 2014, 1:08 a.m. UTC | #3 
On 12/06/14 07:18, Florian Weimer wrote:
> On 06/11/2014 11:01 PM, Roland McGrath wrote:
>> This looks fine to me except for some trivia.
> 
> Thanks, committed with the suggested changes.
> 

We normally add a news item for fixed CVEs.  How does this sound?

* CVE-2014-4043 The posix_spawn_file_actions_addopen implementation did not
  copy the path argument. This allowed programs to trigger use-after-free
  bugs or other situations where the path is mutated. (Bugzilla #17048).


Allan
Florian Weimer June 16, 2014, 9:11 a.m. UTC | #4 
On 06/15/2014 03:08 AM, Allan McRae wrote:
> On 12/06/14 07:18, Florian Weimer wrote:
>> On 06/11/2014 11:01 PM, Roland McGrath wrote:
>>> This looks fine to me except for some trivia.
>>
>> Thanks, committed with the suggested changes.
>>
>
> We normally add a news item for fixed CVEs.  How does this sound?

We didn't know if this would qualify for a CVE at the time of commit.

> * CVE-2014-4043 The posix_spawn_file_actions_addopen implementation did not
>    copy the path argument. This allowed programs to trigger use-after-free
>    bugs or other situations where the path is mutated. (Bugzilla #17048).

The second sentence seems a bit rough.  Perhaps:

"This allowed programs to cause posix_spawn to deference a dangling 
pointer, or use an unexpected pathname argument if the string was 
modified after the posix_spawn_file_actions_addopen invocation."
diff mbox

Patch

2014-06-11  Florian Weimer  <fweimer@redhat.com>

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

diff --git a/NEWS b/NEWS
index 622cdbf..a76ba96 100644
--- 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.
+  16967, 16977, 16978, 16984, 16990, 17009, 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 47f6242..40800b8 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 4d165aa..16d5b69 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"
+
+/* Deallocated 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 5609e58..861e3b4 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 84cecf2..6cd874a 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)
-- 
1.9.3