From patchwork Mon Nov 21 14:54:55 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 83238 Delivered-To: patch@linaro.org Received: by 10.140.97.165 with SMTP id m34csp1571250qge; Mon, 21 Nov 2016 06:55:23 -0800 (PST) X-Received: by 10.13.233.4 with SMTP id s4mr15472028ywe.192.1479740123652; Mon, 21 Nov 2016 06:55:23 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id u18si4838172ybd.226.2016.11.21.06.55.23 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 21 Nov 2016 06:55:23 -0800 (PST) Received-SPF: pass (google.com: domain of libc-alpha-return-74968-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-74968-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-74968-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:subject:to:references:cc:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=TBkNTwy/MyV6ed6S jdPhAMvg7I5YXeSxrFRz9t36cgvWXKTQZ4XQKzQvSQEaTvOkgMzROr+/cqVErVVE WMq/APAcy71D64/8vU+Tn2ym4MLk9s4DYiuZAnTyElFF2w6sJLCUqL/f4Y+VAqY7 WSUqc07R4eC05ReopjP/WZm7FZM= 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:subject:to:references:cc:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; s=default; bh=tl/Ftk9jwwlD7l+JYoO++Q fhVqU=; b=D4kaPs+iCRpKCbc/maX2tqty5q9Gb2iuv+l2C2Ds3ov3tspRwQ9e6w /vWdH2kZ4paHmzHViWDsjkBsBuekjfanb7A6YQR3DQwC8zxq0BmacyO9w1h+dr1i sk7iY2bebgZWfw6PbqAD6kqYrvBcq62IkBQkv0iwYqT2IB61/vPgk= Received: (qmail 17138 invoked by alias); 21 Nov 2016 14:55:13 -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 17118 invoked by uid 89); 21 Nov 2016 14:55:11 -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=Stefan, Hx-languages-length:3280, argv!, Liebler X-HELO: mail-ua0-f182.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=gRJoZcWl7QmfDnBd3ZiQH8mUY9RkLYLpufkZ94lnLQM=; b=L1U9/bHJV8IEjST+hGwoD5krKiHmrhxTNMp+0V6R3vPvD9PsHhf7yDxDzD3pS3Odh7 0nDOo96O317rRfpn7yeQ50xSL9MN8jT1lvLKDJrvK7588xfcBINhnKEwGyM4ozwddR// kPiP7PZ2enBe+QPWCzOUTnR63IHfk4VbFdfNqF8fh0x0h/zUjb0+zuVdTSXPeihaGdur 7N3W8yGcJtwCy656ah3HmVeoN9i1V0qMLZuz6M828RQJM6BXPqZYPSuFdUvlz1lQdE+8 e+Xl+PS5dY8wS526IDmn0GP4EX6ntHVQ4B2JPJGs0exbM5Ng061ADh/wptuOR0R7eHKi fJwQ== X-Gm-Message-State: AKaTC01p1tPyegw7j/CNA5b6VCQQyWSKDw1OutyJccUYP0gMxQWymxpP8ER64RhL+6b77UOj X-Received: by 10.176.0.169 with SMTP id 38mr8086022uaj.34.1479740099977; Mon, 21 Nov 2016 06:54:59 -0800 (PST) Subject: Re: [PATCH] Fix writes past the allocated array bounds in execvpe (BZ# 20847) To: libc-alpha@sourceware.org References: <1479734322-28206-1-git-send-email-adhemerval.zanella@linaro.org> Cc: Stefan Liebler From: Adhemerval Zanella Message-ID: Date: Mon, 21 Nov 2016 12:54:55 -0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: On 21/11/2016 12:16, Stefan Liebler wrote: > On 11/21/2016 02:18 PM, 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) >> { >> - 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) > Is this fix correct? I think it is incomplete based on your remarks. > memcpy reads behind NULL in argv! > Assume: > argv[0]="tst.sh"; argv[1]="abc"; argv[2]=NULL; > => argc=3 > char *new_argv[3 + 2]; > memcpy (new_argv + 2, argv + 1, 3 * sizeof(char *)) > => > new_argv[0]=_PATH_BSHELL > new_argv[1]=file > new_argv[2]=argv[1]; /* "abc" */ > new_argv[3]=argv[2]; /* NULL */ > new_argv[4]=argv[3]; /* => reads from behind NULL-element in argv! */ > Yes, we need to take only the script arguments without the script itself. Something like: diff --git a/posix/execvpe.c b/posix/execvpe.c index bd535b1..96a12bf5 100644 --- a/posix/execvpe.c +++ b/posix/execvpe.c @@ -54,7 +54,7 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[]) 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;