From owner-freebsd-security Mon Nov 13 10:33:57 1995 Return-Path: owner-security Received: (from root@localhost) by freefall.freebsd.org (8.6.12/8.6.6) id KAA17035 for security-outgoing; Mon, 13 Nov 1995 10:33:57 -0800 Received: from jhome.DIALix.COM (jhome.DIALix.COM [192.203.228.69]) by freefall.freebsd.org (8.6.12/8.6.6) with ESMTP id KAA16740 ; Mon, 13 Nov 1995 10:28:52 -0800 Received: (from peter@localhost) by jhome.DIALix.COM (8.6.12/8.6.9) id CAA03123; Tue, 14 Nov 1995 02:26:45 +0800 Date: Tue, 14 Nov 1995 02:26:45 +0800 (WST) From: Peter Wemm To: David Greenman cc: ache@astral.msk.su, committers@freebsd.org, security@freebsd.org Subject: Re: cvs commit: CVSROOT log_accum.pl In-Reply-To: <199511131630.IAA04150@corbin.Root.COM> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-security@freebsd.org Precedence: bulk On Mon, 13 Nov 1995, David Greenman wrote: > >>>Peter, do you have any progress in this issue for now? > >>>Maybe it is time to commit my fix to -current? > > > >> If we decide to change setlogin() so that it only works for session > >>leaders, then I'd prefer that we leave out the printf(). If you want to add > >>that to your own sources, fine, but I prefer to keep console noise minimized > >>to important failures. > > > >Of course. Printf introduced by Peter, I mean "return (EPERM);" here > >not a printf. I refer on my original fix and not to quoted variant > >from Peter. Setlogin must affect only _current_ session as clearly > >said in manpage (and from common sense), so no doubts here. > > The current behavior is not inconsistent with the manual page. It says > nothing about a requirement that the session *leader* must be the caller, > only that it affects the current session. Agreed.. If we were to go this way, perhaps make it so that only the session leader can change an existing name. ie: setlogin(..) { if (error = suser(p->p_ucred, &p->p_acflag)) return (error); if (!SESS_LEADER(p) && p->p_pgrp->pg_session->s_login[0]) { #ifdef I_WANNA_KNOW log(LOG_INFO, "setlogin attempted to change login name on non-session leader: pid %d; cmd: %s", p->p_pid, p->p_comm); #endif return (EPERM); } .. rest of setlogin()... } I think it's important that any test for session leaders is done after the suser() call, otherwise an attempt by a process to use the root-only call would not be flagged in the p->p_acflag variable. I wonder if this is really appropriate though. We are supposed to be able to trust root or setuid programs (they can call reboot() after all). I'm not convinced that making setlogin() fail for root is an inherently safe operation... I suspect the ideal fix would be to change the semantics to use something like the credentials system where it's reference counted and copy-on-write when a process changes it. I suspect it would be better for new processes to inherit the login name from it's parent, rather than have it wedged into the session structure. Perhaps there's room for the login name in struct ucred? It's already got the 16 or so "unsigned long"s (gid_t) in it for the supplemental groups.. what's another 8 bytes? :-) That would put setlogin() in the same class as setuid(), I suspect this may be the long-term correct method as it is "method of least suprise". BTW: I suspect "struct ucred" should be reordered for better internal alignment.. It is currently: struct ucred { short cr_ref; long cr_uid; short cr_ngroups; long cr_groups[NGROUPS]; } The order of cr_ngroups and cr_uid could be swapped making the whole thing 4 bytes smaller (assuming that I understand structure packing.. :-) -Peter > -DG >