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

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

On 21/11/2016 12:16, Stefan Liebler wrote:
> 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?

I think it is incomplete based on your remarks.

> 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!  */
> 

Yes, we need to take only the script arguments without the script
itself.  Something like:

diff --git a/posix/execvpe.c b/posix/execvpe.c
index bd535b1..96a12bf5 100644
--- a/posix/execvpe.c
+++ b/posix/execvpe.c
@@ -54,7 +54,7 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
   new_argv[0] = (char *) _PATH_BSHELL;
   new_argv[1] = (char *) file;
   if (argc > 1)
-    memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
+    memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *));
   else
     new_argv[2] = NULL;