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

On 22/11/2016 08:17, Dominik Vogt wrote:
> On Mon, Nov 21, 2016 at 09:46:22PM +0100, Andreas Schwab wrote:
>> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>
>>> With this change are you ok to push this in?
>>
>> Yes, this is ok.
> 
> No!  The patch writes past the array bounds in the else branch.
> 
> Ciao
> 
> Dominik ^_^  ^_^
> 

Since I made a mistake to push this patch, which I apologize, I think
your previous suggestions is indeed the correct one (patch below).
Reading again, it indeed seems simpler to just account for arguments
and not the final 'NULL' since the 'argv + 1' will indeed ignore
the script name.

On 11/22/2016 02:28 PM, Adhemerval Zanella wrote:
>
>
> On 22/11/2016 08:17, Dominik Vogt wrote:
>> On Mon, Nov 21, 2016 at 09:46:22PM +0100, Andreas Schwab wrote:
>>> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>>
>>>> With this change are you ok to push this in?
>>>
>>> Yes, this is ok.
>>
>> No!  The patch writes past the array bounds in the else branch.
>>
>> Ciao
>>
>> Dominik ^_^  ^_^
>>
>
> Since I made a mistake to push this patch, which I apologize, I think
> your previous suggestions is indeed the correct one (patch below).
> Reading again, it indeed seems simpler to just account for arguments
> and not the final 'NULL' since the 'argv + 1' will indeed ignore
> the script name.
>
> diff --git a/posix/execvpe.c b/posix/execvpe.c
> index 7cdb06a..cf167d0 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)
>         {
> @@ -50,11 +50,11 @@ 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];
> +  char *new_argv[2 + argc];
>    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;
>
>

Hi Adhemerval,

there is an additional special case.
If someone passes an argv array with argv[0] = NULL, then argc is zero 
and new_argv contains two elements but the else path writes NULL to 
new_argv[2], which is past the allocated array bounds.

I don't know if this case is allowed at all.
According to the man-page:
The first argument, by convention, should point to the
filename associated with the file being executed.

Bye
Stefan

On Wed, Nov 23, 2016 at 01:28:16PM +0100, Stefan Liebler wrote:
> On 11/22/2016 02:28 PM, Adhemerval Zanella wrote:
> >diff --git a/posix/execvpe.c b/posix/execvpe.c
> >index 7cdb06a..cf167d0 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)
> >        {
> >@@ -50,11 +50,11 @@ 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];
> >+  char *new_argv[2 + argc];
> >   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;
> >
> >

> there is an additional special case.
> If someone passes an argv array with argv[0] = NULL, then argc is
> zero and new_argv contains two elements but the else path writes
> NULL to new_argv[2], which is past the allocated array bounds.

Sigh, seems to be virtually impossible to get this right.
 
> I don't know if this case is allowed at all.
> According to the man-page:
> The first argument, by convention, should point to the
> filename associated with the file being executed.

Stefan and me tried to understand the Posix spec for the exec
function family, and it's not exactly clear to us whether a NULL
argument list is a usage error or not.  However, since it's
possible to support that case, and funny applications may rely on
it, so __execvpe should probably accept that case.

Maybe fix it with

  char *new_argv[(argc > 0) ? 2 + argc: 3];

?


Ciao

Dominik ^_^  ^_^

On 24/11/2016 09:10, Dominik Vogt wrote:
> On Wed, Nov 23, 2016 at 01:28:16PM +0100, Stefan Liebler wrote:
>> On 11/22/2016 02:28 PM, Adhemerval Zanella wrote:
>>> diff --git a/posix/execvpe.c b/posix/execvpe.c
>>> index 7cdb06a..cf167d0 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)
>>>        {
>>> @@ -50,11 +50,11 @@ 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];
>>> +  char *new_argv[2 + argc];
>>>   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;
>>>
>>>
> 
>> there is an additional special case.
>> If someone passes an argv array with argv[0] = NULL, then argc is
>> zero and new_argv contains two elements but the else path writes
>> NULL to new_argv[2], which is past the allocated array bounds.
> 
> Sigh, seems to be virtually impossible to get this right.
>  
>> I don't know if this case is allowed at all.
>> According to the man-page:
>> The first argument, by convention, should point to the
>> filename associated with the file being executed.
> 
> Stefan and me tried to understand the Posix spec for the exec
> function family, and it's not exactly clear to us whether a NULL
> argument list is a usage error or not.  However, since it's
> possible to support that case, and funny applications may rely on
> it, so __execvpe should probably accept that case.
> 
> Maybe fix it with
> 
>   char *new_argv[(argc > 0) ? 2 + argc: 3];
> 
> ?

This is pretty much what I have proposed in my last patch
iteration [1] (I used your suggestion on accounting the arguments).

POSIX is indeed not specific, but since old execvp{e} implementation
used to support it, I think we should keep supporting it due
compatibility.

[1] https://sourceware.org/ml/libc-alpha/2016-11/msg00832.html