cvs commit: src/lib/libpam/modules/pam_unix pam_unix.8 pam_unix.c

[Yar Tikhiy]

On Sat, Apr 28, 2007 at 04:17:49PM +0300, Alexandr Kovalenko wrote:
> Hello, Yar Tikhiy!
> 
> On Fri, Apr 27, 2007 at 08:26:14PM +0400, you wrote:
> 
> > On Thu, Apr 26, 2007 at 01:54:59PM +0300, Alexandr Kovalenko wrote:
> > > Hello, Yar Tikhiy!
> > > 
> > > On Thu, Apr 26, 2007 at 06:39:01AM +0000, you wrote:
> > > 
> > > > yar         2007-04-26 06:39:01 UTC
> > > > 
> > > >   FreeBSD src repository
> > > > 
> > > >   Modified files:        (Branch: RELENG_6)
> > > >     lib/libpam/modules/pam_unix pam_unix.8 pam_unix.c 
> > > >   Log:
> > > >   MFC:
> > > >           pam_unix.c      1.52
> > > >           pam_unix.8      1.13
> > > >   
> > > >     In account management, verify whether the account has been locked
> > > >     with `pw lock', so that it's impossible to log into a locked account
> > > >     using an alternative authentication mechanism, such as an ssh key.
> > > >     This change affects only accounts locked with pw(8), i.e., having a
> > > >     `*LOCKED*' prefix in their password hash field, so people still can
> > > >     use a different pattern to disable password authentication only.
> > > 
> > > Using the very same logic you should also add checking for '*', and for
> > > any other string, which cannot be in password hash of different
> > > algorithms. By the way, what if some crypto algorithm, which will be
> > > used for password hashing can produce hash, which contains substring
> > > '*LOCKED*' ?
> > 
> > Please don't over-generalize.  My change adds a check for a *LOCKED*
> > prefix only, which cannot appear in a password hash unless its current
> > format is broken.  Neither an old DES hash nor a new multi-algorithm
> > hash can start with *LOCKED*.
> 
> /sbin/nologin was used to disable logging in. There is also UsePAM

That would lose the user's original shell.  OTOH, prepending *LOCKED*
to the password hash is totally reversible.

> option of sshd_config, why then UsePAM yes and UsePAM no should behave
> in a different way?

The funny fact is that now sshd(8) behaves consistently, and it's
partly due to my change.  Earlier OpenSSH started to check for
*LOCKED* by itself if configured for FreeBSD, and it happened without
my help or prodding although I'd been for the change.  The check
is reached only if UsePAM is set to `no', OpenSSH relies on PAM to
do all the local auth stuff otherwise.

> > > But anyway, I think that it is not expected behavour of sshd/pam_unix.
> > > 
> > > >From the pw manual page:
> > > 
> > > USER LOCKING
> > > 	 The pw utility supports a simple _password_ locking mechanism for
> > > 	 users; it works by prepending the string `*LOCKED*' to the
> > > 	 beginning of the password field in master.passwd to prevent
> > > 	 successful authentication.
> > > 
> > > Please note word _password_. There is nothing about locking _account_
> > > completely.
> > 
> > I believe account locking was implied in the days pw(8) was written.
> > 
> > > Please consider reviewing this PR and, hopefully, back out this commit.
> > > At least for a lot of people - it is POLA violation.
> > 
> > Just run adduser(8) and see how it implements account locking and
> > password auth disabling.  That's the system policy my change is in
> > keeping with.
> 
> That's the better argument, than pw(8). Agreee with you.
> 
> But, then you should document it well, check every place where *LOCKED*
> is used in source code and, I suppose this change worth some HEADS UP or
> something like that. Beacuse this change is really may confuse ppl, who
> were relying on the "wrong" behavour for years.

Sure, as soon as we reach total consensus on this issue.

> P.S. Fast and dirty search in sources showed only this:
> 
> crypto/openssh/ChangeLog:   "*LOCKED*" string) for FreeBSD.  Patch jeremie at le-hen.org and
> crypto/openssh/config.h:#define LOCKED_PASSWD_PREFIX "*LOCKED*"
> crypto/openssh/configure.ac:    AC_DEFINE(LOCKED_PASSWD_PREFIX, "*LOCKED*", [Account locked with pw(1)])
> crypto/openssh/sshd.8:.Ql \&*LOCKED\&*
> usr.sbin/adduser/adduser.8:.Dq Li *LOCKED*
> usr.sbin/pw/pw.8:.Ql *LOCKED*
> usr.sbin/pw/pw_user.c:static            char locked_str[] = "*LOCKED*";
> usr.sbin/pw/pw_user.c:           * a "*LOCKED*" prefix from in front of the password to
> 
> -- 
> NEVE-RIPE, will build world for food
> Ukrainian FreeBSD User Group
> http://uafug.org.ua/

-- 
Yar