From owner-cvs-all@FreeBSD.ORG Sun Dec 14 13:36:36 2003 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 13B4A16A4CE; Sun, 14 Dec 2003 13:36:36 -0800 (PST) Received: from odin.ac.hmc.edu (Odin.AC.HMC.Edu [134.173.32.75]) by mx1.FreeBSD.org (Postfix) with ESMTP id 535A343D09; Sun, 14 Dec 2003 13:36:34 -0800 (PST) (envelope-from brdavis@odin.ac.hmc.edu) Received: from odin.ac.hmc.edu (IDENT:brdavis@localhost.localdomain [127.0.0.1]) by odin.ac.hmc.edu (8.12.10/8.12.3) with ESMTP id hBELaPA7006398; Sun, 14 Dec 2003 13:36:25 -0800 Received: (from brdavis@localhost) by odin.ac.hmc.edu (8.12.10/8.12.3/Submit) id hBELaP8s006397; Sun, 14 Dec 2003 13:36:25 -0800 Date: Sun, 14 Dec 2003 13:36:25 -0800 From: Brooks Davis To: Diomidis Spinellis Message-ID: <20031214213624.GA4077@Odin.AC.HMC.Edu> References: <3FDC7D65.3040406@aueb.gr> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="yrj/dFKFPuw6o+aM" Content-Disposition: inline In-Reply-To: <3FDC7D65.3040406@aueb.gr> User-Agent: Mutt/1.5.4i X-Virus-Scanned: by amavisd-milter (http://amavis.org/) on odin.ac.hmc.edu cc: src-committers@FreeBSD.org cc: Robert Watson cc: Brooks Davis cc: cvs-src@FreeBSD.org cc: dds@FreeBSD.org cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src UPDATING (initgroups) X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 14 Dec 2003 21:36:36 -0000 --yrj/dFKFPuw6o+aM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Dec 14, 2003 at 05:10:29PM +0200, Diomidis Spinellis wrote: > Robert Watson wrote: > >On Sat, 13 Dec 2003, Brooks Davis wrote: > >>A similar change is needed in 4.x or the change should be backed there.= =20 > >>I think we should back it out (in stable) until the various users of > >>initgroups are fixed to output something useful and, preferably, not > >>exit when this happens. As it is, we turned an rarely hit edge case > >>that was somewhat difficult to debug into a fatal error that that is > >>about equally difficult to debug and breaks the account in question.=20 > > > >Sigh. I agree. I didn't realize the MFC had happened, but sure enough, > >it's change 1.3.8.2. I have somewhat mixed feelings: I feel strongly we > >should generate an error and fail closed, but I also agree that the > >transition period was too short (we should have a warning period on the > >-STABLE branch), and that we need to do something about error handling. > >I'm surprised more people haven't bumped into it, but I guess the MFC was > >only a couple of days ago. >=20 > I have checked all instances in our code where initgroups(3) is called.= =20 > Appart from a single case, where the error value is silently=20 > propagated upward (openpam_borrow_cred), in all other cases the returned= =20 > value is either checked and appropriately reported or ignored (see=20 > attached file). So, error handling is not a problem. Error handling IS a problem. With a fairly standard configuration, accounts with >16 groups can not log in at least under STABLE. > The problem reported in current@ was probably through a call to=20 > setusercontext(3). The call should have generated a syslog message of=20 > the form: > "initgroups(kjwolf, 1000): invalid argument" > EINVAL is now appropriately documented in setgroups(2): > "The number specified in the ngroups argument is larger than the NGROUPS= =20 > limit." > so again, the error should be visible in the hosts syslog. I don't think a syslog message mentioning "invalid argument" is sufficent in STABLE. We've turned accounts with a minor problem that few people noticed into accounts that can't login. I don't think it's reasionable to force admins to back trace from "invalid argument" to EINVAL to a non-standard meaning listed in the function call manpage, espeicaly since we could emit a useful error instead. > Given that this type of error was silently ignored in the past (with=20 > group memberships more than NGROUPS being silently ignored), I agree=20 > that we might want to help users check their systems. The following=20 > script will check a typical group(5) file and report cases where=20 > setgroups would overflow. >=20 > #!/bin/sh > awk -F'[:,]' ' > { for (i =3D 4; i <=3D NF; i++) if (length($i)) g[$i]++; } > END { for (u in g) if (g[u] > '`sysctl -n kern.ngroups`' - 2) print "Too= =20 > many group memberships for user " u } > ' /etc/group >=20 > I suggest we add it in the corresponding UPDATING entry/entries. This is insufficent. It would not have caught the case we saw at work because the user got the extra groups from NIS. -- Brooks --=20 Any statement of the form "X is the one, true Y" is FALSE. PGP fingerprint 655D 519C 26A7 82E7 2529 9BF0 5D8E 8BE9 F238 1AD4 --yrj/dFKFPuw6o+aM Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.1 (GNU/Linux) iD8DBQE/3NfXXY6L6fI4GtQRAkA2AJ487A8AN1FMZ8hSTWHkuYW+N77RhgCdEP+P S6k9RbxzIKsvhSFdl09Dm9k= =nxiU -----END PGP SIGNATURE----- --yrj/dFKFPuw6o+aM--