[musl] [PATCH] Increase NGROUPS_MAX from 32 to 1024

[musl] [PATCH] Increase NGROUPS_MAX from 32 to 1024

[Kate Deplaix]

Such a restrictive value for NGROUPS_MAX makes it impossible to have a musl-based system with a user belonging to more than 32 groups. If done on the root user, this will break your system.
It also makes it impossible to use certain functions in binaries that have been compiled with musl.

This new value is still very far from Linux's NGROUPS_MAX of 65536 that has been there since Linux 2.6.4 but this is at least one tiny step in the right direction while maintainers investigate how to match Linux's value.

ref: https://www.openwall.com/lists/musl/2021/07/03/1
ref: https://www.openwall.com/lists/musl/2022/12/06/3
ref: https://github.com/ocaml/opam/pull/5383
---
 include/limits.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/limits.h b/include/limits.h
index 53a27b9d..501c3612 100644
--- a/include/limits.h
+++ b/include/limits.h
@@ -45,7 +45,7 @@
 #define NAME_MAX 255
 #endif
 #define PATH_MAX 4096
-#define NGROUPS_MAX 32
+#define NGROUPS_MAX 1024
 #define ARG_MAX 131072
 #define IOV_MAX 1024
 #define SYMLOOP_MAX 40
--
2.40.1

[musl] Re: [PATCH] Increase NGROUPS_MAX from 32 to 1024

[Kate Deplaix]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

Hi,

Any chance this patch could be looked at?

Thanks,
Kate
________________________________
From: Kate Deplaix
Sent: 14 November 2023 23:35
To: musl@lists.openwall.com <musl@lists.openwall.com>
Subject: [PATCH] Increase NGROUPS_MAX from 32 to 1024

Such a restrictive value for NGROUPS_MAX makes it impossible to have a musl-based system with a user belonging to more than 32 groups. If done on the root user, this will break your system.
It also makes it impossible to use certain functions in binaries that have been compiled with musl.

This new value is still very far from Linux's NGROUPS_MAX of 65536 that has been there since Linux 2.6.4 but this is at least one tiny step in the right direction while maintainers investigate how to match Linux's value.

ref: https://www.openwall.com/lists/musl/2021/07/03/1
ref: https://www.openwall.com/lists/musl/2022/12/06/3
ref: https://github.com/ocaml/opam/pull/5383
---
 include/limits.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/limits.h b/include/limits.h
index 53a27b9d..501c3612 100644
--- a/include/limits.h
+++ b/include/limits.h
@@ -45,7 +45,7 @@
 #define NAME_MAX 255
 #endif
 #define PATH_MAX 4096
-#define NGROUPS_MAX 32
+#define NGROUPS_MAX 1024
 #define ARG_MAX 131072
 #define IOV_MAX 1024
 #define SYMLOOP_MAX 40
--
2.40.1

[-- Attachment #2: Type: text/html, Size: 2473 bytes --]

Re: [musl] Re: [PATCH] Increase NGROUPS_MAX from 32 to 1024

[Rich Felker]

On Mon, Mar 11, 2024 at 06:31:13PM +0000, Kate Deplaix wrote:
> Hi,
> 
> Any chance this patch could be looked at?

Thanks for the ping. I'll take a look and see if we can open a
discussion on either doing something like this, or a more complete
fix, in this release cycle.

Rich

> ________________________________
> From: Kate Deplaix
> Sent: 14 November 2023 23:35
> To: musl@lists.openwall.com <musl@lists.openwall.com>
> Subject: [PATCH] Increase NGROUPS_MAX from 32 to 1024
> 
> Such a restrictive value for NGROUPS_MAX makes it impossible to have a musl-based system with a user belonging to more than 32 groups. If done on the root user, this will break your system.
> It also makes it impossible to use certain functions in binaries that have been compiled with musl.
> 
> This new value is still very far from Linux's NGROUPS_MAX of 65536 that has been there since Linux 2.6.4 but this is at least one tiny step in the right direction while maintainers investigate how to match Linux's value.
> 
> ref: https://www.openwall.com/lists/musl/2021/07/03/1
> ref: https://www.openwall.com/lists/musl/2022/12/06/3
> ref: https://github.com/ocaml/opam/pull/5383
> ---
>  include/limits.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/limits.h b/include/limits.h
> index 53a27b9d..501c3612 100644
> --- a/include/limits.h
> +++ b/include/limits.h
> @@ -45,7 +45,7 @@
>  #define NAME_MAX 255
>  #endif
>  #define PATH_MAX 4096
> -#define NGROUPS_MAX 32
> +#define NGROUPS_MAX 1024
>  #define ARG_MAX 131072
>  #define IOV_MAX 1024
>  #define SYMLOOP_MAX 40
> --
> 2.40.1

Re: [musl] Re: [PATCH] Increase NGROUPS_MAX from 32 to 1024

[Kate Deplaix]

[-- Attachment #1: Type: text/plain, Size: 1991 bytes --]

Hi,

Are there any updates on this by any chance?

Many thanks,
Kate
________________________________
From: Rich Felker <dalias@libc.org>
Sent: 12 March 2024 00:46
To: Kate Deplaix <kit-ty-kate@outlook.com>
Cc: musl@lists.openwall.com <musl@lists.openwall.com>
Subject: Re: [musl] Re: [PATCH] Increase NGROUPS_MAX from 32 to 1024

On Mon, Mar 11, 2024 at 06:31:13PM +0000, Kate Deplaix wrote:
> Hi,
>
> Any chance this patch could be looked at?

Thanks for the ping. I'll take a look and see if we can open a
discussion on either doing something like this, or a more complete
fix, in this release cycle.

Rich

> ________________________________
> From: Kate Deplaix
> Sent: 14 November 2023 23:35
> To: musl@lists.openwall.com <musl@lists.openwall.com>
> Subject: [PATCH] Increase NGROUPS_MAX from 32 to 1024
>
> Such a restrictive value for NGROUPS_MAX makes it impossible to have a musl-based system with a user belonging to more than 32 groups. If done on the root user, this will break your system.
> It also makes it impossible to use certain functions in binaries that have been compiled with musl.
>
> This new value is still very far from Linux's NGROUPS_MAX of 65536 that has been there since Linux 2.6.4 but this is at least one tiny step in the right direction while maintainers investigate how to match Linux's value.
>
> ref: https://www.openwall.com/lists/musl/2021/07/03/1
> ref: https://www.openwall.com/lists/musl/2022/12/06/3
> ref: https://github.com/ocaml/opam/pull/5383
> ---
>  include/limits.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/limits.h b/include/limits.h
> index 53a27b9d..501c3612 100644
> --- a/include/limits.h
> +++ b/include/limits.h
> @@ -45,7 +45,7 @@
>  #define NAME_MAX 255
>  #endif
>  #define PATH_MAX 4096
> -#define NGROUPS_MAX 32
> +#define NGROUPS_MAX 1024
>  #define ARG_MAX 131072
>  #define IOV_MAX 1024
>  #define SYMLOOP_MAX 40
> --
> 2.40.1

[-- Attachment #2: Type: text/html, Size: 3313 bytes --]

Re: [musl] Re: [PATCH] Increase NGROUPS_MAX from 32 to 1024

[Thorsten Glaser]

Kate Deplaix dixit:

>Are there any updates on this by any chance?

This has the possibility of being a major ABI break
(requiring soname bump) if not for musl then possibly
for libraries built on musl systems.

bye,
//mirabilos
-- 
15:41⎜<Lo-lan-do:#fusionforge> Somebody write a testsuite for helloworld :-)

Re: [musl] Re: [PATCH] Increase NGROUPS_MAX from 32 to 1024

[Rich Felker]

On Tue, Apr 09, 2024 at 03:46:16PM +0000, Thorsten Glaser wrote:
> Kate Deplaix dixit:
> 
> >Are there any updates on this by any chance?
> 
> This has the possibility of being a major ABI break
> (requiring soname bump) if not for musl then possibly
> for libraries built on musl systems.

I don't think it's the kind of "ABI break" we'd rule out. It would be
if there were interfaces that didn't take a size argument and that
were allowed to write up to NGROUPS_MAX into a caller-provided buffer,
but that's not the case.

I said I'd look at this before and just need to get around to it.

Rich

Re: [musl] [PATCH] Increase NGROUPS_MAX from 32 to 1024

[Rich Felker]

On Tue, Nov 14, 2023 at 11:35:14PM +0000, Kate Deplaix wrote:
> Such a restrictive value for NGROUPS_MAX makes it impossible to have
> a musl-based system with a user belonging to more than 32 groups. If
> done on the root user, this will break your system.
> It also makes it impossible to use certain functions in binaries
> that have been compiled with musl.
> 
> This new value is still very far from Linux's NGROUPS_MAX of 65536
> that has been there since Linux 2.6.4 but this is at least one tiny
> step in the right direction while maintainers investigate how to
> match Linux's value.
> 
> ref: https://www.openwall.com/lists/musl/2021/07/03/1
> ref: https://www.openwall.com/lists/musl/2022/12/06/3
> ref: https://github.com/ocaml/opam/pull/5383
> ---
>  include/limits.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/limits.h b/include/limits.h
> index 53a27b9d..501c3612 100644
> --- a/include/limits.h
> +++ b/include/limits.h
> @@ -45,7 +45,7 @@
>  #define NAME_MAX 255
>  #endif
>  #define PATH_MAX 4096
> -#define NGROUPS_MAX 32
> +#define NGROUPS_MAX 1024
>  #define ARG_MAX 131072
>  #define IOV_MAX 1024
>  #define SYMLOOP_MAX 40
> --
> 2.40.1

I've gone and read the POSIX text on NGROUPS_MAX again:

    Runtime Increasable Values
    
    The magnitude limitations in the following list shall be fixed by
    specific implementations. An application should assume that the
    value of the symbolic constant defined by <limits.h> in a specific
    implementation is the minimum that pertains whenever the
    application is run under that implementation. A specific instance
    of a specific implementation may increase the value relative to
    that supplied by <limits.h> for that implementation. The actual
    value supported by a specific instance shall be provided by the
    sysconf() function.
    
    ...
    
    {NGROUPS_MAX}
         Maximum number of simultaneous supplementary group IDs per
         process.

and for getgroups:

    If the effective group ID of the process is returned with the
    supplementary group IDs, the value returned shall always be
    greater than or equal to one and less than or equal to the value
    of {NGROUPS_MAX}+1.

For the latter, this notation usually means not necessarily the macro
value, but the sysconf value obtainable at runtime.

Since there is no POSIX interface setgroups (this functionality is
left to the implementation), NGROUPS_MAX does not impose a requirement
to support setting that many groups. So increasing the value would not
make things "wrong on old kernels".

However POSIX also allows/encourages the macro to represent a "minimum
that pertains", which could reasonably be the 32, as long as
sysconf(_SG_NGROUPS_MAX) returns a value >= one less than the max
value getgroups can return at runtime.

I was worried using the procfs interface to make
sysconf(_SG_NGROUPS_MAX) dynamic may not be correct, as it could be
decreased after a process comes to have more supplemental groups than
the old limit. However, it appears this is just a read-only sysctl.
The ngroups_max variable was made const in commit
f628867da46f8867e1854e43d7200e42ec22eee2, but even before that the
file was read-only and did not seem to have any mechanism for changing
the limit. So it seems usable.

As for the macro, I think it's actually valid to define it as 65536,
since even if we're running on an old kernel, there is no conformance
distinction. I'm not sure if this is the nicest thing to do though.
Apps may want to start with a buffer of size NGROUPS_MAX and increase
it up to the sysconf value rather than allocating a giant amount of
memory that will never in practice be used. This should be further
discussed, particularly what impact it might have on application
behavior and memory usage.

Of course this still leaves us with what to do for initgroups. My
leaning is to have it behave as now, using a small stack buffer,
regardless of what we do with the macro, but with a change in the
error condition: when getgrouplist returns -1 with *ngroups greater
than the value passed-in, allocate a buffer this size with mmap (to
avoid pulling in a malloc dep; in the worst case it's large enough
malloc would use mmap anyway) and retry.

This avoids ever having to look at the kernel's value of ngroups_max;
setgroups will just fail (and initgroups will report the error) if
it's too large. It does have a TOCTOU race if the groups db changes
between the first call and the retry. If we care about that it could
loop on retry, and always increase the buffer size by at least 50%
each time so that it can't get stuck or consume more than a
logarithmic number of retries.

Thoughts? Does this sound reasonable? What to do with the macro?

Rich

Re: [musl] [PATCH] Increase NGROUPS_MAX from 32 to 1024

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 5504 bytes --]

On Wed, Apr 10, 2024 at 09:07:38PM -0400, Rich Felker wrote:
> On Tue, Nov 14, 2023 at 11:35:14PM +0000, Kate Deplaix wrote:
> > Such a restrictive value for NGROUPS_MAX makes it impossible to have
> > a musl-based system with a user belonging to more than 32 groups. If
> > done on the root user, this will break your system.
> > It also makes it impossible to use certain functions in binaries
> > that have been compiled with musl.
> > 
> > This new value is still very far from Linux's NGROUPS_MAX of 65536
> > that has been there since Linux 2.6.4 but this is at least one tiny
> > step in the right direction while maintainers investigate how to
> > match Linux's value.
> > 
> > ref: https://www.openwall.com/lists/musl/2021/07/03/1
> > ref: https://www.openwall.com/lists/musl/2022/12/06/3
> > ref: https://github.com/ocaml/opam/pull/5383
> > ---
> >  include/limits.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/limits.h b/include/limits.h
> > index 53a27b9d..501c3612 100644
> > --- a/include/limits.h
> > +++ b/include/limits.h
> > @@ -45,7 +45,7 @@
> >  #define NAME_MAX 255
> >  #endif
> >  #define PATH_MAX 4096
> > -#define NGROUPS_MAX 32
> > +#define NGROUPS_MAX 1024
> >  #define ARG_MAX 131072
> >  #define IOV_MAX 1024
> >  #define SYMLOOP_MAX 40
> > --
> > 2.40.1
> 
> I've gone and read the POSIX text on NGROUPS_MAX again:
> 
>     Runtime Increasable Values
>     
>     The magnitude limitations in the following list shall be fixed by
>     specific implementations. An application should assume that the
>     value of the symbolic constant defined by <limits.h> in a specific
>     implementation is the minimum that pertains whenever the
>     application is run under that implementation. A specific instance
>     of a specific implementation may increase the value relative to
>     that supplied by <limits.h> for that implementation. The actual
>     value supported by a specific instance shall be provided by the
>     sysconf() function.
>     
>     ...
>     
>     {NGROUPS_MAX}
>          Maximum number of simultaneous supplementary group IDs per
>          process.
> 
> and for getgroups:
> 
>     If the effective group ID of the process is returned with the
>     supplementary group IDs, the value returned shall always be
>     greater than or equal to one and less than or equal to the value
>     of {NGROUPS_MAX}+1.
> 
> For the latter, this notation usually means not necessarily the macro
> value, but the sysconf value obtainable at runtime.
> 
> Since there is no POSIX interface setgroups (this functionality is
> left to the implementation), NGROUPS_MAX does not impose a requirement
> to support setting that many groups. So increasing the value would not
> make things "wrong on old kernels".
> 
> However POSIX also allows/encourages the macro to represent a "minimum
> that pertains", which could reasonably be the 32, as long as
> sysconf(_SG_NGROUPS_MAX) returns a value >= one less than the max
> value getgroups can return at runtime.
> 
> I was worried using the procfs interface to make
> sysconf(_SG_NGROUPS_MAX) dynamic may not be correct, as it could be
> decreased after a process comes to have more supplemental groups than
> the old limit. However, it appears this is just a read-only sysctl.
> The ngroups_max variable was made const in commit
> f628867da46f8867e1854e43d7200e42ec22eee2, but even before that the
> file was read-only and did not seem to have any mechanism for changing
> the limit. So it seems usable.
> 
> As for the macro, I think it's actually valid to define it as 65536,
> since even if we're running on an old kernel, there is no conformance
> distinction. I'm not sure if this is the nicest thing to do though.
> Apps may want to start with a buffer of size NGROUPS_MAX and increase
> it up to the sysconf value rather than allocating a giant amount of
> memory that will never in practice be used. This should be further
> discussed, particularly what impact it might have on application
> behavior and memory usage.
> 
> Of course this still leaves us with what to do for initgroups. My
> leaning is to have it behave as now, using a small stack buffer,
> regardless of what we do with the macro, but with a change in the
> error condition: when getgrouplist returns -1 with *ngroups greater
> than the value passed-in, allocate a buffer this size with mmap (to
> avoid pulling in a malloc dep; in the worst case it's large enough
> malloc would use mmap anyway) and retry.
> 
> This avoids ever having to look at the kernel's value of ngroups_max;
> setgroups will just fail (and initgroups will report the error) if
> it's too large. It does have a TOCTOU race if the groups db changes
> between the first call and the retry. If we care about that it could
> loop on retry, and always increase the buffer size by at least 50%
> each time so that it can't get stuck or consume more than a
> logarithmic number of retries.
> 
> Thoughts? Does this sound reasonable? What to do with the macro?

Here's a draft of how initgroups could work. I dropped direct use of
mmap since all this code already depends on malloc, and using malloc
simplifies it.

This versions undefined NGROUPS_MAX and replaces it with 2 so that the
fallback path will get triggered to test it. That should be removed
(and probably macros to use internal malloc added) before it's merged.

This change is independent of any potential changes we make to the
macro or to sysconf.

Rich

[-- Attachment #2: initgroups_draft.diff --]
[-- Type: text/plain, Size: 1188 bytes --]

diff --git a/src/misc/initgroups.c b/src/misc/initgroups.c
index 922a9581..d7138976 100644
--- a/src/misc/initgroups.c
+++ b/src/misc/initgroups.c
@@ -1,11 +1,34 @@
 #define _GNU_SOURCE
 #include <grp.h>
 #include <limits.h>
+#include <errno.h>
+#include <stdint.h>
+#include <stdlib.h>
+
+#undef NGROUPS_MAX
+#define NGROUPS_MAX 2
 
 int initgroups(const char *user, gid_t gid)
 {
-	gid_t groups[NGROUPS_MAX];
-	int count = NGROUPS_MAX;
-	if (getgrouplist(user, gid, groups, &count) < 0) return -1;
-	return setgroups(count, groups);
+	gid_t buf[NGROUPS_MAX], *groups = buf;
+	int count = NGROUPS_MAX, prev_count = count;
+	while (getgrouplist(user, gid, groups, &count) < 0) {
+		if (groups != buf) free(groups);
+
+		/* Return if failure isn't buffer size */
+		if (count <= prev_count)
+			return -1;
+
+		/* Always increase by at least 50% to limit to
+		 * logarithmically many retries on TOCTOU races. */
+		if (count < prev_count + (prev_count>>1))
+			count = prev_count + (prev_count>>1);
+
+		groups = calloc(count, sizeof *groups);
+		if (!groups) return -1;
+		prev_count = count;
+	}
+	int ret = setgroups(count, groups);
+	if (groups != buf) free(groups);
+	return ret;
 }

Re: [musl] [PATCH] Increase NGROUPS_MAX from 32 to 1024

[Markus Wichmann]

Am Wed, Apr 10, 2024 at 09:07:38PM -0400 schrieb Rich Felker:
> As for the macro, I think it's actually valid to define it as 65536,
> since even if we're running on an old kernel, there is no conformance
> distinction. I'm not sure if this is the nicest thing to do though.
> Apps may want to start with a buffer of size NGROUPS_MAX and increase
> it up to the sysconf value rather than allocating a giant amount of
> memory that will never in practice be used. This should be further
> discussed, particularly what impact it might have on application
> behavior and memory usage.
>

I had a look at Debian Codesearch for NGROUPS_MAX, to see what
applications are actually doing with the macro. And I found no instance
of anyone using it as an array size. That's what had me most worried,
because obviously increasing an array size by a few orders of magnitude
can cause a stack overrun.

A lot of applications use it or the sysconf() equivalent as upper bounds
for allocations, or even for setgroups(). So they should be fine with an
increase.

> It does have a TOCTOU race if the groups db changes
> between the first call and the retry.

Well, a lot of the login process has races if the user db changes during
the process. I think that is reasonable. As long as the race is resolved
in a safe way (as in, setting either the complete old list or the
complete new list), I think this is sensible. Although, now that I think
about it, the worst that could happen is someone being added to a group
and getting a truncated group list. And then they just have to re-login.
Which they already have to do anyway after being added to a group; they
were just too fast.

Ciao,
Markus

Re: [musl] [PATCH] Increase NGROUPS_MAX from 32 to 1024

[Laurent Bercot]

>I had a look at Debian Codesearch for NGROUPS_MAX, to see what
>applications are actually doing with the macro. And I found no instance
>of anyone using it as an array size.

  I do. e.g. 
https://git.skarnet.org/cgi-bin/cgit.cgi/s6/tree/src/daemontools-extras/s6-applyuidgid.c#n22

  It's in small short-lived utilities that don't allocate anything, so 
I'm
not too worried about overflowing the stack, but the change would not be
friendly to resource-constrained environments.

  My code runs on not-so-conformant systems such as Solaris or MacOS, 
where
I'm not sure that sysconf() and _SC_NGROUPS_MAX are even defined and 
correct.
I can test, but that's more work, and convoluted heuristics to make 
things
support every case are a strong decrease in readability and reliability,
an additional portability nightmare I don't want to deal with. Whereas
NGROUPS_MAX works everywhere.

  I'm not sure what the best course of action is. I think it still 
probably
is eating the ephemeral 256kB stack penalty if NGROUPS_MAX is increased
to 65536.

--
  Laurent