Re: Security release for mod_wsgi (version 3.5)

[Solar Designer <solar () openwall com>]

On Wed, Jun 18, 2014 at 08:08:10PM +1200, Matthew Daley wrote:
> I may be wrong as I haven't been following this discussion entirely, but...

I think Graham is not on oss-security.  CC added.

Graham, please comment on the potential off-by-one bug reported by
Matthew below:

> 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