From patchwork Mon Nov 21 13:18:42 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella X-Patchwork-Id: 83229 Delivered-To: patch@linaro.org Received: by 10.140.97.165 with SMTP id m34csp1516726qge; Mon, 21 Nov 2016 05:19:11 -0800 (PST) X-Received: by 10.13.216.136 with SMTP id a130mr15031493ywe.5.1479734351256; Mon, 21 Nov 2016 05:19:11 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id z66si4723593ybh.72.2016.11.21.05.19.10 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 21 Nov 2016 05:19:11 -0800 (PST) Received-SPF: pass (google.com: domain of libc-alpha-return-74954-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org; spf=pass (google.com: domain of libc-alpha-return-74954-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-74954-patch=linaro.org@sourceware.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id; q=dns; s= default; b=aVLz26dMHpafjJW25cGfBG0iAHSB/pGbAo08aAst9mkZqwBWI/ZnK ZmALvTzIeXJLeUh7CNnOBghu3fLtyO6ByCGz7BS28SxA7ZaoKTIAmabodII+S8f0 BOotVUj1c+3PP36S8TioSyQrPWZlr/3KepRtGwymhSk9FTfqh/ZLjI= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id; s=default; bh=XgTqILylax3li2DataSGubK0Fc0=; b=Jf10ytrfS82cNXYNRAl3rk/8QDRv Wy+L2c4iD1AEgVsdCIqI0BZ8NVbDIfXza8MeEL9OhJA3KRR4huAL397Wys37vcDR B1m+2lKjCuqOK876a6MO5uGB+1+Zz0ftImj0U4lggo/pJgK0JG3NT7Wt5WMj8+pq t3rNYV37HmNpA0o= Received: (qmail 12699 invoked by alias); 21 Nov 2016 13:19:01 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 12665 invoked by uid 89); 21 Nov 2016 13:19:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=103, 6, 1036, __strnlen, enforce X-HELO: mail-vk0-f52.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:date:message-id; bh=BgGaKPmVG1mS2kfsc16/wXGYSXEyyXKESN1tyLw3zwk=; b=JIvIf5kDHyamq1SOaZkzDzM9qSYYJ79Zs0O0ad//WcmMivdZRkeRqfFjg2DFeu5HKF cMEEb5N/OfyAv82SX1SrqdMSY1qlO0qbUTlmFJEpBQEW4WuttpzfYqb6Glajl7O/fZfi 3xK4Zk0AmRHqYDDUlm0UjyEnpy+E8fR0OOCX8h7MA9C6xTEayndk7/adyQFfWzk8PUzB SEPjfp+WHR7OYiWpIyL7RqGluXfpNGoMqPO1f8xLeagxA/YlqJjL7BhQu2lsw3fXeGDp RaBIQSeuGN/n/oiW8Oy3yeeYGEj4mO2sR0MqjVfV4rPb2AGrQMge++nRdf0FAklm1grW QZ/A== X-Gm-Message-State: AKaTC01PEPmE0HlwcIvs1W5go+RL/QZKT5So6IP8HWdwg4xRVBIJXIEqR+f03bKzejPEzEDX X-Received: by 10.31.173.144 with SMTP id w138mr6606102vke.153.1479734328975; Mon, 21 Nov 2016 05:18:48 -0800 (PST) From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [PATCH] Fix writes past the allocated array bounds in execvpe (BZ# 20847) Date: Mon, 21 Nov 2016 11:18:42 -0200 Message-Id: <1479734322-28206-1-git-send-email-adhemerval.zanella@linaro.org> 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(-) -- 2.7.4 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) { - 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) @@ -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);