Bug 151914
|
Description
Kathy Whyte
2005-03-23 15:24:55 UTC
The pam_ccreds module could fork/exec a setuid program that could read and write .security.db. This would be similar to unix_chkpwd. SELinux could be used to further protect the use of this program. Yea, that's the correct fix. I vaguely remember talking about this before. Okay, I just looked at the code for unix_ckpwd and pam_ccreds. I think I could implement this. Will you accept a decent patch to pam_ccreds? Eventually, it should get upstream. Sure! That'd be great! Thanks! Kathy Whyte Created attachment 115327 [details]
Patch to add ccreds_chkpwd to pam_ccreds (like unix_chkpwd)
This patch adds a helper program, ccreds_chkpwd, to pam_ccreds. This program
may be setuid root so that xscreensaver, which does not run as root, may
authenticate users using pam_ccreds. This is the same idea that
unix_chkpwd/pam_unix uses.
Created attachment 115328 [details]
Patch to add ccreds_chkpwd to pam_ccreds (like unix_chkpwd)
This patch adds a helper program, ccreds_chkpwd, to pam_ccreds. This program
may be setuid root so that xscreensaver, which does not run as root, may
authenticate users using pam_ccreds. This is the same idea that
unix_chkpwd/pam_unix uses.
Created attachment 115329 [details]
Patch to add ccreds_chkpwd to pam_ccreds (like unix_chkpwd)
This patch adds a helper program, ccreds_chkpwd, to pam_ccreds. This program
may be setuid root so that xscreensaver, which does not run as root, may
authenticate users using pam_ccreds. This is the same idea that
unix_chkpwd/pam_unix uses.
Created attachment 115993 [details]
Patch to add ccreds_chkpwd to pam_ccreds (like unix_chkpwd)
This version of the patch fixes a segfault that sometimes happened because of
an incorrect call to pam_cc_end().
Any comments on this patch? This is a proven solution from pam_unix. This seems like a no-brainer, once the patch has been reviewed. Sorry I dropped the ball on providing feedback. I just tested it and it works great! Would love to get it into an official pam_ccreds release (and also into RHEL 4) Kathy Whyte There is missing check for p == NULL before strlen(p) in the cc_chkpwd.c in _ccreds_verify_password(). Or probably even more correctly the _ccreds_verify_password() should not be called if no data is read from stdin? Otherwise the patch seems OK to me and I'll probably include it in next pam_ccreds rebuild. Will you send it upstream? Created attachment 119338 [details]
Patch to add ccreds_chkpwd to pam_ccreds (like unix_chkpwd)
Okay, _ccreds_verify_password not receives "" if the password is blank. This
is consistent with how pam_ccreds works before the patch.
I have tried to propose this patch using one of PADL's mailing lists. I have
not yet received a response. More recently, I tried to submit the patch using
PADL's Bugzilla. Unfortunately, pam_ccreds is not in their Bugzilla. I
submitted a request that the project be added to Bugzilla and am waiting for
that.
Have you folks gotten xscreensaver + pam_ccreds to unlock when connected to network? With this patch and system-auth similar to the one in bug 145044 (except using ldap auth, not kerberos) everything else is working nicely, but xscreensaver wont unlock when connected to network. I had to basically make a copy of the system-auth to xscreensaver pam config and change the pam_ldap auth line in the new xscreensaver pam config to this to make it work in both connected and offline mode: auth sufficient /lib/security/$ISA/pam_ldap.so use_first_pass This patch fixes the authenticate mode of the pam_ccreds module. A similar technique would need to be used to fix the other two modes. Right now, only unlocking when the network authentication daemon is NOT available is fixed. I have tried to approach PADL with this. I am waiting for them to add the pam_ccreds project to their Bugzilla so I may start communicating with them and providing patches upstream. See http://bugzilla.padl.com/show_bug.cgi? id=227. I think this is a critical problem with pam_ccreds (if it wasn't then unix_passwd would not exist.) However, I have received no interest yet in fixing it from PADL. I know what has to be done, but need feedback from the maintainer before I will proceed. In the above comment, unix_passwd is a mistake. It should be unix_chkpwd. The patch works fine. Yes, mods to /etc/pam.d/xscreensaver are required, but works as it should. Mike, We understand and respect your decision/situation. We appreciate your work to date. Kathy Whyte Ok, thanks for explaining the situation, I was just wondering if I'm missing something and indeed I was :) Thanks Mike for the work on this. Created attachment 120974 [details] Patch to add ccreds_* to pam_ccreds (like unix_chkpwd) This patch replaces ccreds_chkpwd with ccreds_validate and adds ccreds_store and ccreds_update. All three features are now supported. This patch needs testing and a review of its code. Is it safe to implement ccreds_store and ccreds_update as suid programs? Note that they have the same checks as unix_chkpwd. Could someone help me convince PADL to open up their Bugzilla to pam_ccreds? See http://bugzilla.padl.com/show_bug.cgi?id=227. I'd like to get this work upstream or at least get some feedback from the developers. I'm afraid that the store and update actions can't be implemented securely through setuid helpers. The setuid helpers would allow storing any password to the ccreds database without knowing the old password and regardless of the real password set in the remote (LDAP or KRB5) server. So the only acceptable patch allows only verification of the stored password. Created attachment 122060 [details]
Patch to add ccreds_chkpwd to pam_ccreds (like unix_chkpwd)
This patch no longer attempts to implement support for storing/updating when
EUID != 0. Instead, when the EUID != 0, store and updating operation simply
return PAM_SUCCESS. No operation is performed.
Is this safer? I don't think the store and update operations are critical,
especially when invoked by a program running with user privileges.
Yes, it should be OK and I agree that the store and update operations in this case don't have to be implemented. I've added your patch with other modifications to rawhide package. Please test it. Confirmed fixed. Could anyone at Red Hat encourage padl.org to open up a pam_ccreds project in their Bugzilla? See comment #18 and http://bugzilla.padl.com/show_bug.cgi?id=227. I'd like to get this upstream. If you are going to submit a patch, please ensure that it conforms with the existing coding style. It looks like this patch is now upstream, as of pam_ccreds-5. |