[patch] getgrouplist.3: Improve example code

[patch] getgrouplist.3: Improve example code

[Tobias Stoeckmann]

The example code does not validate the supplied ngroup argument.
On 32 bit systems this code can lead to heap overflows within
getgrouplist call.

Verify that ngroups really contains the amount of entries for
which memory has been allocated.

While at it fixed a small typo ("to" was missing).

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
---
 man3/getgrouplist.3 | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/man3/getgrouplist.3 b/man3/getgrouplist.3
index 1fe260b..da36cc7 100644
--- a/man3/getgrouplist.3
+++ b/man3/getgrouplist.3
@@ -97,7 +97,7 @@ groups, then
 returns \-1.
 In this case, the value returned in
 .IR *ngroups
-can be used to resize the buffer passed to a further call
+can be used to resize the buffer passed to a further call to
 .BR getgrouplist ().
 .SH VERSIONS
 This function is present since glibc 2.2.4.
@@ -152,6 +152,7 @@ ngroups = 3
 .SS Program source
 \&
 .EX
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <grp.h>
@@ -163,6 +164,7 @@ main(int argc, char *argv[])
     int ngroups;
     struct passwd *pw;
     struct group *gr;
+    gid_t *groups;
 
     if (argc != 3) {
         fprintf(stderr, "Usage: %s <user> <ngroups>\en", argv[0]);
@@ -171,7 +173,12 @@ main(int argc, char *argv[])
 
     ngroups = atoi(argv[2]);
 
-    gid_t *groups = malloc(sizeof(*groups) * ngroups);
+    if (ngroups < 0 || (size_t)ngroups > SIZE_MAX / sizeof(*groups)) {
+        fprintf(stderr, "ngroups invalid\en");
+        exit(EXIT_FAILURE);
+    }
+
+    groups = malloc(sizeof(*groups) * ngroups);
     if (groups == NULL) {
         perror("malloc");
         exit(EXIT_FAILURE);
-- 
2.33.1

Re: [patch] getgrouplist.3: Improve example code

[Alejandro Colomar (man-pages)]

Hello Tobias,

On 10/22/21 6:09 PM, Tobias Stoeckmann wrote:
> The example code does not validate the supplied ngroup argument.
> On 32 bit systems this code can lead to heap overflows within
> getgrouplist call.
> 
> Verify that ngroups really contains the amount of entries for
> which memory has been allocated.
> 
> While at it fixed a small typo ("to" was missing).
> 
> Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
You're right that it may overflow (almost every calculation may overflow 
in a program, if you don´t restrict yourself to sane values), but I 
won't fix that code.

Why:

- I don't think other manual pages that use malloc(3) do the same test, 
and consistency is (very) important.

- We use atoi(3) for simplicity in user input, instead of using 
strtol(3) properly, which a real program should do.  Using strtol(3) 
would add a lot of complexity to our example program.  Since there are 
already many reasons that number can be invalid, I don't see much value 
in checking one of the possible problems of that number.

- Sane values for ngroups are very far from INT32_MAX.  I don't expect 
any present or future real case scenario where a user may be in anything 
close to thousands of millions of groups.  Do you?

- I don't like malloc(3)'s API.  Consider implementing your own 
mallocarray() in terms of malloc(3), and placing there the code to check 
for overflow in the input.  I use 'void *mallocarray(ptrdiff_t nmemb, 
ssize_t size);'.  If the malloc(3) page needs some advise on how to 
carefully use it, especially regarding its input, we should add that 
advise in malloc.3.

- In the end it's not a problem intrinsic to the usage of 
getgrouplist(3), but a problem of malloc(3), and I don't want to clutter 
the getgrouplist(3) example with malloc(3)'s.  Let's keep examples 
simple, even if they aren't perfect.

However, I applied 2 commits for the side effects of this patch, which I 
liked.

Thanks!

Alex

---

$ git show -2
commit a1692d75260d96f7771a06bced4f36060474590b (HEAD -> main, alx/main, 
alx/HEAD)
Author: Alejandro Colomar <alx.manpages@gmail.com>
Date:   Mon Oct 25 21:45:49 2021 +0200

     getgrouplist.3: wfix

     Reported-by: Tobias Stoeckmann <tobias@stoeckmann.org>
     Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>

diff --git a/man3/getgrouplist.3 b/man3/getgrouplist.3
index 533296370..5b173beef 100644
--- a/man3/getgrouplist.3
+++ b/man3/getgrouplist.3
@@ -97,7 +97,7 @@ groups, then
  returns \-1.
  In this case, the value returned in
  .IR *ngroups
-can be used to resize the buffer passed to a further call
+can be used to resize the buffer passed to a further call to
  .BR getgrouplist ().
  .SH VERSIONS
  This function is present since glibc 2.2.4.

commit c59aa7fc414eb3394ace200e4c71511232ef5801
Author: Alejandro Colomar <alx.manpages@gmail.com>
Date:   Mon Oct 25 21:44:37 2021 +0200

     getgrouplist.3: Place variable definitions on top of function

     Reported-by: Tobias Stoeckmann <tobias@stoeckmann.org>
     Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>

diff --git a/man3/getgrouplist.3 b/man3/getgrouplist.3
index 1fe260bda..533296370 100644
--- a/man3/getgrouplist.3
+++ b/man3/getgrouplist.3
@@ -163,6 +163,7 @@ main(int argc, char *argv[])
      int ngroups;
      struct passwd *pw;
      struct group *gr;
+    gid_t *groups;

      if (argc != 3) {
          fprintf(stderr, "Usage: %s <user> <ngroups>\en", argv[0]);
@@ -171,7 +172,7 @@ main(int argc, char *argv[])

      ngroups = atoi(argv[2]);

-    gid_t *groups = malloc(sizeof(*groups) * ngroups);
+    groups = malloc(sizeof(*groups) * ngroups);
      if (groups == NULL) {
          perror("malloc");
          exit(EXIT_FAILURE);


-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/