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

Nathaniel McCallum npmccallum at redhat.com
Wed Oct 22 18:55:09 UTC 2014


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 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.

Nathaniel




More information about the sssd-devel mailing list