Re: Security release for mod_wsgi (version 3.5)

[Matthew Daley <mattd () bugfuzz com>]

I may be wrong as I haven't been following this discussion entirely, but...

On Wed, Jun 18, 2014 at 12:39 AM, Graham Dumpleton
<graham.dumpleton () gmail com> wrote:
> This feature was added for one specific user and wouldn't be a well known feature unless people were reading change 
> notes diligently as don't believe it is even covered in the documentation.
>
> Given that this code also only executes as root, the only error which could technically arise in this code for 
> setgroups() is if the number of groups exceeded NGROUPS_MAX.
>
> This should not occur though as the number of groups was previously validated when the configuration was read:
>
>     if (groups_list) {
>         const char *group_name = NULL;
>         long groups_maximum = NGROUPS_MAX;
>         const char *items = NULL;
>
> #ifdef _SC_NGROUPS_MAX
>         groups_maximum = sysconf(_SC_NGROUPS_MAX);
>         if (groups_maximum < 0)
>             groups_maximum = NGROUPS_MAX;
> #endif
>         groups = (gid_t *)apr_pcalloc(cmd->pool,
>                                       groups_maximum*sizeof(groups[0]));
>
>         groups[groups_count++] = gid;
>
>         items = groups_list;
>         group_name = ap_getword(cmd->pool, &items, ',');
>
>         while (group_name && *group_name) {
>             if (groups_count > groups_maximum)

This is an off-by-one error, isn't it? As in, it should be testing for
groups_count >= groups_maximum and not the current test.

>                 return "Too many supplementary groups WSGI daemon process";
>
>             groups[groups_count++] = ap_gname2id(group_name);
>             group_name = ap_getword(cmd->pool, &items, ',');
>         }
>     }
>
> Thus was pre-validated input.

- Matthew Daley