[ I've added the security-audit list to the CC:, as most of my
answers are to security-related questions. ]
> 1.c. It will be nice if we can determine _why_ shadow entry unavailable.
> If getspnam() returns NULL, what a cause? Maybe it just does not
> exists,
Yes, and the same applies to other get{pw,sp}* functions. In
particular, don't repeat the mistake of pam_unix and libpwdb where
they assume that a NULL return from fgetpwent() and fgets() means
EOF. Both can lose data when updating the password file.
I have a patch for this (and other potential issues) for libpwdb, it
is to use ferror() after fgets(). I'm afraid there's no portable
solution for the case of using fgetpwnam(), so you should probably
avoid it when re-writing the password file.
> If we determined that system password is empty, we can not ask user to
> provide password, but if it already provided some password, we should
> check
> if it is empty... Maybe I'm not right here?
My opinion is that you're right.
> But wait, there is already one application that can broke if the "right"
> behavour will be restored. It is xlock -- it will not allow to press
> "Ok" button untill user fills password field! So, if user has really
> empty
> system password, he will be unable to unlock his screen...
And what's the point in locking an X session without a password? The
user needs just a screensaver, then. xlock should probably be fixed.
> Also around this -- if module flags does not permits usage of null
> passwords,
> but user has empty password in _encrypted_ form. In this case, his
> password
> field will not be empty (e.g. crypt("", "aa") == "aaQSqAReePlq6").
> Should we disallow access in this case also, by checking entered
> password,
> not only stored one?
If "nullok" is not set, then you should not allow an empty password,
no matter how it is represented in the password file.
> 3. setuid binary helper. I see a little demand on this (I was very
> curious why it was implemented, untill I found at least one application
> that can rely on it -- it is xlock and the like). Should we really
> implement this helper? Maybe xlock should have it's own helper instead?
Implement, probably.
Install SUID by default, no. If a package makes use of your PAM
module in its default /etc/pam.d/ file, then it is aware of your
module by definition, so it can also enable the helper if it plans to
do authentication as non-root. There's only one problem: disabling
the helper when the last package that needs it gets uninstalled. I
doubt distribution makers will care to implement this, though. :-(
I just don't like the idea of installing something SUID when it may
not be needed.
> And if should:
> it seemed to me that this helper should accept a username entered, and
> not
> rely on getuid() -- if we have some "users" shared one uid, e.g. for
> mail access, this helper should _not_ check "mail owner"'s password,
> but mail user's one... In other words, it should accept username,
> and verify if this username have uid equal of those user running the
> helper, and _not_ as it does currently (checks password of user
> determined
> by uid).
Well, while I have multiple UID 0 accounts on some of my systems
(such as, separate for admins and backup scripts running via SSH),
such things aren't officially supported. /etc/security script on
*BSD's will complain about them. Many programs will use getpwuid()
to locate the home directory and get their configuration file from
there. Basically, I don't think you should be spending your time on
incomplete support for this.
> is should be unnecessary to call helper from pam_unix when there is
> no password record available (getpwnam() returned NULL) or if uid
> returned by getpwnam() does not match our uid returned by getuid(),
> isn't it?
Huh? Both sound wrong to me. BTW, current pam_pwdb (probably
pam_unix as well, haven't checked) does the wrong thing: it will run
the helper binary even when the user being authenticated doesn't
match current UID, so it is possible to authenticate as any user
using the current user's password. Fortunately, this can only happen
when running as non-root, so there will be no UID change.
You should be running the helper binary if not running as root and
the target username matches the current UID (the one that the helper
binary will use).
> DoS attacks for example...
If someone DoS'es your system into returning NULL from getpwnam(),
so, well, they've succeeded in a DoS attack on your authentication.
You can't and shouldn't try to recover from this with a helper binary
or whatever.
> 3.a Maybe this helper should be used to change password also?
No.
> Why not just use:
>
> pam_converse(..., &resp, ...);
> pass = resp[0].resp; resp[0].resp = NULL;
> pam_drop_reply(resp);
pass would become invalid after the pam_drop_reply() in your code. I
agree that extra strdup()'s should be avoided, but you need to put
your response processing (which is hopefully only a few lines) before
the pam_drop_reply(), and be sure you have no return's in there.
> 4.b. Is there any way to clear shadow file buffer, and should we clean
Maybe look into using setbuf() + fgetspent(). Let me know if it
works good for you, as I have this on my TODO as well.
> it and other shadow (crypted) passwords so carefully? I see e.g.
The current code isn't careful enough in doing this. There're a lot
of data that isn't left in the address space due to pure luck, and
there's enough for an attack that is left.
My opinion is that we should either change the Linux-PAM docs so that
they stop requiring that the modules should leave no sensitive data
(as an application developer could count on this), or modify the code
(PAM modules + possibly glibc + possibly gcc) to actually achieve
this (very useful) goal.
> `pam_overwrite(salt); salt=NULL' code fragments -- are them necessary
> without cleaning up buffers that are used by getspnam() etc?
You could want to add the crypt(3) output buffer to your list of
things to clear. Unfortunately, there's no standard that says
crypt(3) can't return a pointer to a read-only location on error, but
I think it should be safe to assume that the buffer is writable if
authentication succeeds. Of course, you should clean the buffer for
the case of failed authentication as well, but this is currently
tricky. Perhaps you could check for a recent enough version of glibc
(2.2+, I'm afraid, as md5crypt has only been fixed to clean other
internal buffers just recently, and the old UFC-crypt code I was too
lazy to fix) and go ahead with the memset().
(Of course, my opinion remains that you should have no password
hashing code in a PAM module like this.)
Finally, to be nearly(*) sure there's nothing left, you need either
modifications to gcc or you can use the approach suggested by Richard
Henderson:
1. Switch to a private stack for the calls to functions that process
sensitive data. You can use MAP_GROWSDOWN to avoid allocating more
memory than you need and avoid wasting the CPU on zeroing it out (the
data will remain, but not be accessible from the user-space, which is
enough in our case).
2. Use a piece of assembly code to clean your registers, register
windows, and whatever other relevant storage exists on your
architecture.
(*) Having in mind things like internal CPU registers that could turn
out to be accessible from the user-space. (The Pentium RDMSR bug was
almost this, except that RDMSR is a privileged instruction.)
I am not sure whether we should go for this level of complexity, but
if we don't, the documentation for Linux-PAM needs to be changed to
explain the level of security PAM modules are expected to provide, so
that application developers don't make wrong assumptions.
Of course, a much simpler way to achieve the desired level of security
is to pass an execve(2) (as in checkpassword) or fork(2) a temporary
child for just the authentication (as in popa3d). BTW, Linux-PAM docs
should be explicit about the calls that are valid from a process other
than the one that is to stay as the service.
> 4.c. Can anyone comment -- does we really need to support "brokencrypt"
> (that was a bug with endianess issues) that was discussed in this
> list already?
No. And you don't need to have any password hashing code in your
module. Those who want compatibility can use the old modules.
You should also make it easy to replace your salt generation code
with a libc/libcrypt call (have that code in one place).
> 4.e. Counting of unsuccesseful auth attempts in pam_unix seemed to me
> also strange. It stores one counter together with username. It should
> be some demand of doing so, and I just can't find it... Note that this
> approach can be used for implementing some DoS attack -- give many
> huge "usernames" -- all will be in memory, and we will have all memory
> used, but no "too many attempts" error (I'm not shure here)...
> I don't really understand cooperation between pam module and application
> here -- in respect of "attepmts" -- for example, pam_cracklib uses it's
> own loop attempting to ask new password, but pam_unix does not.
> What is the Right Thing and where (i.e. in what pam entry/stack)?
I'd implement a loop similar to one found in pam_cracklib (but cleaner).
> 4.f. bigcrypt() implementation in both pam_pwdb and pam_unix seemed to
> be broken. I don't understand it (just don't looked to it really), but
> verified it -- trivial program that calls bigcrypt with two command-line
> argument and prints result showed me that "big" prefix in bigcrypt
> does nothing -- password with 8 letters crypts the same way as any
> longer
> password that have same first 8 letters. I.e. result from bigcrypt
> is the same as from plain crypt. What's happened here!?
It could be buggy, or it could be your testing that's buggy, but you
should drop bigcrypt entirely either way. It's insecure.
> 4.g. Should we ever support "plain" crypt?
Yes, because you have it in libc anyway, and because many systems
have such password entries. If you were to drop obsolete crypt's,
md5crypt would be among them, as well. ;-)
> But I can argue against "bigcrypt" option -- it should be noop.
It says that "bigcrypt shouldn't be used for new passwords" here, but
I would drop the option entirely if I was writing a new module from
scratch like you do.
> 4.h. currently pam_unix always sets PAM_AUTHTOK (or private item) after
> asking password _before_ it checking. It should set this item only
> after
> _successeful_ checking, and clear it on each unsuccesseful attempt,
> isn't it?
I don't think this matters, as PAM_AUTHTOK can also be set by modules
that don't do authentication at all (pam_cracklib and the like).
> And it should clear it if it permits empty pass (see 2. also), isn't it?
I think pam_end() would take care of that, but such logins shouldn't
be permitted anyway.
Signed,
Solar Designer
