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 11:33, Andreas Schwab wrote:
> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> 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.
> 
> Did the bugs already exist before commit 1eb8930608?

For first issue I see so, since it allocates the argument list as:

 64           /* Count the arguments.  */
 65           int argc = 0;
 66           while (argv[argc++])
 67             ;
 68           size_t len = (argc + 1) * sizeof (char *);
 69           char **script_argv;
 70           void *ptr = NULL;
 71           if (__libc_use_alloca (len))
 72             script_argv = alloca (len);
 73           else
 74             script_argv = ptr = malloc (len);

Taking in consideration only argument list plus one but then writing
argument list plus 2 position on 'scripts_argv'.  The old implementation
does not fail on tst-vfork3 with newer GCC and I did not investigate why.

For second issue, old implementation seems to get the correct size:

 87       size_t pathlen;
 88       size_t alloclen = 0;
 89       char *path = getenv ("PATH");
 90       if (path == NULL)
 91         {
 92           pathlen = confstr (_CS_PATH, (char *) NULL, 0);
 93           alloclen = pathlen + 1;
 94         }
 95       else
 96         pathlen = strlen (path);
 97 
 98       size_t len = strlen (file) + 1;
 99       alloclen += pathlen + len + 1;
100 
101       char *name;
102       char *path_malloc = NULL;
103       if (__libc_use_alloca (alloclen))
104         name = alloca (alloclen);
105       else
106         {
107           path_malloc = name = malloc (alloclen);
108           if (name == NULL)
109             return -1;
110         }

It calculates the final buffer to pass on execve as path length (without
'\0') plus executable length plus 1 for final '\0' and an extra 1 for
'/'.

> 
>> +  if (((file_len-1) > NAME_MAX)
> 
> Spaces around operator and remove the redundant parens.

Ack, I will change it commit.