| Message ID | 1479755377-12442-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
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."
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."
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);
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(-) -- 2.7.4