[PATCH] group_member: Get rid of unbounded alloca.

[Joe Simmons-Talbott]

On Tue, Jul 11, 2023 at 04:07:43PM -0400, Siddhesh Poyarekar wrote:
> On 2023-07-11 15:29, Joe Simmons-Talbott via Libc-alpha wrote:
> > Replace an unbounded alloca call with a scratch_buffer to avoid
> > potential stack overflow.  Becasue group_member doesn't return an error
> > indicator abort if we are unable to allocate memory.  Add a testcase.
> 
> So technically speaking there is an upper bound to this alloca at 64k
> groups, so you can't really call this an unbounded alloca.  However when
> there are more than 4k groups and a thread with a small stack (e.g. 16k,
> which is PTHREAD_MIN_STACKSIZE for a number of architectures) calls
> group_member, the alloca will overflow the stack.
> 

Updated commit message with typo fix in v2[1]

[1] https://sourceware.org/pipermail/libc-alpha/2023-August/150773.html

Thanks,
Joe
> > 
> > Checked on x86_64-linux-gnu.
> > ---
> >   posix/Makefile           |  1 +
> >   posix/group_member.c     | 27 +++++++++++++++-----------
> >   posix/tst-group_member.c | 41 ++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 58 insertions(+), 11 deletions(-)
> >   create mode 100644 posix/tst-group_member.c
> > 
> > diff --git a/posix/Makefile b/posix/Makefile
> > index 3d368b91f6..7491ee8917 100644
> > --- a/posix/Makefile
> > +++ b/posix/Makefile
> > @@ -293,6 +293,7 @@ tests := \
> >     tst-glob_symlinks \
> >     tst-gnuglob \
> >     tst-gnuglob64 \
> > +  tst-group_member \
> >     tst-mmap \
> >     tst-mmap-offset \
> >     tst-nanosleep \
> > diff --git a/posix/group_member.c b/posix/group_member.c
> > index 22422b1f9f..42a4adb9b4 100644
> > --- a/posix/group_member.c
> > +++ b/posix/group_member.c
> > @@ -16,9 +16,10 @@
> >      License along with the GNU C Library; if not, see
> >      <https://www.gnu.org/licenses/>.  */
> > +#include <scratch_buffer.h>
> > +#include <stdlib.h>
> >   #include <sys/types.h>
> >   #include <unistd.h>
> > -#include <stdlib.h>
> >   #include <limits.h>
> >   #ifndef NGROUPS_MAX
> > @@ -28,22 +29,26 @@
> >   int
> >   __group_member (gid_t gid)
> >   {
> > -  int n, size;
> > +  int n;
> >     gid_t *groups;
> > +  struct scratch_buffer buf;
> > +  scratch_buffer_init (&buf);
> > +
> > +  n = __getgroups (0, NULL);
> > +  if (!scratch_buffer_set_array_size (&buf, n, sizeof (*groups)))
> > +    abort ();
> 
> This is better than, e.g. returning a new error code (and maybe bumping ABI)
> since applications will still need to be fixed to account for the new error
> return.
> 
> > +  groups = buf.data;
> > -  size = NGROUPS_MAX;
> > -  do
> > -    {
> > -      groups = __alloca (size * sizeof *groups);
> > -      n = __getgroups (size, groups);
> > -      size *= 2;
> > -    }
> > -  while (n == size / 2);
> > +  n = __getgroups (n, groups);
> >     while (n-- > 0)
> >       if (groups[n] == gid)
> > -      return 1;
> > +      {
> > +	scratch_buffer_free (&buf);
> > +        return 1;
> > +      }
> > +  scratch_buffer_free (&buf);
> >     return 0;
> >   }
> >   weak_alias (__group_member, group_member)
> > diff --git a/posix/tst-group_member.c b/posix/tst-group_member.c
> > new file mode 100644
> > index 0000000000..7f70841832
> > --- /dev/null
> > +++ b/posix/tst-group_member.c
> > @@ -0,0 +1,41 @@
> > +/* Basic tests for group_member.
> > +   Copyright (C) 2023 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#include <alloca.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +
> > +#include <support/check.h>
> > +
> > +static int do_test (void)
> > +{
> > +  int n;
> > +  gid_t *groups;
> > +
> > +  n = getgroups (0, NULL);
> > +  groups = alloca (n * sizeof (*groups));
> > +  n = getgroups (n, groups);
> > +
> > +  while (n-- > 0)
> > +    TEST_COMPARE (1, group_member(groups[n]));
> > +
> > +  return EXIT_SUCCESS;
> > +}
> > +
> > +#include <support/test-driver.c>
> 
> OK, smoke test for getgroups and group_member.  I wish we could make this a
> container test and somehow test a user with multiple groups but I don't
> think we can actually do that.
> 
> I'll leave the actual review to someone else given that we've already
> discussed this offline and I'd like to get more people pitch in with their
> views on this.
> 
> This should probably get a bug report too, it's not security sensitve AFAICT
> since the number of supplementary groups (which is the primary vector for
> stack overflow) needs elevated privileges to control.
> 
> Thanks,
> Sid
>