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@xxxxxxxxxxxxxx>
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@xxxxxxxxx>
Date: Mon Oct 25 21:45:49 2021 +0200
getgrouplist.3: wfix
Reported-by: Tobias Stoeckmann <tobias@xxxxxxxxxxxxxx>
Signed-off-by: Alejandro Colomar <alx.manpages@xxxxxxxxx>
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@xxxxxxxxx>
Date: Mon Oct 25 21:44:37 2021 +0200
getgrouplist.3: Place variable definitions on top of function
Reported-by: Tobias Stoeckmann <tobias@xxxxxxxxxxxxxx>
Signed-off-by: Alejandro Colomar <alx.manpages@xxxxxxxxx>
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/
