diff mbox series

[v6] posix: Deprecate group_member for Linux

Message ID 20231212165657.3368122-1-josimmon@redhat.com
State New
Headers show
Series [v6] posix: Deprecate group_member for Linux | expand

Commit Message

Joe Simmons-Talbott Dec. 12, 2023, 4:56 p.m. UTC 
The alloca usage in group_member could lead to stack overflow on Linux.
Removing the alloca usage would require group_member to handle the error
condition where memory could not be allocated and that cannot be done
since group_member returns a boolean value.  Thus deprecate group_member.
Add an internal only implementation of __group_member2 using a
scratch_buffer and return -1 for memory allocation errors.  Use
__group_member2 for in place of __group_member internally.  Add testcases
for both group_member and __group_member2.
---
Changes to v5:
* Add __group_member2 and use it internally in the place of the now
  deprecated group_member.
* Add a testcase for __group_member2.

Changes to v4:
* Rebase onto latest commit.

Changes to v3:
* Fix include guards to match file location _BITS_GROUP_MEMBER_H
* Fix indentation of preprocessor directives

Changes to v2:
* Move the linux group_member.h to the bits directory
* Include the correct group_member.h in posix/unistd.h

Changes to v1:
* Add NEWS entry
* Move group_member.h to bits/group_member.h
* Include bits/group_member.h in installed headers
* Add tests to group_member.h files to only be included from unistd.h

 NEWS                                        |  4 ++
 bits/group_member.h                         | 31 +++++++++++++++
 include/unistd.h                            |  1 +
 posix/Makefile                              |  8 ++++
 posix/group_member.c                        | 32 +++++++++++++++
 posix/tst-group_member.c                    | 41 ++++++++++++++++++++
 posix/tst-group_member2.c                   | 43 +++++++++++++++++++++
 posix/unistd.h                              |  6 +--
 sysdeps/posix/euidaccess.c                  |  9 ++++-
 sysdeps/unix/sysv/linux/bits/group_member.h | 32 +++++++++++++++
 sysdeps/unix/sysv/linux/faccessat.c         |  8 +++-
 11 files changed, 209 insertions(+), 6 deletions(-)
 create mode 100644 bits/group_member.h
 create mode 100644 posix/tst-group_member.c
 create mode 100644 posix/tst-group_member2.c
 create mode 100644 sysdeps/unix/sysv/linux/bits/group_member.h

Comments

Florian Weimer Dec. 13, 2023, 10:36 a.m. UTC | #1 
* Joe Simmons-Talbott:

> +int
> +__group_member2 (gid_t gid)
> +{
> +  int n, size;
> +  gid_t *groups;
> +  struct scratch_buffer sbuf;
> +  scratch_buffer_init (&sbuf);
> +
> +  size = NGROUPS_MAX;
> +  do
> +    {
> +      if (!scratch_buffer_set_array_size (&sbuf, sizeof (*groups), size))
> +	return -1;
> +      groups = sbuf.data;
> +
> +      n = __getgroups (size, groups);
> +      size *= 2;
> +    }
> +  while (n == size / 2);

This doesn't look quite right to me, sorry.  The initial size we use for
getgroups should be based on the scratch buffer default, not the value
for NGROUPS_MAX.

In the loop, we should call getgroups with 0, use that to resize the
scratch array, and then call getgroups again with that size.  The loop
is still required because setgroups is eventually supposed to be
async-signal-safe by analogy.

Thanks,
Florian
Joe Simmons-Talbott Dec. 13, 2023, 3:36 p.m. UTC | #2 
On Wed, Dec 13, 2023 at 11:36:43AM +0100, Florian Weimer wrote:
> * Joe Simmons-Talbott:
> 
> > +int
> > +__group_member2 (gid_t gid)
> > +{
> > +  int n, size;
> > +  gid_t *groups;
> > +  struct scratch_buffer sbuf;
> > +  scratch_buffer_init (&sbuf);
> > +
> > +  size = NGROUPS_MAX;
> > +  do
> > +    {
> > +      if (!scratch_buffer_set_array_size (&sbuf, sizeof (*groups), size))
> > +	return -1;
> > +      groups = sbuf.data;
> > +
> > +      n = __getgroups (size, groups);
> > +      size *= 2;
> > +    }
> > +  while (n == size / 2);
> 
> This doesn't look quite right to me, sorry.  The initial size we use for
> getgroups should be based on the scratch buffer default, not the value
> for NGROUPS_MAX.
> 
> In the loop, we should call getgroups with 0, use that to resize the
> scratch array, and then call getgroups again with that size.  The loop
> is still required because setgroups is eventually supposed to be
> async-signal-safe by analogy.

Thank you for the review.  I've sent an updated v7 patch [1] with your
suggested changes.

Thanks,
Joe

[1] https://sourceware.org/pipermail/libc-alpha/2023-December/153290.html
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 9432564444..c1f9e5d0cc 100644
--- a/NEWS
+++ b/NEWS
@@ -44,6 +44,10 @@  Deprecated and removed features, and other changes affecting compatibility:
   ".tmp", to avoid examining temporary files created by the RPM and dpkg
   package managers.
 
+* Deprecated group_member on Linux as it uses alloca to allocate a large
+  buffer and has no capability for indicating failure for other memory
+  allocations.
+
 Changes to build and runtime requirements:
 
 * Building on LoongArch requires at a minimum binutils 2.41 for vector
diff --git a/bits/group_member.h b/bits/group_member.h
new file mode 100644
index 0000000000..7c43e7ee06
--- /dev/null
+++ b/bits/group_member.h
@@ -0,0 +1,31 @@ 
+/* group_member declaration
+   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/>.  */
+
+#ifndef _UNISTD_H
+# error "Never use <bits/group_member.h> directly; include <unistd.h> instead."
+#endif
+
+#ifndef _BITS_GROUP_MEMBER_H
+# define _BITS_GROUP_MEMBER_H 1
+
+# ifdef	__USE_GNU
+/* Return nonzero iff the calling process is in group GID.  */
+extern int group_member (__gid_t __gid) __THROW;
+# endif
+
+#endif /* _BITS_GROUP_MEMBER_H */
diff --git a/include/unistd.h b/include/unistd.h
index e241603b81..39d5bda372 100644
--- a/include/unistd.h
+++ b/include/unistd.h
@@ -131,6 +131,7 @@  extern __gid_t __getegid (void) attribute_hidden;
 extern int __getgroups (int __size, __gid_t __list[]) attribute_hidden;
 libc_hidden_proto (__getpgid)
 extern int __group_member (__gid_t __gid) attribute_hidden;
+extern int __group_member2 (__gid_t __gid) attribute_hidden;
 extern int __setuid (__uid_t __uid);
 extern int __setreuid (__uid_t __ruid, __uid_t __euid);
 extern int __setgid (__gid_t __gid);
diff --git a/posix/Makefile b/posix/Makefile
index 3ab124d040..c4948e3980 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -29,6 +29,7 @@  headers := \
   bits/getopt_core.h \
   bits/getopt_ext.h \
   bits/getopt_posix.h \
+  bits/group_member.h \
   bits/local_lim.h \
   bits/mman_ext.h \
   bits/posix1_lim.h \
@@ -291,6 +292,7 @@  tests := \
   tst-glob_symlinks \
   tst-gnuglob \
   tst-gnuglob64 \
+  tst-group_member \
   tst-mmap \
   tst-mmap-offset \
   tst-nanosleep \
@@ -479,6 +481,10 @@  tests-special += \
   # tests-special
 endif
 
+# This test calls __group_member2 directly, which is not exported from glibc.
+tests-internal += tst-group_member2
+tests-static += tst-group_member2
+
 include ../Rules
 
 ifeq ($(run-built-tests),yes)
@@ -606,6 +612,8 @@  bug-glob1-ARGS = "$(objpfx)"
 tst-execvp3-ARGS = --test-dir=$(objpfx)
 CFLAGS-tst-spawn3.c += -DOBJPFX=\"$(objpfx)\"
 
+CFLAGS-tst-group_member.c += -Wno-error=deprecated-declarations
+
 # Test voluntarily overflows struct dirent
 CFLAGS-bug-glob2.c += $(no-fortify-source)
 
diff --git a/posix/group_member.c b/posix/group_member.c
index 22422b1f9f..576386494e 100644
--- a/posix/group_member.c
+++ b/posix/group_member.c
@@ -18,6 +18,7 @@ 
 
 #include <sys/types.h>
 #include <unistd.h>
+#include <scratch_buffer.h>
 #include <stdlib.h>
 #include <limits.h>
 
@@ -47,3 +48,34 @@  __group_member (gid_t gid)
   return 0;
 }
 weak_alias (__group_member, group_member)
+
+int
+__group_member2 (gid_t gid)
+{
+  int n, size;
+  gid_t *groups;
+  struct scratch_buffer sbuf;
+  scratch_buffer_init (&sbuf);
+
+  size = NGROUPS_MAX;
+  do
+    {
+      if (!scratch_buffer_set_array_size (&sbuf, sizeof (*groups), size))
+	return -1;
+      groups = sbuf.data;
+
+      n = __getgroups (size, groups);
+      size *= 2;
+    }
+  while (n == size / 2);
+
+  while (n-- > 0)
+    if (groups[n] == gid)
+      {
+	scratch_buffer_free (&sbuf);
+        return 1;
+      }
+
+  scratch_buffer_free (&sbuf);
+  return 0;
+}
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>
diff --git a/posix/tst-group_member2.c b/posix/tst-group_member2.c
new file mode 100644
index 0000000000..ee448c578a
--- /dev/null
+++ b/posix/tst-group_member2.c
@@ -0,0 +1,43 @@ 
+/* 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 <posix/unistd.h>
+
+#include <support/check.h>
+
+extern int __group_member2 (__gid_t __gid);
+
+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_member2(groups[n]));
+
+  return EXIT_SUCCESS;
+}
+
+#include <support/test-driver.c>
diff --git a/posix/unistd.h b/posix/unistd.h
index 0477527a60..b65d4c63d0 100644
--- a/posix/unistd.h
+++ b/posix/unistd.h
@@ -710,10 +710,10 @@  extern __gid_t getegid (void) __THROW;
    of its supplementary groups in LIST and return the number written.  */
 extern int getgroups (int __size, __gid_t __list[]) __THROW __wur
     __fortified_attr_access (__write_only__, 2, 1);
+
 #ifdef	__USE_GNU
-/* Return nonzero iff the calling process is in group GID.  */
-extern int group_member (__gid_t __gid) __THROW;
-#endif
+# include <bits/group_member.h>
+#endif 
 
 /* Set the user ID of the calling process to UID.
    If the calling process is the super-user, set the real
diff --git a/sysdeps/posix/euidaccess.c b/sysdeps/posix/euidaccess.c
index 2282a0a8dd..2eb9db4c95 100644
--- a/sysdeps/posix/euidaccess.c
+++ b/sysdeps/posix/euidaccess.c
@@ -81,7 +81,7 @@  extern int errno;
 
 #ifdef _LIBC
 
-# define group_member __group_member
+# define group_member __group_member2
 # define euidaccess __euidaccess
 
 #else
@@ -167,9 +167,14 @@  euidaccess (const char *path, int mode)
 		    || (stats.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))))
     return 0;
 
+  int gm = group_member (stats.st_gid);
+  if (euid != stats.st_uid && egid != stats.st_gid)
+    if (gm == -1)
+      return -1;
+
   if (euid == stats.st_uid)
     granted = (unsigned int) (stats.st_mode & (mode << 6)) >> 6;
-  else if (egid == stats.st_gid || group_member (stats.st_gid))
+  else if (egid == stats.st_gid || gm)
     granted = (unsigned int) (stats.st_mode & (mode << 3)) >> 3;
   else
     granted = (stats.st_mode & mode);
diff --git a/sysdeps/unix/sysv/linux/bits/group_member.h b/sysdeps/unix/sysv/linux/bits/group_member.h
new file mode 100644
index 0000000000..0dd9505c76
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/bits/group_member.h
@@ -0,0 +1,32 @@ 
+/* group_member declaration
+   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/>.  */
+
+#ifndef _UNISTD_H
+# error "Never use <bits/group_member.h> directly; include <unistd.h> instead."
+#endif
+
+#ifndef _BITS_GROUP_MEMBER_H
+# define _BITS_GROUP_MEMBER_H 1
+
+# ifdef	__USE_GNU
+/* Return nonzero iff the calling process is in group GID.  Deprecated */
+extern int group_member (__gid_t __gid) __THROW
+	__attribute_deprecated_msg__ ("may overflow the stack");
+# endif
+
+#endif /* _BITS_GROUP_MEMBER_H */
diff --git a/sysdeps/unix/sysv/linux/faccessat.c b/sysdeps/unix/sysv/linux/faccessat.c
index 0ccbd778b5..f28ab0a6f4 100644
--- a/sysdeps/unix/sysv/linux/faccessat.c
+++ b/sysdeps/unix/sysv/linux/faccessat.c
@@ -59,11 +59,17 @@  __faccessat (int fd, const char *file, int mode, int flag)
 		   || (stats.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))))
     return 0;
 
+  int gm = __group_member2 (stats.st_gid);
+  if (uid != stats.st_uid &&
+      (stats.st_gid != ((flag & AT_EACCESS) ? __getegid () : __getgid ())))
+    if (gm == -1)
+      return -1;
+
   int granted = (uid == stats.st_uid
 		 ? (unsigned int) (stats.st_mode & (mode << 6)) >> 6
 		 : (stats.st_gid == ((flag & AT_EACCESS)
 				     ? __getegid () : __getgid ())
-		    || __group_member (stats.st_gid))
+		    || gm)
 		 ? (unsigned int) (stats.st_mode & (mode << 3)) >> 3
 		 : (stats.st_mode & mode));