This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix writes past the allocated array bounds in execvpe (BZ# 20847)
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.