Date: Wed, 8 Jul 2020 20:08:05 +0000 (UTC) From: Gordon Tetlow <gordon@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-releng@freebsd.org Subject: svn commit: r363025 - in releng/11.4/lib/libc: gen tests/gen Message-ID: <202007082008.068K85vn024315@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: gordon Date: Wed Jul 8 20:08:05 2020 New Revision: 363025 URL: https://svnweb.freebsd.org/changeset/base/363025 Log: Fix posix_spawnp(3) buffer overflow. Approved by: so Security: FreeBSD-SA-20:18.posix_spawnp Security: CVE-2020-7458 Added: releng/11.4/lib/libc/tests/gen/spawnp_enoexec.sh (contents, props changed) Modified: releng/11.4/lib/libc/gen/exec.c releng/11.4/lib/libc/gen/posix_spawn.c releng/11.4/lib/libc/tests/gen/Makefile releng/11.4/lib/libc/tests/gen/posix_spawn_test.c Modified: releng/11.4/lib/libc/gen/exec.c ============================================================================== --- releng/11.4/lib/libc/gen/exec.c Wed Jul 8 19:58:00 2020 (r363024) +++ releng/11.4/lib/libc/gen/exec.c Wed Jul 8 20:08:05 2020 (r363025) @@ -49,6 +49,9 @@ __FBSDID("$FreeBSD$"); extern char **environ; +static const char execvPe_err_preamble[] = "execvP: "; +static const char execvPe_err_trailer[] = ": path too long\n"; + int execl(const char *name, const char *arg, ...) { @@ -149,8 +152,8 @@ execvPe(const char *name, const char *path, char * con const char **memp; size_t cnt, lp, ln; int eacces, save_errno; - char *cur, buf[MAXPATHLEN]; - const char *p, *bp; + char buf[MAXPATHLEN]; + const char *bp, *np, *op, *p; struct stat sb; eacces = 0; @@ -158,7 +161,7 @@ execvPe(const char *name, const char *path, char * con /* If it's an absolute or relative path name, it's easy. */ if (strchr(name, '/')) { bp = name; - cur = NULL; + op = NULL; goto retry; } bp = buf; @@ -169,34 +172,42 @@ execvPe(const char *name, const char *path, char * con return (-1); } - cur = alloca(strlen(path) + 1); - if (cur == NULL) { - errno = ENOMEM; - return (-1); - } - strcpy(cur, path); - while ((p = strsep(&cur, ":")) != NULL) { + op = path; + ln = strlen(name); + while (op != NULL) { + np = strchrnul(op, ':'); + /* * It's a SHELL path -- double, leading and trailing colons * mean the current directory. */ - if (*p == '\0') { + if (np == op) { + /* Empty component. */ p = "."; lp = 1; - } else - lp = strlen(p); - ln = strlen(name); + } else { + /* Non-empty component. */ + p = op; + lp = np - op; + } + /* Advance to the next component or terminate after this. */ + if (*np == '\0') + op = NULL; + else + op = np + 1; + /* * If the path is too long complain. This is a possible * security issue; given a way to make the path too long * the user may execute the wrong program. */ if (lp + ln + 2 > sizeof(buf)) { - (void)_write(STDERR_FILENO, "execvP: ", 8); + (void)_write(STDERR_FILENO, execvPe_err_preamble, + sizeof(execvPe_err_preamble) - 1); (void)_write(STDERR_FILENO, p, lp); - (void)_write(STDERR_FILENO, ": path too long\n", - 16); + (void)_write(STDERR_FILENO, execvPe_err_trailer, + sizeof(execvPe_err_trailer) - 1); continue; } bcopy(p, buf, lp); @@ -215,14 +226,28 @@ retry: (void)_execve(bp, argv, envp); case ENOEXEC: for (cnt = 0; argv[cnt]; ++cnt) ; - memp = alloca((cnt + 2) * sizeof(char *)); + + /* + * cnt may be 0 above; always allocate at least + * 3 entries so that we can at least fit "sh", bp, and + * the NULL terminator. We can rely on cnt to take into + * account the NULL terminator in all other scenarios, + * as we drop argv[0]. + */ + memp = alloca(MAX(3, cnt + 2) * sizeof(char *)); if (memp == NULL) { /* errno = ENOMEM; XXX override ENOEXEC? */ goto done; } - memp[0] = "sh"; - memp[1] = bp; - bcopy(argv + 1, memp + 2, cnt * sizeof(char *)); + if (cnt > 0) { + memp[0] = argv[0]; + memp[1] = bp; + bcopy(argv + 1, memp + 2, cnt * sizeof(char *)); + } else { + memp[0] = "sh"; + memp[1] = bp; + memp[2] = NULL; + } (void)_execve(_PATH_BSHELL, __DECONST(char **, memp), envp); goto done; Modified: releng/11.4/lib/libc/gen/posix_spawn.c ============================================================================== --- releng/11.4/lib/libc/gen/posix_spawn.c Wed Jul 8 19:58:00 2020 (r363024) +++ releng/11.4/lib/libc/gen/posix_spawn.c Wed Jul 8 20:08:05 2020 (r363025) @@ -28,6 +28,7 @@ __FBSDID("$FreeBSD$"); #include "namespace.h" +#include <sys/param.h> #include <sys/queue.h> #include <sys/wait.h> @@ -202,8 +203,20 @@ struct posix_spawn_args { volatile int error; }; +#define PSPAWN_STACK_ALIGNMENT 16 +#define PSPAWN_STACK_ALIGNBYTES (PSPAWN_STACK_ALIGNMENT - 1) +#define PSPAWN_STACK_ALIGN(sz) \ + (((sz) + PSPAWN_STACK_ALIGNBYTES) & ~PSPAWN_STACK_ALIGNBYTES) + #if defined(__i386__) || defined(__amd64__) +/* + * Below we'll assume that _RFORK_THREAD_STACK_SIZE is appropriately aligned for + * the posix_spawn() case where we do not end up calling _execvpe and won't ever + * try to allocate space on the stack for argv[]. + */ #define _RFORK_THREAD_STACK_SIZE 4096 +_Static_assert((_RFORK_THREAD_STACK_SIZE % PSPAWN_STACK_ALIGNMENT) == 0, + "Inappropriate stack size alignment"); #endif static int @@ -244,10 +257,36 @@ do_posix_spawn(pid_t *pid, const char *path, pid_t p; #ifdef _RFORK_THREAD_STACK_SIZE char *stack; + size_t cnt, stacksz; - stack = malloc(_RFORK_THREAD_STACK_SIZE); + stacksz = _RFORK_THREAD_STACK_SIZE; + if (use_env_path) { + /* + * We need to make sure we have enough room on the stack for the + * potential alloca() in execvPe if it gets kicked back an + * ENOEXEC from execve(2), plus the original buffer we gave + * ourselves; this protects us in the event that the caller + * intentionally or inadvertently supplies enough arguments to + * make us blow past the stack we've allocated from it. + */ + for (cnt = 0; argv[cnt] != NULL; ++cnt) + ; + stacksz += MAX(3, cnt + 2) * sizeof(char *); + stacksz = PSPAWN_STACK_ALIGN(stacksz); + } + + /* + * aligned_alloc is not safe to use here, because we can't guarantee + * that aligned_alloc and free will be provided by the same + * implementation. We've actively hit at least one application that + * will provide its own malloc/free but not aligned_alloc leading to + * a free by the wrong allocator. + */ + stack = malloc(stacksz); if (stack == NULL) return (ENOMEM); + stacksz = (((uintptr_t)stack + stacksz) & ~PSPAWN_STACK_ALIGNBYTES) - + (uintptr_t)stack; #endif psa.path = path; psa.fa = fa; @@ -271,8 +310,7 @@ do_posix_spawn(pid_t *pid, const char *path, * parent. Because of this, we must use rfork_thread instead while * almost every other arch stores the return address in a register. */ - p = rfork_thread(RFSPAWN, stack + _RFORK_THREAD_STACK_SIZE, - _posix_spawn_thr, &psa); + p = rfork_thread(RFSPAWN, stack + stacksz, _posix_spawn_thr, &psa); free(stack); #else p = rfork(RFSPAWN); Modified: releng/11.4/lib/libc/tests/gen/Makefile ============================================================================== --- releng/11.4/lib/libc/tests/gen/Makefile Wed Jul 8 19:58:00 2020 (r363024) +++ releng/11.4/lib/libc/tests/gen/Makefile Wed Jul 8 20:08:05 2020 (r363025) @@ -20,6 +20,15 @@ ATF_TESTS_C+= wordexp_test # TODO: t_siginfo (fixes require further inspection) # TODO: t_sethostname_test (consistently screws up the hostname) +FILESGROUPS+= posix_spawn_test_FILES + +posix_spawn_test_FILES= spawnp_enoexec.sh +posix_spawn_test_FILESDIR= ${TESTSDIR} +posix_spawn_test_FILESMODE= 0755 +posix_spawn_test_FILESOWN= root +posix_spawn_test_FILESGRP= wheel +posix_spawn_test_FILESPACKAGE= ${PACKAGE} + CFLAGS+= -DTEST_LONG_DOUBLE # Not sure why this isn't defined for all architectures, since most Modified: releng/11.4/lib/libc/tests/gen/posix_spawn_test.c ============================================================================== --- releng/11.4/lib/libc/tests/gen/posix_spawn_test.c Wed Jul 8 19:58:00 2020 (r363024) +++ releng/11.4/lib/libc/tests/gen/posix_spawn_test.c Wed Jul 8 20:08:05 2020 (r363025) @@ -93,11 +93,50 @@ ATF_TC_BODY(posix_spawn_no_such_command_negative_test, } } +ATF_TC_WITHOUT_HEAD(posix_spawnp_enoexec_fallback); +ATF_TC_BODY(posix_spawnp_enoexec_fallback, tc) +{ + char buf[FILENAME_MAX]; + char *myargs[2]; + int error, status; + pid_t pid, waitres; + + snprintf(buf, sizeof(buf), "%s/spawnp_enoexec.sh", + atf_tc_get_config_var(tc, "srcdir")); + myargs[0] = buf; + myargs[1] = NULL; + error = posix_spawnp(&pid, myargs[0], NULL, NULL, myargs, myenv); + ATF_REQUIRE(error == 0); + waitres = waitpid(pid, &status, 0); + ATF_REQUIRE(waitres == pid); + ATF_REQUIRE(WIFEXITED(status) && WEXITSTATUS(status) == 42); +} + +ATF_TC_WITHOUT_HEAD(posix_spawnp_enoexec_fallback_null_argv0); +ATF_TC_BODY(posix_spawnp_enoexec_fallback_null_argv0, tc) +{ + char buf[FILENAME_MAX]; + char *myargs[1]; + int error, status; + pid_t pid, waitres; + + snprintf(buf, sizeof(buf), "%s/spawnp_enoexec.sh", + atf_tc_get_config_var(tc, "srcdir")); + myargs[0] = NULL; + error = posix_spawnp(&pid, buf, NULL, NULL, myargs, myenv); + ATF_REQUIRE(error == 0); + waitres = waitpid(pid, &status, 0); + ATF_REQUIRE(waitres == pid); + ATF_REQUIRE(WIFEXITED(status) && WEXITSTATUS(status) == 42); +} + ATF_TP_ADD_TCS(tp) { ATF_TP_ADD_TC(tp, posix_spawn_simple_test); ATF_TP_ADD_TC(tp, posix_spawn_no_such_command_negative_test); + ATF_TP_ADD_TC(tp, posix_spawnp_enoexec_fallback); + ATF_TP_ADD_TC(tp, posix_spawnp_enoexec_fallback_null_argv0); return (atf_no_error()); } Added: releng/11.4/lib/libc/tests/gen/spawnp_enoexec.sh ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ releng/11.4/lib/libc/tests/gen/spawnp_enoexec.sh Wed Jul 8 20:08:05 2020 (r363025) @@ -0,0 +1,4 @@ +# $FreeBSD$ +# Intentionally no interpreter + +exit 42
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202007082008.068K85vn024315>