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 13:51, Dominik Vogt wrote:
> On Mon, Nov 21, 2016 at 11:18:42AM -0200, 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)
>
> This loop is broken. It calculates the wrong value; if there are
> three arguments, argc will be 4. (See patch below).
As Andreas pointed out this is intended so memcpy can copy the
terminating null pointer.
>
>> {
>> - 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];
>
> This doesn't fix all problems. The memcpy below still copies junk
> from argv[1 + argc], which is past the end of argv, to new_argv.
> Fixing the loop fixes this problem.
Yes and Stefan Liebler pointed out earlier and I added an
extra snippet to handle it [1]
[1] https://sourceware.org/ml/libc-alpha/2016-11/msg00721.html.
>
>> new_argv[0] = (char *) _PATH_BSHELL;
>> new_argv[1] = (char *) file;
>> if (argc > 1)
>
>> @@ -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;
>
> I think the existing buffer size calculation in __execvpe() is
> fine after all, because of the "+ 1" in the next line:
>
>> size_t path_len = __strnlen (path, PATH_MAX - 1) + 1;
> ^^^^
>
This addresses only the '/' that will appended after path handling
in the loop. The extra '\0' for the program executable name still
need to be added and '__strnlen (file, NAME_MAX + 1)' potentially
does not take it in account.
That's why I have a added a comment before the buffer allocation
explaining from where the allocation came from.
>>
>> - 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);
>
> Alternative change:
>
> --- snip ---
> --- a/posix/execvpe.c
> +++ b/posix/execvpe.c
> @@ -38,10 +38,10 @@
> 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)
> {
> @@ -49,7 +49,7 @@ maybe_script_execute (const char *file, char
> *const argv[], ch
> }
>
> /* Construct an argument list for the shell. */
> - char *new_argv[argc + 1];
> + char *new_argv[2 + argc];
> new_argv[0] = (char *) _PATH_BSHELL;
> new_argv[1] = (char *) file;
> if (argc > 1)
> --- snip ---
>
> (Note the reversed order from "argc + 1" to "2 + argc" which is
> supposed to make it clear what the "2" is for. Or maybe even
>
> 2 + argc - 1 + 1
> ^^^ ^^^ ^^^
> | | |___ terminating NULL
> | |_______ all but one elements of argv copied
> |__________________ first two elements "_PATH_SHELL" and "file"
>
> Ciao
>
> Dominik ^_^ ^_^
>