From owner-svn-src-all@freebsd.org Wed Jul 8 20:08:06 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 7890936DAB1; Wed, 8 Jul 2020 20:08:06 +0000 (UTC) (envelope-from gordon@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4B29N62fk5z3bt6; Wed, 8 Jul 2020 20:08:06 +0000 (UTC) (envelope-from gordon@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 3E823256D6; Wed, 8 Jul 2020 20:08:06 +0000 (UTC) (envelope-from gordon@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 068K86OB024319; Wed, 8 Jul 2020 20:08:06 GMT (envelope-from gordon@FreeBSD.org) Received: (from gordon@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 068K85vn024315; Wed, 8 Jul 2020 20:08:05 GMT (envelope-from gordon@FreeBSD.org) Message-Id: <202007082008.068K85vn024315@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: gordon set sender to gordon@FreeBSD.org using -f From: Gordon Tetlow Date: Wed, 8 Jul 2020 20:08:05 +0000 (UTC) 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 X-SVN-Group: releng X-SVN-Commit-Author: gordon X-SVN-Commit-Paths: in releng/11.4/lib/libc: gen tests/gen X-SVN-Commit-Revision: 363025 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Jul 2020 20:08:06 -0000 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 #include #include @@ -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