> >
> > 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
I haven't seen your code, -- do you think that checking a flag or a
compile-time macro in places that may do set*id calls would add too
much complexity?
> 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".
OK, this is acceptable.
> > > # 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.
Of course, they should be audited. This is what I am doing. There're
enough other reasons for this.
> Login from linux-utils,
> for example, always logs a line something like:
> login[1234]: authentification failed for <USERNAME>: User not known to PAM module
I'm not going to use login from util-linux on PAM'ified systems, and
I've made sure the login I do use with PAM (currently SimplePAMApps
with patches) doesn't do things like this.
-rw-r----- 1 build sources 11154 Aug 11 11:19 SimplePAMApps-0.60-owl-login.diff
Currently, it relies on the PAM modules to do some meaningful logging
in case of failed authentication, which is probably wrong. :-(
> 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.
Yes, perhaps this is what should be done. The modules then need to
be even more careful about their use of return values.
> > > o nullok and nonull semantics: nullok will permit login without
> > 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.
Well, this adds complexity to the documentation: two options with
non-obvious semantics.
> > > 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.
Did you get an answer from Andrew? This is really not specific to
just pam_unix.
> > 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?
It's the iteration count for modern password hashing algorithms, to
be encoded as a part of the "salt".
Your module shouldn't know anything about the "count". When compiled
with a regular libc, it won't support the modern hashes, and thus the
"count=" is of no use. When compiled with a libc/libcrypt providing
crypt_gensalt(), it should be able to extract the integer argument and
pass it to crypt_gensalt(), which is just a few lines of code.
> > 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
No.
> uses defferent hashing algo that is not supported in our implementation).
There should be no systems that don't support the traditional
crypt(3). There will be some non-Linux/*BSD systems that don't have
the MD5-based crypt(3) in libc, but that's life. (Actually, I don't
think Linux-PAM works on those at all.) Don't forget that there is
the current pam_unix for those very few who will need the backwards
compatibility brokenness for some reason.
> I thinked about this, and have no good solution (and I have no -lcrypt
I think I am suggesting an acceptable one. Perhaps my explanation
isn't good enough...
> that supports this kind of crypt that you mentioned also ;) -- I need
You can get a patch for glibc at the URL I've mentioned, but this
isn't really required as your module should compile on a regular libc
as well (without part of that extended functionality).
> to generate a correct salt (different for md5 and DES) even for glibc's
Yes, you need salt generation code for those two hash types for the
case of regular libc. That's those 2 KB of source I've mentioned.
> "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()))
This is both overly complicated and not as good as one can do with
the randomness sources we have on Linux and *BSD's.
> 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).
Yes. You need to read the 6 bytes from /dev/urandom and encode them.
You can take the /dev/urandom access code from my pam_pwdb patch, and
the salt generation from my crypt_blowfish package.
> Ok, for now all this stuff also goes to two separate files -- one for
> your variant and one for "compat" variant. :(
Why the ":("? This should be two tiny files, of about 2 KB each.
Actually, the /dev/urandom access code can be common to both, so you
can have one 3 KB large (or small;-) source file.
> Wow, corrupted passwd/shadow files is another separate and rather big story :(
It's not entirely separate in that you should do your processing in a
fail-close way.
> 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
No. If libc functions do something in an unsafe way, but the interface
doesn't require them to do so, then they should be improved. get*nam()
can be written in such a way that they return NULL in case of detectable
errors (I/O or wrong file format), which is the safe behavior. (We've
already discussed that it would sometimes be nice to be able to tell
whether there was an error or the username didn't exist, but this isn't
required for the case of authentication.)
> 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;
> }
> ....
> }
Well, this looks broken.
I don't think you have a reason to do things in such a broken way.
> 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();
Yes, this can work.
> Your comments are very useful, really.
Thanks for listening to so many different opinions and trying to
develop something that would be useful to all of us.
Signed,
Solar Designer
