From 12565bd6138d6d5c0f6f615c55a2609d000e5ed0 Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Wed, 11 Jun 2014 23:12:52 +0200 Subject: 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. --- ChangeLog | 13 +++++++++++++ NEWS | 2 +- posix/spawn_faction_addopen.c | 13 ++++++++++--- posix/spawn_faction_destroy.c | 22 ++++++++++++++++++++-- posix/spawn_int.h | 2 +- posix/tst-spawn.c | 10 +++++++++- 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 + + [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 [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 #include -/* 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) -- cgit 1.4.1