diff mbox

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

Message ID d9adaf6e-9bf7-b73f-f123-e846ff5388b5@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Nov. 21, 2016, 2:54 p.m. UTC 
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 mbox

Patch

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;