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

[Dominik Vogt <vogt at linux dot vnet dot ibm dot com>]

On Mon, Nov 21, 2016 at 11:18:42AM -0200, Adhemerval Zanella wrote:
> This patch fixes an invalid write out or stack allocated buffer in
> 2 places at execvpe implementation:
> 
>   1. On 'maybe_script_execute' function where it allocates the new
>      argument list and it does not account that a minimum of argc
>      plus 3 elements (default shell path, script name, arguments,
>      and ending null pointer) should be considered.  The straightforward
>      fix is just to take account of the correct list size.
> 
>   2. On '__execvpe' where the executable file name lenght may not
>      account for ending '\0' and thus subsequent path creation may
>      write past array bounds because it requires to add the terminating
>      null.  The fix is to change how to calculate the executable name
>      size to add the final '\0' and adjust the rest of the code
>      accordingly.
> 
> As described in GCC bug report 78433 [1], these issues were masked off by
> GCC because it allocated several bytes more than necessary so that many
> off-by-one bugs went unnoticed.
> 
> Checked on x86_64 with a latest GCC (7.0.0 20161121) with -O3 on CFLAGS.
> 
> 	* posix/execvpe.c (maybe_script_execute): Remove write past allocated
> 	array bounds.
> 	(__execvpe): Likewise.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78433
> ---
>  ChangeLog       |  7 +++++++
>  posix/execvpe.c | 17 +++++++++++------
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/posix/execvpe.c b/posix/execvpe.c
> index d933f9c..bd535b1 100644
> --- a/posix/execvpe.c
> +++ b/posix/execvpe.c
> @@ -41,15 +41,16 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
>    ptrdiff_t argc = 0;
>    while (argv[argc++] != NULL)

This loop is broken.  It calculates the wrong value; if there are
three arguments, argc will be 4. (See patch below).

>      {
> -      if (argc == INT_MAX - 1)
> +      if (argc == INT_MAX - 2)
>  	{
>  	  errno = E2BIG;
>  	  return;
>  	}
>      }
>  
> -  /* Construct an argument list for the shell.  */
> -  char *new_argv[argc + 1];
> +  /* 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 + 2];

This doesn't fix all problems.  The memcpy below still copies junk
from argv[1 + argc], which is past the end of argv, to new_argv.
Fixing the loop fixes this problem.

>    new_argv[0] = (char *) _PATH_BSHELL;
>    new_argv[1] = (char *) file;
>    if (argc > 1)

> @@ -91,10 +92,11 @@ __execvpe (const char *file, char *const argv[], char *const envp[])
>    /* Although GLIBC does not enforce NAME_MAX, we set it as the maximum
>       size to avoid unbounded stack allocation.  Same applies for
>       PATH_MAX.  */
> -  size_t file_len = __strnlen (file, NAME_MAX + 1);
> +  size_t file_len = __strnlen (file, NAME_MAX) + 1;

I think the existing buffer size calculation in __execvpe() is
fine after all, because of the "+ 1" in the next line:

>    size_t path_len = __strnlen (path, PATH_MAX - 1) + 1;
                                                     ^^^^

>  
> -  if ((file_len > NAME_MAX)
> +  /* NAME_MAX does not include the terminating null character.  */
> +  if (((file_len-1) > NAME_MAX)
>        || !__libc_alloca_cutoff (path_len + file_len + 1))
>      {
>        errno = ENAMETOOLONG;
> @@ -103,6 +105,9 @@ __execvpe (const char *file, char *const argv[], char *const envp[])
>  
>    const char *subp;
>    bool got_eacces = false;
> +  /* The resulting string maximum size would be potentially a entry
> +     in PATH plus '/' (path_len + 1) and then the the resulting file name
> +     plus '\0' (file_len since it already accounts for the '\0').  */
>    char buffer[path_len + file_len + 1];
>    for (const char *p = path; ; p = subp)
>      {
> @@ -123,7 +128,7 @@ __execvpe (const char *file, char *const argv[], char *const envp[])
>           execute.  */
>        char *pend = mempcpy (buffer, p, subp - p);
>        *pend = '/';
> -      memcpy (pend + (p < subp), file, file_len + 1);
> +      memcpy (pend + (p < subp), file, file_len);
>  
>        __execve (buffer, argv, envp);

Alternative change:

--- snip ---
--- a/posix/execvpe.c
+++ b/posix/execvpe.c
@@ -38,10 +38,10 @@
 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)
       	{
@@ -49,7 +49,7 @@ maybe_script_execute (const char *file, char
*const argv[], ch
     }

   /* Construct an argument list for the shell.  */
-  char *new_argv[argc + 1];
+  char *new_argv[2 + argc];
   new_argv[0] = (char *) _PATH_BSHELL;
   new_argv[1] = (char *) file;
   if (argc > 1)
--- snip ---

(Note the reversed order from "argc + 1" to "2 + argc" which is
supposed to make it clear what the "2" is for.  Or maybe even

   2 + argc - 1 + 1
  ^^^        ^^^ ^^^
   |          |   |___ terminating NULL
   |          |_______ all but one elements of argv copied
   |__________________ first two elements "_PATH_SHELL" and "file"

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany