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

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

[Adhemerval Zanella]

Changes from previous version:

 - Fix memcpy write out the bound in maybe_script_execute.

--

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.

	[BZ #20847]
	* 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 | 19 ++++++++++++-------
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/posix/execvpe.c b/posix/execvpe.c
index d933f9c..96a12bf5 100644
--- a/posix/execvpe.c
+++ b/posix/execvpe.c
@@ -41,19 +41,20 @@ 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)
-    memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
+    memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *));
   else
     new_argv[2] = NULL;
 
@@ -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;
   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);
 
-- 
2.7.4

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

[Andreas Schwab]

On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> diff --git a/posix/execvpe.c b/posix/execvpe.c
> index d933f9c..96a12bf5 100644
> --- a/posix/execvpe.c
> +++ b/posix/execvpe.c
> @@ -41,19 +41,20 @@ 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];

The array is now always one element too big, unless execvpe was called
with argv[0] == NULL.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Adhemerval Zanella]

On 21/11/2016 18:10, Andreas Schwab wrote:
> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
>> diff --git a/posix/execvpe.c b/posix/execvpe.c
>> index d933f9c..96a12bf5 100644
>> --- a/posix/execvpe.c
>> +++ b/posix/execvpe.c
>> @@ -41,19 +41,20 @@ 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];
> 
> The array is now always one element too big, unless execvpe was called
> with argv[0] == NULL.

You are right, I keep forgetting 'argc' in this context also contains the
script name itself.  The memcpy adjustment is indeed the only fix required
for this part:

diff --git a/posix/execvpe.c b/posix/execvpe.c
index d933f9c..7cdb06a 100644
--- a/posix/execvpe.c
+++ b/posix/execvpe.c
@@ -48,12 +48,13 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
        }
     }

-  /* Construct an argument list for the shell.  */
+  /* 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];
   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;

With this change are you ok to push this in?
 

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

[Andreas Schwab]

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.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Dominik Vogt]

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 ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

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

[Adhemerval Zanella]

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;
 

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

[Andreas Schwab]

On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> @@ -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;
>    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)

Spaces around operator and remove redundant parens.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Stefan Liebler]

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

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

[Dominik Vogt]

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 ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

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

[Adhemerval Zanella]

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