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

Lukas Slebodnik lslebodn at redhat.com
Thu Oct 23 09:28:44 UTC 2014


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.

LS
-------------- next part --------------
>From 5b659122d61f553a1cb09bf7f5beaf1379689fc2 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Mon, 20 Oct 2014 22:21:25 +0200
Subject: [PATCH] PAM: Remove authtok from PAM stack with OTP

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
---
 src/providers/krb5/krb5_auth.c | 14 ++++++++++++++
 src/sss_client/pam_sss.c       | 16 +++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index f539d5068ec29f7b06f734a3417864b43122b1b7..c96b7aee99da8c3d43a67a04bb1f67ee048d4705 100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -1161,6 +1161,20 @@ static void krb5_auth_done(struct tevent_req *subreq)
         krb5_auth_store_creds(state->domain, pd);
     }
 
+    if (res->otp == true && pd->cmd == SSS_PAM_AUTHENTICATE) {
+        uint32_t otp_flag = 1;
+        ret = pam_add_response(pd, SSS_OTP, sizeof(uint32_t),
+                               (const uint8_t *) &otp_flag);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  "pam_add_response failed: %d (%s).\n",
+                  ret, sss_strerror(ret));
+            state->pam_status = PAM_SYSTEM_ERR;
+            state->dp_err = DP_ERR_OK;
+            goto done;
+        }
+    }
+
     state->pam_status = PAM_SUCCESS;
     state->dp_err = DP_ERR_OK;
     ret = EOK;
diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
index abe9b05478cbf480b3430dccd1951e9bfb0e29c1..d64e826daeb80be8998ef3b410047e3a44051b07 100644
--- a/src/sss_client/pam_sss.c
+++ b/src/sss_client/pam_sss.c
@@ -206,7 +206,7 @@ static size_t add_string_item(enum pam_item_type type, const char *str,
     return rp;
 }
 
-static void overwrite_and_free_pam_items(struct pam_items *pi)
+static void overwrite_and_free_authtoks(struct pam_items *pi)
 {
     if (pi->pam_authtok != NULL) {
         _pam_overwrite_n((void *)pi->pam_authtok, pi->pam_authtok_size);
@@ -222,6 +222,11 @@ static void overwrite_and_free_pam_items(struct pam_items *pi)
 
     pi->pamstack_authtok = NULL;
     pi->pamstack_oldauthtok = NULL;
+}
+
+static void overwrite_and_free_pam_items(struct pam_items *pi)
+{
+    overwrite_and_free_authtoks(pi);
 
     free(pi->domain_name);
     pi->domain_name = NULL;
@@ -998,6 +1003,15 @@ static int eval_response(pam_handle_t *pamh, size_t buflen, uint8_t *buf,
                     D(("do_pam_conversation failed."));
                 }
                 break;
+            case SSS_OTP:
+                D(("OTP was used, removing authtokens."));
+                overwrite_and_free_authtoks(pi);
+                ret = pam_set_item(pamh, PAM_AUTHTOK, NULL);
+                if (ret != PAM_SUCCESS) {
+                    D(("Failed to remove PAM_AUTHTOK after using otp [%s]",
+                       pam_strerror(pamh,ret)));
+                }
+                break;
             default:
                 D(("Unknown response type [%d]", type));
         }
-- 
2.1.0



More information about the sssd-devel mailing list