Solar Designer wrote:
>
> Hi,
>
> What you propose mostly looks fine. I've got a few comments, though.
>
> > o algorithm used to determine if we should lookup a shadow entry
> > and if we need (re)se uid for this (for NIS+ etc) -- it is
> > implemented the same as pam_unix currently have, until we will
> > have a better solution
>
> There should be a way to ensure that no set*id calls are done by the
> module (disabling NIS support). This will simplify code audits for
> those of us who aren't going to use anything other than shadow.
Maybe next time -- I don't want to go so away in spite of compatibility.
I'll place #ifdef here -- another complecity, but at least _all_in_one_place_,
unlike in current pam_{unix,pwdb} code. There is _only_one_ function to
deal with getspnam() in my implementation, and in one small separate file.
BTW, this file can have two versions -- with and without that setreuids(),
e.g. "getshadow_setuids.c" and "getshadow_nosetuids.c" and link to
one of these named "getshadow.c".
> > o we always logs entered username (even if user may have (mis)typed
> > his password instead of username -- we can't determine this)
>
> This is inacceptable for me. If a username isn't know or it is not
> possible to determine whether the username is known at this time,
> then don't log it (and the authentication should fail, of course).
> Possibly add an option to log unknown usernames, but the default
> should be to not log them.
> > # There are some comments in current pam_unix about possibility
> > # for the user to type his password instead of username, and
> > # about carefulness while logging username. I don't think
> > # this is useful -- we need to log username that was tried in
> > # any case -- if we will not log it, we will lost the ability
> > # to track some attacks.
>
> Yes, so what? We can't log all possible attacks anyway.
>
Ok, so "audit" option should go back ;) (or similar)
Seriously, for this to work, all other applications (login etc) that
uses PAM should also be audited/examined/etc. Login from linux-utils,
for example, always logs a line something like:
login[1234]: authentification failed for <USERNAME>: User not known to PAM module
The <USERNAME> part comes from PAM_USER that is set by login itself.
So, login etc should _NOT_ log this if error is PAM_USER_UNKNOWN.
Agreed, in summary. ;)
> > o nullok and nonull semantics: nullok will permit login without
> > prompting a password (actually, _any_ password can be used)
> > if user's password (or shadow) entry is empty. Nonull will not
> > permit the usage of empty password, even if it matches the crypted
> > one stored in system. There is a minor defference in this --
> > two options are not opposite of each other. Moreother, them
> > can be used simultaneously :)
>
> I understand your desire to maintain pam_unix compatibility, but I
> think this gets overly complicated. You mentioned that The Right
> change would break xlock for the case of empty passwords. I don't
> think this is a reason to add workarounds into your PAM module.
This have _very_ little overhead in complecitly.
> Complexity is a _serious_ disadvantage to a module like this.
Yeah, I know this. Thanks for reminder.
[]
> > o it is not clear to me if I understand PAM_PRELIM_CHECK/PAM_UPDATE
> > logic correctly. I verify (possible ask) the current password then
> > first flag set, and change (and possible ask) the new password then
> > second is set (current pam_unix does the same). Is this a right
> > think to do to ask current password in prelim_check? If not, then
> > we have troubles stacking modules
>
> I think you've got it wrong. It was my understanding that the
> prelim_check is for checking the availability of resources required
> for the module's functionality, such as configuration files.
This is how things implemented in current pam_unix. This also seemed
strange to me, and so I asked about it.
pam_sm_chauthtok():
---
...
obtain username (in all cases, regardless on PAM_PRELIM_CHECK/PAM_UPDATE)
...
if (on(UNIX__PRELIM, ctrl)) { -- in case PAM_PRELIM_CHECK
/* obtain and verify the current password (OLDAUTHTOK) for the user. */
..
} else if (on(UNIX__UPDATE, ctrl)) { -- in case PAM_UPDATE
/* prompt for new password etc, and change it */
...
}
...
---
> > o flags that was removed but exists in current pam_unix, and
> > explanation:
> >
> > bigcrypt
> > ok, it is not so good as md5, why it ever needed when "plain"
> > crypt() and md5 exists?
>
> This is fine with me. In fact, you could want to drop the md5 flag
> as well and switch to the syntax I'm using in my patched pam_pwdb,
> which looks like:
>
> prefix= -- will use the traditional DES-based hashes
> prefix=xy -- the same (could use any valid salt)
> prefix=$1$ -- FreeBSD-style MD5-based hashes (replaces "md5")
> prefix=_ count=100001 -- BSDI/FreeSec extended/"new-style" DES-based hashes
> prefix=$2a$ count=8 -- OpenBSD-style Blowfish-based hashes
What's the `count' here?
> Ideally, the PAM module should know nothing about these or other
^^^^^^^
> supported hash types. It shouldn't know how to process the prefix or
> the count, -- these are to be passed into crypt_gensalt in libcrypt.
> This is how things work on my systems (well, those that have PAM at
> all). However, as your module shouldn't depend on a particular libc,
> you should probably provide the ability to generate salts for the
> traditional (you call it plain) and MD5-based hashes (but not the
> password hashing code itself), this should be about 2 KB of source in
> a separate source file.
One gory detail is that we will be totally incompatible with systems
that have no support for such a nice things in -lcrypt (and also that
uses defferent hashing algo that is not supported in our implementation).
I thinked about this, and have no good solution (and I have no -lcrypt
that supports this kind of crypt that you mentioned also ;) -- I need
to generate a correct salt (different for md5 and DES) even for glibc's
"smart" crypt() routine now, and have a little trouble to choose a good
salt for md5 -- in current pam_unix it is a md5(f(gettimeofday(),getpid()))
that is good enouth, but if I does not have md5 implementation (I wan't
include it in pam_unix at least if system have smart crypt() like glibc's
one), I should use another routine for salt generation).
Ok, for now all this stuff also goes to two separate files -- one for
your variant and one for "compat" variant. :(
> > shadow
> > It was used only in passwd stack. Since files is only one storage
> > that new pam_unix supports (there is nis also in current pam_unix),
> > and since new module will authomatically determine if shadow or
> > plain password file should be used, this is unnecessary
>
> Well, unless you drop the support for hashes in /etc/passwd (which is
> an acceptable thing to do for the sake of simplicity), there's still
> some use for this option: it can make sure the password hash doesn't
> get written into /etc/passwd even if the password files are corrupt
> in a way that would currently make your module think that shadow isn't
> in use for an entry.
Wow, corrupted passwd/shadow files is another separate and rather big story :(
And I don't think that we should consider this case here (but should have it
in mind) -- if I'll think about this, I'll also think about replacing
getspnam()/getpwnam()/etc calls by my own versions that will very carefully
look to contents of the files, and, as a result, nsswitch mechanics will
not be used. And there will be _A_LOT_ of new complicitly/bugs/etc. Ooch! :^8
> > When changing password, module looks if the user have a shadow entry,
> > by searching /etc/shadow first and when (if not found) looking to
> > plain passwd entry in /etc/passwd trying to change password there.
> > # This is not consistent with auth stack. Probably I should first
> > # check if passwd entry (from getpwnam()) equals to "x" and attempts
> > # to open shadow only if it is; and should try to open
> > # /etc/passwd file if passwd != "*NP*" (well, does not contains "*").
>
> I think it is simpler and more reliable to re-introduce "shadow" and
> only look for the password in either shadow or passwd, not both.
But Ok, this makes sence -- if "shadow" option is given, than do not even
try to update /etc/passwd (and, for auth stack, always look to shadow,
even if pw_passwd value is not "x" or "*NP* etc).
> > Module will lock the passwd/shadow file while it updates information,
> > but _not_ when it verifies user's current password or asks for a new
>
> This is correct. The password file should be consistent all the time
> and only replaced with a rename(2).
>
> > one. This is to avoid long period of lock when user enters his
> > password.
>
> Huh? You wouldn't have to lock the file for this long even if
> read-only access required locking.
Ok, this needs some explanation. Again, this is how things are implemented
in current pam_unix:
pam_sm_chauthtok() {
/* the first thing this routine does is: */
lckpwdf(); /* I looked to your patches for pam_pwdb in this respect.
There is no return value checking here!! */
...
/* obtain username, unlock/return if failed */
if (on(UNIX__PRELIM, ctrl)) {
/* verify current password: ask for it from application,
possible very long delay while user entered the current password */
ulckpwdf();
return;
}
else if (on(UNIX__UPDATE, ctrl)) {
for (try = 1; try < maxtries; ++try) { /* note loop here! */
/* obtain the new password. Also, very long delay possible */
}
/* update (change) password */
ulckpwdf();
return;
}
....
}
As you can see, it locks (well, attempts to) password file (in
incompatible (with what?) way using LCKPWDF) in the first line of
routine, and unlocks it only at return (I'm just lazy to check that
current core unlocks password back _in all places_ where it returns
from pam_sm_chauthtok() -- doh!).
> > When changing a password, module will always compares current password
> > with entered by user to enshure that noone has changed this password
> > while
> > user entered a new one.
>
> This is a nice property if you do this second check after locking the
> password file.
This is exactly that I do. :)
I want to obtain current system password just before comparing it
with that user has entered (when autentificating, in both auth and passwd,
stack, and with possibility to verify that system password is not empty, for
nullok), and in passwd stack, I'll also compare PAM_OLDAUTHTOK with
hash in shadow file just before I write new line to it, like this:
lckpwd() -- this is my routine, do not confuse with current pam_unix
while (read(line, from shadow file)) {
if (line is a user's entry) {
compare(OLDAUTHTOK, passwd(line)) <----
write(new entry to new file)
found = 1;
}
else {
write(line to new file)
}
}
rename(/etc/shadow, /etc/oshadow);
rename(/etc/nshadow, /etc/shadow);
unlockpw();
This is very simplified code with many checks ommitted (yes, I know about
ferror() etc! :), just to show the idea.
> Signed,
> Solar Designer
Thank you!
Your comments are very useful, really.
Regards,
Michael.
