group_member: Get rid of unbounded alloca.

  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.

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
  

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.

> 
> 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
  

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
>