svn commit: r314780 - head/lib/libpam/modules/pam_exec

[Pedro Giffuni]

On 3/12/2017 12:40 PM, Lawrence Stewart wrote:
> 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<out> 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);
>   }

OK, the nitems issue is cosmetical at this time.

Are you getting PAM_BUF_ERR, in other words, is tmp NULL? We may be 
hitting some strict limit in reallocarray().

Pedro.