From owner-svn-src-head@freebsd.org Sun Mar 12 17:42:00 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2DE21D09AD1 for ; Sun, 12 Mar 2017 17:42:00 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from lauren.room52.net (unknown [IPv6:2001:19f0:5801:2ba:5400:ff:fe59:15d6]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id BB68D1C3E; Sun, 12 Mar 2017 17:41:59 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from lgwl-lstewart2.corp.netflix.com (c110-22-60-167.eburwd6.vic.optusnet.com.au [110.22.60.167]) by lauren.room52.net (Postfix) with ESMTPSA id B22B0C96; Mon, 13 Mar 2017 04:41:54 +1100 (AEDT) Subject: Re: svn commit: r314780 - head/lib/libpam/modules/pam_exec To: Pedro Giffuni , src-committers@FreeBSD.org, =?UTF-8?Q?Dag-Erling_Sm=c3=b8rgrav?= , svn-src-head@FreeBSD.org References: <201703061545.v26FjkNI027057@repo.freebsd.org> <739617a4-3eed-28d1-73e4-86d25d6d5fed@freebsd.org> <1839903b-fb05-bf3f-17bb-697afca9ecb7@FreeBSD.org> From: Lawrence Stewart Message-ID: Date: Mon, 13 Mar 2017 04:40:29 +1100 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1839903b-fb05-bf3f-17bb-697afca9ecb7@FreeBSD.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 12 Mar 2017 17:42:00 -0000 On 13/03/2017 04:30, Pedro Giffuni wrote: > > > On 3/12/2017 12:14 PM, Lawrence Stewart wrote: >> Hi Pedro, >> >> On 07/03/2017 02:45, Pedro F. Giffuni wrote: >>> Author: pfg >>> Date: Mon Mar 6 15:45:46 2017 >>> New Revision: 314780 >>> URL: https://svnweb.freebsd.org/changeset/base/314780 >>> >>> Log: >>> libpam: extra bounds checking through reallocarray(3). >>> Reviewed by: des >>> MFC after: 1 week >>> >>> Modified: >>> head/lib/libpam/modules/pam_exec/pam_exec.c >>> >>> Modified: head/lib/libpam/modules/pam_exec/pam_exec.c >>> ============================================================================== >>> >>> --- head/lib/libpam/modules/pam_exec/pam_exec.c Mon Mar 6 >>> 15:42:03 2017 (r314779) >>> +++ head/lib/libpam/modules/pam_exec/pam_exec.c Mon Mar 6 >>> 15:45:46 2017 (r314780) >>> @@ -138,7 +138,7 @@ _pam_exec(pam_handle_t *pamh __unused, >>> nitems = sizeof(env_items) / sizeof(*env_items); >>> /* Count PAM return values put in the environment. */ >>> nitems_rv = options->return_prog_exit_status ? PAM_RV_COUNT : 0; >>> - tmp = realloc(envlist, (envlen + nitems + 1 + nitems_rv + 1) * >>> + tmp = reallocarray(envlist, envlen + nitems + 1 + nitems_rv + 1, >>> sizeof(*envlist)); >>> if (tmp == NULL) { >>> openpam_free_envlist(envlist); >>> >> This commit breaks pam_exec for me... without this change I see the >> expected PAM_* environment variables from my execed script, but with >> this change I no longer see any of them. > Thanks for the report. > > It seems strange this can cause any failure. Perhaps there is a latent > overflow here and we have been living with it? I will revert while it is > investigated. > > BTW, the "nitems" variable may conflict with nitems() in sys/param.h. I don't think so. I manually ran the compile step in /usr/src/lib/libpam/modules/pam_exec replacing -o with -E per: cc -DOPENPAM_STATIC_MODULES -O2 -pipe -I/usr/src/contrib/openpam/include -I/usr/src/lib/libpam -DOPENPAM_DEBUG -MD -MF.depend.pam_exec.o -MTpam_exec.o -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-address-of-packed-member -Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter -Wno-parentheses -Qunused-arguments -c pam_exec.c -E | vim - and the preprocessed code in question looks sane (included a few lines of context either side): envlist = pam_getenvlist(pamh); for (envlen = 0; envlist[envlen] != ((void *)0); ++envlen) ; nitems = sizeof(env_items) / sizeof(*env_items); nitems_rv = options->return_prog_exit_status ? 24 : 0; tmp = reallocarray(envlist, envlen + nitems + 1 + nitems_rv + 1, sizeof(*envlist)); if (tmp == ((void *)0)) { openpam_free_envlist(envlist); return (PAM_BUF_ERR); } Cheers, Lawrence