openssh bugs - Feb 2004 - [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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.