This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3] Fix writes past the allocated array bounds in execvpe (BZ#20847)
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Fri, 2 Dec 2016 16:01:39 -0200
- Subject: Re: [PATCH v3] Fix writes past the allocated array bounds in execvpe (BZ#20847)
- Authentication-results: sourceware.org; auth=none
- References: <1479908285-7461-1-git-send-email-adhemerval.zanella@linaro.org> <136a4a3d-d570-4c19-c98b-1d35fa662dd9@linaro.org>
Ping, I think it should be safe to commit this revision since it is what
Dominik has suggested also [1]. I would just like some ack for the test
changes.
[1] https://sourceware.org/ml/libc-alpha/2016-11/msg00887.html
On 28/11/2016 15:56, Adhemerval Zanella wrote:
> Ping.
>
> On 23/11/2016 11:38, Adhemerval Zanella wrote:
>> Commit 6c9e1be87a37bf wrongly fixes BZ#20847 by lefting the else branch
>> on maybe_script_execute to still being able to invalid write on stack
>> allocated buffer. It happens if execvp{e} is executed with an empty
>> arguments list ({ NULL }) and although manual states first argument
>> should be the script name itself, by convention, old and current
>> implementation allows it.
>>
>> This patch fixes the issue by just account for arguments and not the
>> final 'NULL' (since the 'argv + 1' will indeed ignored the script name).
>> The empty argument list is handled in a special case with a minimum
>> allocated size. The patch also adds extra tests for such case in
>> tst-vfork3.
>>
>> Tested on x86_64.
>>
>> [BZ #20847]
>> * posix/execvpe.c (maybe_script_execute): Remove write past allocated
>> array bounds for else branch.
>> (__execvpe): Style fixes.
>> * posix/tst-vfork3.c (run_script): New function.
>> (create_script): Likewise.
>> (do_test): Use run_script internal function.
>> (do_prepare): Use create_script internal function.
>> ---
>> ChangeLog | 12 ++++
>> posix/execvpe.c | 19 ++++--
>> posix/tst-vfork3.c | 185 +++++++++++++++++++----------------------------------
>> 3 files changed, 91 insertions(+), 125 deletions(-)
>>
>> diff --git a/posix/execvpe.c b/posix/execvpe.c
>> index 7cdb06a..a2d0145 100644
>> --- a/posix/execvpe.c
>> +++ b/posix/execvpe.c
>> @@ -38,8 +38,8 @@
>> static void
>> maybe_script_execute (const char *file, char *const argv[], char *const envp[])
>> {
>> - ptrdiff_t argc = 0;
>> - while (argv[argc++] != NULL)
>> + ptrdiff_t argc;
>> + for (argc = 0; argv[argc] != NULL; argc++)
>> {
>> if (argc == INT_MAX - 1)
>> {
>> @@ -48,13 +48,18 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
>> }
>> }
>>
>> - /* Construct an argument list for the shell. It will contain at minimum 3
>> - arguments (current shell, script, and an ending NULL. */
>> - char *new_argv[argc + 1];
>> + /* Construct an argument list for the shell based on original arguments:
>> + 1. Empty list (argv = { NULL }, argc = 1 }: new argv will contain 3
>> + arguments - default shell, script to execute, and ending NULL.
>> + 2. Non empty argument list (argc = { ..., NULL }, argc > 1}: new argv
>> + will contain also the default shell and the script to execute. It
>> + will also skip the script name in arguments and only copy script
>> + arguments. */
>> + char *new_argv[argc > 1 ? 2 + argc : 3];
>> new_argv[0] = (char *) _PATH_BSHELL;
>> new_argv[1] = (char *) file;
>> if (argc > 1)
>> - memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *));
>> + memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
>> else
>> new_argv[2] = NULL;
>>
>> @@ -96,7 +101,7 @@ __execvpe (const char *file, char *const argv[], char *const envp[])
>> size_t path_len = __strnlen (path, PATH_MAX - 1) + 1;
>>
>> /* NAME_MAX does not include the terminating null character. */
>> - if (((file_len-1) > NAME_MAX)
>> + if ((file_len - 1 > NAME_MAX)
>> || !__libc_alloca_cutoff (path_len + file_len + 1))
>> {
>> errno = ENAMETOOLONG;
>> diff --git a/posix/tst-vfork3.c b/posix/tst-vfork3.c
>> index 05edc5a..f5fe5c3 100644
>> --- a/posix/tst-vfork3.c
>> +++ b/posix/tst-vfork3.c
>> @@ -33,84 +33,64 @@ char *tmpdirname;
>> #define PREPARE(argc, argv) do_prepare ()
>> #include "../test-skeleton.c"
>>
>> -static int
>> -do_test (void)
>> +static void
>> +run_script (const char *script, char *const argv[])
>> {
>> - mtrace ();
>> -
>> - const char *path = getenv ("PATH");
>> - if (path == NULL)
>> - path = "/bin";
>> - char pathbuf[strlen (tmpdirname) + 1 + strlen (path) + 1];
>> - strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path);
>> - if (setenv ("PATH", pathbuf, 1) < 0)
>> - {
>> - puts ("setenv failed");
>> - return 1;
>> - }
>> -
>> - size_t i;
>> - char *argv[3] = { (char *) "script1.sh", (char *) "1", NULL };
>> - for (i = 0; i < 5; i++)
>> + for (size_t i = 0; i < 5; i++)
>> {
>> pid_t pid = vfork ();
>> if (pid < 0)
>> - {
>> - printf ("vfork failed: %m\n");
>> - return 1;
>> - }
>> + FAIL_EXIT1 ("vfork failed: %m");
>> else if (pid == 0)
>> {
>> - execvp ("script1.sh", argv);
>> + execvp (script, argv);
>> _exit (errno);
>> }
>> +
>> int status;
>> if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid)
>> - {
>> - puts ("waitpid failed");
>> - return 1;
>> - }
>> + FAIL_EXIT1 ("waitpid failed");
>> else if (status != 0)
>> {
>> if (WIFEXITED (status))
>> - printf ("script1.sh failed with status %d\n",
>> - WEXITSTATUS (status));
>> + FAIL_EXIT1 ("%s failed with status %d\n", script,
>> + WEXITSTATUS (status));
>> else
>> - printf ("script1.sh kill by signal %d\n",
>> - WTERMSIG (status));
>> - return 1;
>> + FAIL_EXIT1 ("%s killed by signal %d\n", script,
>> + WTERMSIG (status));
>> }
>> }
>> +}
>>
>> - argv[0] = (char *) "script2.sh";
>> - argv[1] = (char *) "2";
>> - for (i = 0; i < 5; i++)
>> +static int
>> +do_test (void)
>> +{
>> + mtrace ();
>> +
>> + const char *path = getenv ("PATH");
>> + if (path == NULL)
>> + path = "/bin";
>> + char pathbuf[strlen (tmpdirname) + 1 + strlen (path) + 1];
>> + strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path);
>> + if (setenv ("PATH", pathbuf, 1) < 0)
>> {
>> - pid_t pid = vfork ();
>> - if (pid < 0)
>> - {
>> - printf ("vfork failed: %m\n");
>> - return 1;
>> - }
>> - else if (pid == 0)
>> - {
>> - execvp ("script2.sh", argv);
>> - _exit (errno);
>> - }
>> - int status;
>> - if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid)
>> - {
>> - puts ("waitpid failed");
>> - return 1;
>> - }
>> - else if (status != 0)
>> - {
>> - printf ("script2.sh failed with status %d\n", status);
>> - return 1;
>> - }
>> + puts ("setenv failed");
>> + return 1;
>> }
>>
>> - for (i = 0; i < 5; i++)
>> + char *argv00[] = { NULL };
>> + run_script ("script0.sh", argv00);
>> + char *argv01[] = { (char*) "script0.sh", NULL };
>> + run_script ("script0.sh", argv01);
>> +
>> + char *argv1[] = { (char *) "script1.sh", (char *) "1", NULL };
>> + run_script ("script1.sh", argv1);
>> +
>> + char *argv2[] = { (char *) "script2.sh", (char *) "2", NULL };
>> + run_script ("script2.sh", argv2);
>> +
>> + /* Same as before but with execlp. */
>> + for (size_t i = 0; i < 5; i++)
>> {
>> pid_t pid = vfork ();
>> if (pid < 0)
>> @@ -137,87 +117,56 @@ do_test (void)
>> }
>>
>> unsetenv ("PATH");
>> - argv[0] = (char *) "echo";
>> - argv[1] = (char *) "script 4";
>> - for (i = 0; i < 5; i++)
>> - {
>> - pid_t pid = vfork ();
>> - if (pid < 0)
>> - {
>> - printf ("vfork failed: %m\n");
>> - return 1;
>> - }
>> - else if (pid == 0)
>> - {
>> - execvp ("echo", argv);
>> - _exit (errno);
>> - }
>> - int status;
>> - if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid)
>> - {
>> - puts ("waitpid failed");
>> - return 1;
>> - }
>> - else if (status != 0)
>> - {
>> - printf ("echo failed with status %d\n", status);
>> - return 1;
>> - }
>> - }
>> + char *argv4[] = { (char *) "echo", (char *) "script 4", NULL };
>> + run_script ("echo", argv4);
>>
>> return 0;
>> }
>>
>> static void
>> +create_script (const char *script, const char *contents, size_t size)
>> +{
>> + int fd = open (script, O_WRONLY | O_CREAT, 0700);
>> + if (fd < 0
>> + || TEMP_FAILURE_RETRY (write (fd, contents, size)) != size
>> + || fchmod (fd, S_IRUSR | S_IXUSR) < 0)
>> + FAIL_EXIT1 ("could not write %s\n", script);
>> + close (fd);
>> +}
>> +
>> +static void
>> do_prepare (void)
>> {
>> size_t len = strlen (test_dir) + sizeof ("/tst-vfork3.XXXXXX");
>> tmpdirname = malloc (len);
>> - char *script1 = malloc (len + sizeof "/script1.sh");
>> - char *script2 = malloc (len + sizeof "/script2.sh");
>> - if (tmpdirname == NULL || script1 == NULL || script2 == NULL)
>> - {
>> - puts ("out of memory");
>> - exit (1);
>> - }
>> + if (tmpdirname == NULL)
>> + FAIL_EXIT1 ("out of memory");
>> strcpy (stpcpy (tmpdirname, test_dir), "/tst-vfork3.XXXXXX");
>>
>> tmpdirname = mkdtemp (tmpdirname);
>> if (tmpdirname == NULL)
>> - {
>> - puts ("could not create temporary directory");
>> - exit (1);
>> - }
>> + FAIL_EXIT1 ("could not create temporary directory");
>> +
>> + char script0[len + sizeof "/script0.sh"];
>> + char script1[len + sizeof "/script1.sh"];
>> + char script2[len + sizeof "/script2.sh"];
>>
>> + strcpy (stpcpy (script0, tmpdirname), "/script0.sh");
>> strcpy (stpcpy (script1, tmpdirname), "/script1.sh");
>> strcpy (stpcpy (script2, tmpdirname), "/script2.sh");
>>
>> - /* Need to make sure tmpdirname is at the end of the linked list. */
>> + add_temp_file (script0);
>> add_temp_file (script1);
>> - add_temp_file (tmpdirname);
>> 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);
>>
>> const char content1[] = "#!/bin/sh\necho script $1\n";
>> - int fd = open (script1, O_WRONLY | O_CREAT, 0700);
>> - if (fd < 0
>> - || TEMP_FAILURE_RETRY (write (fd, content1, sizeof content1))
>> - != sizeof content1
>> - || fchmod (fd, S_IRUSR | S_IXUSR) < 0)
>> - {
>> - printf ("Could not write %s\n", script1);
>> - exit (1);
>> - }
>> - close (fd);
>> + create_script (script1, content1, sizeof content1);
>>
>> const char content2[] = "echo script $1\n";
>> - fd = open (script2, O_WRONLY | O_CREAT, 0700);
>> - if (fd < 0
>> - || TEMP_FAILURE_RETRY (write (fd, content2, sizeof content2))
>> - != sizeof content2
>> - || fchmod (fd, S_IRUSR | S_IXUSR) < 0)
>> - {
>> - printf ("Could not write %s\n", script2);
>> - exit (1);
>> - }
>> - close (fd);
>> + create_script (script2, content2, sizeof content2);
>> }
>>