Re: [PATCH v3] Fix writes past the allocated array bounds in execvpe (BZ#20847)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

If no one opposes it I will commit it shortly.

On 02/12/2016 16:01, Adhemerval Zanella wrote:
> 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);
>>>  }
>>>