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

[Stefan Liebler <stli at linux dot vnet dot ibm dot com>]

On 11/21/2016 02:18 PM, 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)
>      {
> -      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];
>    new_argv[0] = (char *) _PATH_BSHELL;
>    new_argv[1] = (char *) file;
>    if (argc > 1)
Is this fix correct?
memcpy reads behind NULL in argv!
Assume:
argv[0]="tst.sh"; argv[1]="abc"; argv[2]=NULL;
=> argc=3
char *new_argv[3 + 2];
memcpy (new_argv + 2, argv + 1, 3 * sizeof(char *))
=>
new_argv[0]=_PATH_BSHELL
new_argv[1]=file
new_argv[2]=argv[1]; /* "abc"  */
new_argv[3]=argv[2]; /* NULL  */
new_argv[4]=argv[3]; /* => reads from behind NULL-element in argv!  */