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. > 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. > 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. Complexity is a _serious_ disadvantage to a module like this. > o suid binary helper should accept username as argument -- I still > think that it should use getpwnam() instead of getpwuid(). Thus, > it will not be compatible with current pam_unix implementation. This is fine with me. > 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. Why would you "have trouble stacking modules" without asking the password from prelim_check? > 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 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. > 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. > # 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. > Comments for [uw]tmp work. We should open /var/log/wtmp > and /var/state/utmp files in open_session and close them > only in close_session, so that changing uids and chroot > will not affect this. But in this case we need to have You shouldn't leave an open fd around. If an application needs to chroot and/or switch to non-root, it currently has to fork and keep a root process specifically to close the PAM session. I don't like this, but this is how things work now and nothing obviously-better has been proposed. > (non-suid) helper. Note that in forked process we should > close some files that can be opened by application, e.g. > if it has some pipes opened. PAM modules shouldn't fork. > enforce_root (proposed) > ask current password and verify it's expiration even if current uid=0. > Normally module will not ask current password in this case. Speaking of the syntax, I have an enforce= option in pam_passwdqc, and it accepts one of three values: none, users, everyone (there's also a "root" flag internally, but it didn't make sense to enforce password policy for just root and not the users). Perhaps you could borrow this syntax (and use "everyone" for what you describe), even though in your case this applies to authentication. > md5 > use md5 algorithm to encrypt password (by default it is encrypted > using system's crypt routine). 1,$s/encrypt/hash/g 1,$s/crypted/hashed/g ;-) > 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. > 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. > 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. Signed, Solar Designer
