From a21a20cb6569fa31616d7f4f1efa585cce5f54d4 Mon Sep 17 00:00:00 2001 From: Klas Lindfors Date: Mon, 13 Jun 2016 09:04:22 +0200 Subject: [PATCH 1/3] only process results of OTP check after user is found relates #97 --- pam_yubico.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/pam_yubico.c b/pam_yubico.c index fa9f943..2688e59 100644 --- a/pam_yubico.c +++ b/pam_yubico.c @@ -1040,21 +1040,6 @@ pam_sm_authenticate (pam_handle_t * pamh, ykclient_strerror (rc))); DBG (("ykclient url used: %s", ykclient_get_last_url(ykc))); - switch (rc) - { - case YKCLIENT_OK: - break; - - case YKCLIENT_BAD_OTP: - case YKCLIENT_REPLAYED_OTP: - retval = PAM_AUTH_ERR; - goto done; - - default: - retval = PAM_AUTHINFO_UNAVAIL; - goto done; - } - /* authorize the user with supplied token id */ if (cfg->ldapserver != NULL || cfg->ldap_uri != NULL) valid_token = authorize_user_token_ldap (cfg, user, otp_id); @@ -1064,7 +1049,21 @@ pam_sm_authenticate (pam_handle_t * pamh, switch(valid_token) { case 1: - retval = PAM_SUCCESS; + switch (rc) + { + case YKCLIENT_OK: + retval = PAM_SUCCESS; + break; + + case YKCLIENT_BAD_OTP: + case YKCLIENT_REPLAYED_OTP: + retval = PAM_AUTH_ERR; + break; + + default: + retval = PAM_AUTHINFO_UNAVAIL; + break; + } break; case 0: DBG (("Internal error while validating user")); From fee0bcc231154876b4312d7a2107f7f3963c0bc6 Mon Sep 17 00:00:00 2001 From: Klas Lindfors Date: Mon, 13 Jun 2016 10:12:55 +0200 Subject: [PATCH 2/3] drop check for OTP length, should trigger error later anyways. relates #97 --- pam_yubico.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/pam_yubico.c b/pam_yubico.c index 2688e59..2e119cc 100644 --- a/pam_yubico.c +++ b/pam_yubico.c @@ -988,16 +988,13 @@ pam_sm_authenticate (pam_handle_t * pamh, } password_len = strlen (password); - if (password_len < (cfg->token_id_length + TOKEN_OTP_LEN)) - { - DBG (("OTP too short to be considered : %zu < %u", password_len, (cfg->token_id_length + TOKEN_OTP_LEN))); - retval = PAM_AUTH_ERR; - goto done; - } /* In case the input was systempassword+YubiKeyOTP, we want to skip over "systempassword" when copying the token_id and OTP to separate buffers */ - skip_bytes = password_len - (cfg->token_id_length + TOKEN_OTP_LEN); + if(password_len > cfg->token_id_length + TOKEN_OTP_LEN) + { + skip_bytes = password_len - (cfg->token_id_length + TOKEN_OTP_LEN); + } DBG (("Skipping first %i bytes. Length is %zu, token_id set to %u and token OTP always %u.", skip_bytes, password_len, cfg->token_id_length, TOKEN_OTP_LEN)); From 4fb0be3870406d70a93452e0aa6d51bc5bd128b2 Mon Sep 17 00:00:00 2001 From: Klas Lindfors Date: Mon, 13 Jun 2016 11:08:09 +0200 Subject: [PATCH 3/3] add tests for empty OTP validation also fix around so ldap case checks with length of the authorized token, not the length of the passed in id. --- pam_yubico.c | 2 +- tests/pam_test.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/pam_yubico.c b/pam_yubico.c index 2e119cc..733c7cf 100644 --- a/pam_yubico.c +++ b/pam_yubico.c @@ -357,7 +357,7 @@ authorize_user_token_ldap (struct cfg *cfg, /* Only values containing this prefix are considered. */ if ((!cfg->yubi_attr_prefix || !strncmp (cfg->yubi_attr_prefix, vals[i]->bv_val, yubi_attr_prefix_len))) { - if(!strncmp (token_id, vals[i]->bv_val + yubi_attr_prefix_len, strlen (token_id))) + if(!strncmp (token_id, vals[i]->bv_val + yubi_attr_prefix_len, strlen (vals[i]->bv_val + yubi_attr_prefix_len))) { DBG (("Token Found :: %s", vals[i]->bv_val)); retval = 1; diff --git a/tests/pam_test.c b/tests/pam_test.c index 7e5459d..6be0d1f 100644 --- a/tests/pam_test.c +++ b/tests/pam_test.c @@ -64,6 +64,8 @@ static struct data { {"foo", "vvincrediblltrerdegkkrkkneieultcjdghrejjbckh"}, {"foo", "vvincredibletrerdegkkrkkneieultcjdghrejjbckl"}, {"test", "ccccccbchvthlivuitriujjifivbvtrjkjfirllluurj"}, + {"foo", ""}, + {"bar", ""}, }; @@ -194,6 +196,26 @@ static int test_authenticate3(void) { return pam_sm_authenticate(4, 0, sizeof(cfg) / sizeof(char*), cfg); } +static int test_authenticate4(void) { + const char *cfg[] = { + "id=1", + "urllist=http://localhost:"YKVAL_PORT1"/wsapi/2/verify;http://localhost:"YKVAL_PORT2"/wsapi/2/verify", + "authfile="AUTHFILE, + "debug", + }; + return pam_sm_authenticate(5, 0, sizeof(cfg) / sizeof(char*), cfg); +} + +static int test_authenticate5(void) { + const char *cfg[] = { + "id=1", + "urllist=http://localhost:"YKVAL_PORT1"/wsapi/2/verify;http://localhost:"YKVAL_PORT2"/wsapi/2/verify", + "authfile="AUTHFILE, + "debug", + }; + return pam_sm_authenticate(6, 0, sizeof(cfg) / sizeof(char*), cfg); +} + static int test_fail_authenticate1(void) { const char *cfg[] = { "id=1", @@ -244,6 +266,14 @@ static int test_authenticate_ldap3(void) { return pam_sm_authenticate(4, 0, sizeof(ldap_cfg2) / sizeof(char*), ldap_cfg2); } +static int test_authenticate_ldap4(void) { + return pam_sm_authenticate(5, 0, sizeof(ldap_cfg) / sizeof(char*), ldap_cfg); +} + +static int test_authenticate_ldap5(void) { + return pam_sm_authenticate(6, 0, sizeof(ldap_cfg) / sizeof(char*), ldap_cfg); +} + static pid_t run_mock(const char *port, const char *type) { pid_t pid = fork(); if(pid == 0) { @@ -287,6 +317,14 @@ int main(void) { ret = 6; goto out; } + if(test_authenticate4() != PAM_AUTH_ERR) { + ret = 7; + goto out; + } + if(test_authenticate5() != PAM_USER_UNKNOWN) { + ret = 8; + goto out; + } #ifdef HAVE_LIBLDAP if(test_authenticate_ldap1() != PAM_SUCCESS) { ret = 1001; @@ -308,6 +346,14 @@ int main(void) { ret = 1005; goto out; } + if(test_authenticate_ldap4() != PAM_AUTH_ERR) { + ret = 1006; + goto out; + } + if(test_authenticate_ldap5() != PAM_USER_UNKNOWN) { + ret = 1007; + goto out; + } #endif out: