cvs commit: src UPDATING (initgroups)

[Brooks Davis]

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. 
> >>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. 
> >
> >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.
> 
> I have checked all instances in our code where initgroups(3) is called. 
>  Appart from a single case, where the error value is silently 
> propagated upward (openpam_borrow_cred), in all other cases the returned 
> value is either checked and appropriately reported or ignored (see 
> 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 
> setusercontext(3).  The call should have generated a syslog message of 
> 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 
> 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 
> group memberships more than NGROUPS being silently ignored), I agree 
> that we might want to help users check their systems.  The following 
> script will check a typical group(5) file and report cases where 
> setgroups would overflow.
> 
> #!/bin/sh
> awk -F'[:,]' '
> { for (i = 4; i <= NF; i++) if (length($i)) g[$i]++; }
> END { for (u in g) if (g[u] > '`sysctl -n kern.ngroups`' - 2) print "Too 
> many group memberships for user " u }
> ' /etc/group
> 
> 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

-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-src/attachments/20031214/b473dc76/attachment.bin