bugzilla-daemon at mindrot.org
2004-Feb-20 02:01 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-20 13:01 ------- Created an attachment (id=548) --> (http://bugzilla.mindrot.org/attachment.cgi?id=548&action=view) NGROUPS patch ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-20 02:01 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-20 13:01 ------- The proposed fix is wrong, I think. Even with sysconf, the NGROUPS_MAX could be REALLY large. You don't really want the max possible, you want the current number, right? Patch attached. The only really interesting part is that the bit where it passes a fake gid_t array to getgrouplist() works, but gets a SEGV on return from that function (on my RH9 Linux box, anyway). Changing fake to an array[4] and setting ngroups to 0 works, though. Haven't investigated further. Does this look more correct, though? ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-20 03:31 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From dtucker at zip.com.au 2004-02-20 14:31 ------- (From update of attachment 548)>diff -u -u -r1.42 uidswap.c >+ saved_egroups = xrealloc(saved_egroups, >+ saved_egroupslen * sizeof(*saved_egroups));I get a fatal here during authentication: debug1: temporarily_use_uid: 500/500 (e=0/0) xrealloc: zero size ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-20 03:32 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 dtucker at zip.com.au changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #539 is|0 |1 obsolete| | ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-23 04:42 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From djm at mindrot.org 2004-02-23 15:42 ------- (From update of attachment 548) There are two changes in this patch. Making the groups_byname array dynamic is OK, but I don't know about this:>+ } else { >+ char gidstr[32]; >+ >+ logit("getgrgid: unknown group id: %d", >+ (int)groups_bygid[i]); >+ snprintf(gidstr, sizeof(gidstr), "%d", >+ (int)groups_bygid[i]); >+ groups_byname[i] = xstrdup(gidstr); >+ }I not sure whether it makes sense, but is it a change in behaviour and should be a separate patch. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-23 19:59 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-24 06:59 ------- Would you rather the "unknown group id" string bit be dumped altogether. Previously (well, currently, I guess) the code just discards the byname entry for any groups without a name. This results in the bygid[] and byname[] arrays not being parallel for every index. If you want them to stay parallel, you need a filler for the gids without names. Alternatively, you could make them NULL and just catch that case in any code that touches groups_byname[]. So would you rather I: a) do it the old way and just drop out unnamed groups (and be non-parallel) b) stay parallel but make the unnamed entries be NULL (and fix ga_match() and ga_free()) c) stay parallel but add an informative error message (as it is now) I'll be happy to resubmit a patch post-haste if someone would care to make that decision. Probably a or b sounds fine to me. On further consideration, c isn't so tasteful. :) I'd like top get this patch off my plate, so I can wrap up the NGROUPS stuff. Anything else I need to do to make this go in in the near future? ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-23 20:06 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From mouring at eviladmin.org 2004-02-24 07:06 ------- I think we are in a lock for 3.8.. However I'm more in favor of the existing behavior. The only other issue is the one pointed out by Darren. "No secondary groups" - Ben ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-23 20:19 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-24 07:19 ------- I've got the "no secondary groups" thing fixed in my tree (I think). I'll go for existing behavior on this and repost the patch. I notice that this bug is now block #793 (3.8p1 release) so that is fine for me. As long as the patch is "approved" it is off my plate and onto yours :) ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-23 21:34 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 openssh_bugzilla at hockin.org changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #548 is|0 |1 obsolete| | ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-24 08:33 ------- Created an attachment (id=549) --> (http://bugzilla.mindrot.org/attachment.cgi?id=549&action=view) new NGROUPS patch This should work if you have no supplementary groups. It also keeps existing behavior for unnamed groups in groups_byname[]. Issue: the first call to getgrouplist() in groupaccess.c:ga_init(). On my unpatched RH9 box, this segfaults. On my RHEL3 box (should be just like RH9) it works. Based on stack examination, the getgroupslist() function on my RH9 box writes the gid list to the stack, heedless of the ngroups parameter. The RHEL box seems to do the right thing, except for wantonly assuming that at least ONE gid will be available and that ngroups is at least 1. So I work around the case of requiring one gid (not too gross), but what can be done about it ignoring the ngroups param on RH9? Nothing that seems reasonable. Fix glibc. So I think this is correct, or as correct as can be. More testers to confirm that would be nice. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-23 22:55 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From djm at mindrot.org 2004-02-24 09:55 ------- We could use OpenBSD's: $ wc -l /usr/src/lib/libc/gen/getgrouplist.c 94 /usr/src/lib/libc/gen/getgrouplist.c but from reading the manpage, it looks like the behaviour of returning the number of groups is not guaranteed - OpenBSD's implementation will return up to will return up to *ngroups, but not more. We may need OpenBSD's implementation anyway - getgrouplist isn't POSIX, it comes from 4.4BSD. I think we will need to feed getgrouplist with the maximum available, i.e: int ngroups; gid_t *gidlist ngroups = NGROUPS_MAX; #if defined(HAVE_SYSCONF) && defined(_SC_NGROUPS_MAX) ngroups = MAX(ngroups, sysconf(_SC_NGROUPS_MAX)); #endif gidlist = xmalloc(sizeof(*gidlist) * ngroups)); if (getgrouplist(user, group, gidlist, &ngroups) == -1) fatal xfree(gidlist); ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-23 23:36 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-24 10:36 ------- NGROUPS_MAX might be INT_MAX. You *can't* use it as an array size. We could replace getgrouplist() with a hand-rolled: int get_ngroups(const char *user); That would avoid the reliance on using getgrouplist() with a short list. How hard is it to walk the getgr* functions to count how many groups you are in? We can add the 'base' group, too and filter that out from the getgr* results. I bet it's 20 lines of code. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 00:13 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From djm at mindrot.org 2004-02-24 11:13 -------> NGROUPS_MAX might be INT_MAX.That would be utterly idiotic. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 00:22 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-24 11:22 ------- Not if there really is NO LIMIT to the number of groups. Or it might be 256k. Is it really worth a 256 to a meg of stack space for this? ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 00:26 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-24 11:26 ------- Created an attachment (id=550) --> (http://bugzilla.mindrot.org/attachment.cgi?id=550&action=view) NGROUPS patch with our own get_ngroups() Since getgrouplist() is stupid when we just want it to count the groups, we can roll our own. This might not be needed. The prior (-4) version of this patch SHOULD work on anything that supports getgrouplist() (modulo buffer overflow bugs in getgrouplist() which this is happy to trigger), which must be everything that openssh supports, since we use it. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 01:08 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From dtucker at zip.com.au 2004-02-24 12:08 ------- (From update of attachment 550)>Index: uidswap.c[...]>+ saved_egroupslen = getgroups(0, NULL);Should use get_ngroups here too, no?>Index: groupaccess.c[...]>+ struct group *gr = getgrent();If you're using getgrent() shouldn't you setgrent() first? ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 01:12 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From djm at mindrot.org 2004-02-24 12:12 ------- All the manpages that I see state that sysconf(SC_NGROUPS_MAX) is the canonical place to determine the maximum number of groups that an account can be a member of. Furthermore, the Linux 2.6.1 and glibc -current headers define NGROUPS_MAX as 32. Why would anyone set a *runtime* limit higher than the actual max number of groups in use for a single account? ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 01:13 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From djm at mindrot.org 2004-02-24 12:13 ------- Created an attachment (id=551) --> (http://bugzilla.mindrot.org/attachment.cgi?id=551&action=view) Size group array using sysconf, if available ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 01:16 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From djm at mindrot.org 2004-02-24 12:16 ------- Darren,>+ saved_egroupslen = getgroups(0, NULL);This does the right thing: it returns the number of supplemental groups that the process is a member of. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 01:20 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From dtucker at zip.com.au 2004-02-24 12:20 ------- Ah, OK, misread it. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 01:28 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-24 12:28 ------- Darren: getgroups() works properly, but yeah setgrent() is probably correct. Damien: Linux 2.6.3 will change NGROUPS_MAX to 64k. You *really* do not care what _SC_NGROUPS_MAX says - it is the MAXIMUM (actually, it's the minimum maximum, says POSIX. The actual maximum may be higher than sysconf() reports, if I recall correctly). All you care about is the ACTUAL count. You can get the actual count from a getgrent loop or from a proper getgrouplist() (which work on all systems, and not just HAVE_SYSCONF systems). I am pretty sure that either version will work (with a call to setgrent() to be pedantic in the get_ngroups() version) properly on the systems we care about. sysconf() will probably work but is sub-optimal and (to be pedantic) potentially wrong. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 01:30 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 openssh_bugzilla at hockin.org changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |openssh_bugzilla at hockin.org ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 01:41 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From djm at mindrot.org 2004-02-24 12:41 ------- SuSv3 says _SC_NGROUPS_MAX: "Maximum number of simultaneous supplementary group IDs per process". Sizing arrays using NGROUPS_MAX and/or sysconf is a very common idiom. Even the Linux manpages recommend this. Most systems don't even have anything like 64k groups, let alone accounts which are members of all of them. Have you not heard of optimising for the common case? What you propose means more complexity for every piece of downstream software that uses supplemental groups. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 02:05 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-24 13:05 ------- Well, there are very few bits of code that need hacking to work with 64k groups, so I have to discount the bit about extra complexity. Speaking of optimizing for the common case: this is called ONCE (unless I misread) per process. The real optimization is to use only as much memory as is strictly needed, though neither you nor I are actually optimizing anything at all. The runtime of this code is so far away from the fast path of anything that it's dumb to be arguing about. I should also mention that sooner or later _SC_NGROUPS_MAX may end up as an actual tunable in Linux. Again, you don't care what the maximum is, just what the actual number is. Further, since the patch(es) I proposed are VERY simple and work reliably, why would you opt AGAINST them, for something that is less precise AND might not be available on a platform (thereby falling back on today's buggy behavior). I can't see the reason for arguing that as a win. But, in the end, it's not my project, right? ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 02:10 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 djm at mindrot.org changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |RESOLVED Resolution| |FIXED ------- Additional Comments From djm at mindrot.org 2004-02-24 13:10 ------- Simpler patch applied. If necessary, we can revisit this under a different bug post-release if necessary. Tim, for a long time software has sized arrays using NGROUPS_MAX and/or sysconf. By changing NGROUPS_MAX, you break binary compat in scary ways. By making the baseline _SC_NGROUPS_MAX so high, you waste memory everywhere and force everyone else to do the work in cleaning up after you. Hopefully the glibc people will try to shelter userland from this silly and gratuitous change. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
bugzilla-daemon at mindrot.org
2004-Feb-24 02:16 UTC
[Bug 787] Minor security problem due to use of deprecated NGROUPS_MAX in uidswap.c (sshd)
http://bugzilla.mindrot.org/show_bug.cgi?id=787 ------- Additional Comments From openssh_bugzilla at hockin.org 2004-02-24 13:16 ------- I did not find as many examples of people (mis)using NGROUPS_MAX as you'd like to believe. Customers demanded more groups. So it had to grow. As I keep arguing - you DON'T want to know the maximum groups (in 99% of cases). You want to know the ACTUAL groups. And I am doing what I can to find apps like openssh that are broken, and help them to see the light. Once it is fixed, it's fixed. Cheers. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.