Date: Tue, 14 Nov 1995 02:26:45 +0800 (WST) From: Peter Wemm <peter@jhome.DIALix.COM> To: David Greenman <davidg@Root.COM> Cc: ache@astral.msk.su, committers@freebsd.org, security@freebsd.org Subject: Re: cvs commit: CVSROOT log_accum.pl Message-ID: <Pine.BSF.3.91.951114020403.353D-100000@jhome.DIALix.COM> In-Reply-To: <199511131630.IAA04150@corbin.Root.COM>
next in thread | previous in thread | raw e-mail | index | archive | help
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
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.3.91.951114020403.353D-100000>