[SSSD] [PATCH] PAM: Remove authtok from PAM stack with OTP

Jakub Hrozek jhrozek at redhat.com
Fri Nov 7 14:15:53 UTC 2014


On Thu, Nov 06, 2014 at 04:37:31PM -0500, Nathaniel McCallum wrote:
> On Thu, 2014-10-23 at 11:28 +0200, Lukas Slebodnik wrote:
> > On (22/10/14 14:55), Nathaniel McCallum wrote:
> > >On Wed, 2014-10-22 at 17:43 +0200, Lukas Slebodnik wrote:
> > >> On (22/10/14 10:58), Nathaniel McCallum wrote:
> > >> >On Tue, 2014-10-21 at 20:34 +0200, Lukas Slebodnik wrote:
> > >> >> On (21/10/14 20:06), Jakub Hrozek wrote:
> > >> >> >On Tue, Oct 21, 2014 at 01:29:53PM -0400, Nathaniel McCallum wrote:
> > >> >> >> On Tue, 2014-10-21 at 00:44 +0200, Lukas Slebodnik wrote:
> > >> >> >> > ehlo,
> > >> >> >> > 
> > >> >> >> > We remove the password from the PAM stack when OTP is used to make sure
> > >> >> >> > that other pam modules (pam-gnome-keyring, pam_mount) cannot use it anymore
> > >> >> >> > and have to request a password on their own.
> > >> >> >> > 
> > >> >> >> > Resolves: https://fedorahosted.org/sssd/ticket/2287
> > >> >> >> > 
> > >> >> >> > Simple patch is attached.
> > >> >> >> 
> > >> >> >> I may be wrong, but I think that making the pam_add_response() and
> > >> >> >> pam_set_item() errors non-fatal is incorrect. Attempting to use the OTP
> > >> >> >> credentials again could result in further errors, keyring problems or
> > >> >> >> account locking. It seems to me that it would better to fail the
> > >> >> >> authentication if you cannot guarantee that OTP credentials will not be
> > >> >> >> reused.
> > >> >> >
> > >> >> >On the other hand, logging in as the user in question (and then letting
> > >> >> >him to sudo) might be the only way of getting access into the system at
> > >> >> >all..
> > >> >> Should I change it or no?
> > >> >> 
> > >> >> It would be very simple change :-)
> > >> >
> > >> >I'm not sure I understand Jakub's objection. Could someone clarify?
> > >> >
> > >> >As I understand it, a failure in these functions is largely restricted
> > >> >to thinks like OOM. In such a case, I wonder if login will be possible
> > >> >at all.
> > >> >
> > >> Sorry, I don't understand you.
> > >> Do you mean client part(src/sss_client/pam_sss.c) or responder part?
> > >> 
> > >> Because on client part, OOM is not threated as failure.
> > >>  976                     env_item = strdup((char *)&buf[p]);
> > >>  977                     if (env_item == NULL) {
> > >>  978                         D(("strdup failed"));
> > >>  979                         break;
> > >>  980                     }
> > >>  981                     ret = putenv(env_item);
> > >>  982                     if (ret == -1) {
> > >>  983                         D(("putenv failed."));
> > >>  984                         break;
> > >>  985                     }
> > >> 
> > >> //break will cause jump out of switch and if there are no more data in buffer
> > >> then PAM_SUCCESS will be returned from function eval_response
> > >
> > >Looking at the code again, I think the problem only exists in the
> > >handling of the return value from pam_add_response(). See your
> > >comment: /* Not fatal */
> > >
> > I'm fine with changing code in back end.
> > I was against a change in client code.
> > 
> > >I believe this block should be fatal. The function pam_add_response()
> > >only fails in the case of OOM (see src/providers/dp_pam_data_util.c). It
> > >is extremely likely that an OOM here, if ignored, will appear somewhere
> > >later in the chain. Since it is safe to fail the authentication here, we
> > >should do so.
> > >
> > >This is especially true because leaving the credentials on the pam stack
> > >may cause server-side issues like account locking. This is more
> > >difficult to recover from than an OOM-caused authentication failure.
> > >
> > >In short, if authentication succeeds but we cannot remove the
> > >credentials from the pam stack due to OOM (extremely unlikely), we
> > >should fail the authentication.
> > >
> > Updated version is attached.
> 
> I just finished testing this. Works for me. ACK
> 
> Nathaniel

Thank you for the review!

master: 2368a0fc19bcd56581eccd8397289e4513a383a5 



More information about the sssd-devel mailing list