From 0ce0e63d269dba2c75e8df06e8100d63dad39472 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Thomas?= Date: Sun, 26 Feb 2017 05:04:45 +0100 Subject: [PATCH 1/4] Perform OTP validation only if token is authorized When using `try_first_pass` or `use_first_pass`, the password we inherit from PAM might not actually be an OTP challenge. Currently, we happily leak it to the validation server without first checking if it matches an authorized token ID. This postpones sending the actual request until we know the token ID is authorized. --- pam_yubico.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pam_yubico.c b/pam_yubico.c index 59b27bc..b5544f0 100644 --- a/pam_yubico.c +++ b/pam_yubico.c @@ -1058,12 +1058,6 @@ pam_sm_authenticate (pam_handle_t * pamh, else password = NULL; - rc = ykclient_request (ykc, otp); - - DBG ("ykclient return value (%d): %s", rc, - ykclient_strerror (rc)); - DBG ("ykclient url used: %s", ykclient_get_last_url(ykc)); - /* 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); @@ -1073,6 +1067,10 @@ pam_sm_authenticate (pam_handle_t * pamh, switch(valid_token) { case 1: + rc = ykclient_request (ykc, otp); + DBG ("ykclient return value (%d): %s", rc, ykclient_strerror (rc)); + DBG ("ykclient url used: %s", ykclient_get_last_url(ykc)); + switch (rc) { case YKCLIENT_OK: From 7b6aad719a099bc28448fed9c9304563d6665f87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Thomas?= Date: Sun, 26 Feb 2017 17:53:46 +0100 Subject: [PATCH 2/4] Return early if the user has no authorized tokens Currently, if a user has no associated tokens, we still prompt for an OTP challenge and attempt to verify it. This adds a check earlier to avoid the useless prompt in that case. The `nullok` option is also added. It changes the return value from PAM_USER_UNKNOWN to PAM_IGNORE. (fixes #97) Finally, some constants have been turned to symbolic form for clarity and debugging output is improved. --- README | 6 +++ pam_yubico.c | 95 ++++++++++++++++++++++++++++++++++------------- tests/util_test.c | 22 +++++------ util.c | 15 ++++---- util.h | 6 +++ 5 files changed, 100 insertions(+), 44 deletions(-) diff --git a/README b/README index 22c3564..9a0e2b0 100644 --- a/README +++ b/README @@ -171,6 +171,12 @@ stacked modules password and will never prompt the user - if no password is available or the password is not appropriate, the user will be denied access. +nullok:: +If set, don't fail when there are no tokens declared for the user +in the authorization mapping files or in LDAP. +This can be used to make YubiKey authentication optional unless +the user has associated tokens. + urllist:: List of URL templates to be used. This is set by calling ykclient_set_url_bases. The list should be in the format : diff --git a/pam_yubico.c b/pam_yubico.c index b5544f0..49f7726 100644 --- a/pam_yubico.c +++ b/pam_yubico.c @@ -108,6 +108,7 @@ struct cfg int verbose_otp; int try_first_pass; int use_first_pass; + int nullok; const char *auth_file; const char *capath; const char *cainfo; @@ -136,8 +137,9 @@ struct cfg #define DBG(x...) if (cfg->debug) { D(cfg->debug_file, x); } /* - * Authorize authenticated OTP_ID for login as USERNAME using - * AUTHFILE. Return -2 if the user is unknown, -1 if the OTP_ID does not match, 0 on internal failures, otherwise success. + * Authorize authenticated OTP_ID for login as USERNAME using AUTHFILE. + * + * Returns one of AUTH_FOUND, AUTH_NOT_FOUND, AUTH_NO_TOKENS, AUTH_ERROR. */ static int authorize_user_token (struct cfg *cfg, @@ -145,7 +147,7 @@ authorize_user_token (struct cfg *cfg, const char *otp_id, pam_handle_t *pamh) { - int retval; + int retval = AUTH_ERROR; if (cfg->auth_file) { @@ -167,7 +169,7 @@ authorize_user_token (struct cfg *cfg, pwres = getpwnam_r (username, &pass, buf, buflen, &p); if (p == NULL) { DBG ("getpwnam_r: %s", strerror(pwres)); - return 0; + return AUTH_ERROR; } /* Getting file from user home directory @@ -175,21 +177,19 @@ authorize_user_token (struct cfg *cfg, */ if (! get_user_cfgfile_path (NULL, "authorized_yubikeys", p, &userfile)) { DBG ("Failed figuring out per-user cfgfile"); - return 0; + return AUTH_ERROR; } DBG ("Dropping privileges"); if(pam_modutil_drop_priv(pamh, &privs, p)) { DBG ("could not drop privileges"); - retval = 0; - goto free_out; + goto free_out; } retval = check_user_token (userfile, username, otp_id, cfg->debug, cfg->debug_file); if(pam_modutil_regain_priv(pamh, &privs)) { - DBG (("could not restore privileges")); - retval = 0; + DBG ("could not restore privileges"); goto free_out; } @@ -202,7 +202,7 @@ free_out: /* * This function will look in ldap id the token correspond to the - * requested user. It will returns 0 for failure and 1 for success. + * requested user. * * ldaps is only supported for ldap_uri based connections. * ldap_cacertfile usually needs to be set for this to work. @@ -218,13 +218,15 @@ free_out: * If using ldap_uri, you can specify multiple failover hosts * eg. * ldap_uri=ldaps://host1.fqdn.example.com,ldaps://host2.fqdn.example.com + * + * Returns one of AUTH_FOUND, AUTH_NOT_FOUND, AUTH_NO_TOKENS, AUTH_ERROR. */ static int authorize_user_token_ldap (struct cfg *cfg, const char *user, const char *token_id) { - int retval = 0; + int retval = AUTH_ERROR; #ifdef HAVE_LIBLDAP /* LDAPv2 is historical -- RFC3494. */ int protocol = LDAP_VERSION3; @@ -261,7 +263,6 @@ authorize_user_token_ldap (struct cfg *cfg, if (rc != LDAP_SUCCESS) { DBG ("ldap_initialize: %s", ldap_err2string (rc)); - retval = 0; goto done; } } @@ -270,7 +271,6 @@ authorize_user_token_ldap (struct cfg *cfg, if ((ld = ldap_init (cfg->ldapserver, PORT_NUMBER)) == NULL) { DBG ("ldap_init"); - retval = 0; goto done; } } @@ -301,7 +301,6 @@ authorize_user_token_ldap (struct cfg *cfg, i = (strlen(cfg->user_attr) + strlen(cfg->ldapdn) + strlen(user) + 3) * sizeof(char); if ((find = malloc(i)) == NULL) { DBG ("Failed allocating %zu bytes", i); - retval = 0; goto done; } sprintf (find, "%s=%s,%s", cfg->user_attr, user, cfg->ldapdn); @@ -325,19 +324,19 @@ authorize_user_token_ldap (struct cfg *cfg, { DBG ("ldap_search_ext_s: %s", ldap_err2string (rc)); - retval = 0; goto done; } + /* Start looing for tokens */ + retval = AUTH_NO_TOKENS; + e = ldap_first_entry (ld, result); if (e == NULL) { DBG (("No result from LDAP search")); - retval = -2; } else { - retval = -1; /* Iterate through each returned attribute. */ for (a = ldap_first_attribute (ld, e, &ber); a != NULL; a = ldap_next_attribute (ld, e, ber)) @@ -357,10 +356,15 @@ 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 (vals[i]->bv_val + yubi_attr_prefix_len))) + /* We have found at least one possible token ID so change the default return value to AUTH_NOT_FOUND */ + if (retval == AUTH_NO_TOKENS) + { + retval = AUTH_NOT_FOUND; + } + if(token_id && !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; + retval = AUTH_FOUND; } } } @@ -713,6 +717,8 @@ parse_cfg (int flags, int argc, const char **argv, struct cfg *cfg) cfg->try_first_pass = 1; if (strcmp (argv[i], "use_first_pass") == 0) cfg->use_first_pass = 1; + if (strcmp (argv[i], "nullok") == 0) + cfg->nullok = 1; if (strncmp (argv[i], "authfile=", 9) == 0) cfg->auth_file = argv[i] + 9; if (strncmp (argv[i], "capath=", 7) == 0) @@ -962,6 +968,37 @@ pam_sm_authenticate (pam_handle_t * pamh, goto done; } } + /* check if the user has at least one associated token id */ + /* we set otp_id to NULL so that no matches will ever be found + * but AUTH_NO_TOKENS will be returned if there are no tokens for the user */ + if (cfg->ldapserver != NULL || cfg->ldap_uri != NULL) + valid_token = authorize_user_token_ldap (cfg, user, NULL); + else + valid_token = authorize_user_token (cfg, user, NULL, pamh); + + switch(valid_token) + { + case AUTH_ERROR: + DBG ("Internal error while looking for user tokens"); + retval = PAM_AUTHINFO_UNAVAIL; + goto done; + case AUTH_NOT_FOUND: + /* User has associated tokens, so continue */ + DBG ("Tokens found for user"); + break; + case AUTH_NO_TOKENS: + DBG ("No tokens found for user"); + if (cfg->nullok) { + retval = PAM_IGNORE; + } else { + retval = PAM_USER_UNKNOWN; + } + goto done; + default: + DBG ("Unhandled value while looking for user tokens"); + retval = PAM_AUTHINFO_UNAVAIL; + goto done; + } if (password == NULL) { @@ -1066,7 +1103,8 @@ pam_sm_authenticate (pam_handle_t * pamh, switch(valid_token) { - case 1: + case AUTH_FOUND: + DBG ("Token is associated to the user. Validating the OTP..."); rc = ykclient_request (ykc, otp); DBG ("ykclient return value (%d): %s", rc, ykclient_strerror (rc)); DBG ("ykclient url used: %s", ykclient_get_last_url(ykc)); @@ -1087,21 +1125,26 @@ pam_sm_authenticate (pam_handle_t * pamh, break; } break; - case 0: - DBG ("Internal error while validating user"); + case AUTH_ERROR: + DBG ("Internal error while looking for user tokens"); retval = PAM_AUTHINFO_UNAVAIL; break; - case -1: + case AUTH_NOT_FOUND: DBG ("Unauthorized token for this user"); retval = PAM_AUTH_ERR; break; - case -2: - DBG ("Unknown user"); - retval = PAM_USER_UNKNOWN; + case AUTH_NO_TOKENS: + DBG ("No tokens found for user"); + if (cfg->nullok) { + retval = PAM_IGNORE; + } else { + retval = PAM_USER_UNKNOWN; + } break; default: DBG ("Unhandled value for token-user validation"); retval = PAM_AUTHINFO_UNAVAIL; + break; } done: diff --git a/tests/util_test.c b/tests/util_test.c index cc68e65..0d26c9c 100644 --- a/tests/util_test.c +++ b/tests/util_test.c @@ -75,27 +75,27 @@ static void test_check_user_token(void) { fclose(handle); ret = check_user_token(file, "foobar", "hhhvhvhdhbid", 1, stdout); - assert(ret == 1); + assert(ret == AUTH_FOUND); ret = check_user_token(file, "foobar", "hnhbhnhbhnhb", 1, stdout); - assert(ret == 1); + assert(ret == AUTH_FOUND); ret = check_user_token(file, "foobar", "hnhbhnhbhnhc", 1, stdout); - assert(ret == -1); + assert(ret == AUTH_NOT_FOUND); ret = check_user_token(file, "kaka", "hihbhdhrhbhj", 1, stdout); - assert(ret == 1); + assert(ret == AUTH_FOUND); ret = check_user_token(file, "bar", "hnhbhnhbhnhb", 1, stdout); - assert(ret == 1); + assert(ret == AUTH_FOUND); ret = check_user_token(file, "foo", "hdhrhbhjhvhu", 1, stdout); - assert(ret == -2); + assert(ret == AUTH_NO_TOKENS); ret = check_user_token(file, "foo2", "cccccccccccc", 1, stdout); - assert(ret == 1); + assert(ret == AUTH_FOUND); ret = check_user_token(file, "foo2", "vvvvvvvvvvvv", 1, stdout); - assert(ret == 1); + assert(ret == AUTH_FOUND); ret = check_user_token(file, "foo2", "vvvvvvvvvvcc", 1, stdout); - assert(ret == -1); + assert(ret == AUTH_NOT_FOUND); ret = check_user_token(file, "foo2", "", 1, stdout); - assert(ret == -1); + assert(ret == AUTH_NOT_FOUND); ret = check_user_token(file, "foo", "", 1, stdout); - assert(ret == -2); + assert(ret == AUTH_NO_TOKENS); remove(file); } diff --git a/util.c b/util.c index b04ecaf..57d2e9b 100644 --- a/util.c +++ b/util.c @@ -85,8 +85,9 @@ get_user_cfgfile_path(const char *common_path, const char *filename, const struc /* - * This function will look for users name with valid user token id. It - * will returns -2 if the user is unknown, -1 if the token do not match the user line, 0 for internal failure and 1 for success. + * This function will look for users name with valid user token id. + * + * Returns one of AUTH_FOUND, AUTH_NOT_FOUND, AUTH_NO_TOKENS, AUTH_ERROR. * * File format is as follows: * :: @@ -102,7 +103,7 @@ check_user_token (const char *authfile, { char buf[1024]; char *s_user, *s_token; - int retval = 0; + int retval = AUTH_ERROR; int fd; struct stat st; FILE *opwfile; @@ -136,7 +137,7 @@ check_user_token (const char *authfile, return retval; } - retval = -2; + retval = AUTH_NO_TOKENS; while (fgets (buf, 1024, opwfile)) { char *saveptr = NULL; @@ -155,17 +156,17 @@ check_user_token (const char *authfile, { if(verbose) D (debug_file, "Matched user: %s", s_user); - retval = -1; //We found at least one line for the user + retval = AUTH_NOT_FOUND; /* We found at least one line for the user */ do { s_token = strtok_r (NULL, ":", &saveptr); if(verbose) D (debug_file, "Authorization token: %s", s_token); - if (s_token && strcmp (otp_id, s_token) == 0) + if (s_token && otp_id && strcmp (otp_id, s_token) == 0) { if(verbose) D (debug_file, "Match user/token as %s/%s", username, otp_id); - return 1; + return AUTH_FOUND; } } while (s_token != NULL); diff --git a/util.h b/util.h index 69c69c0..37421c9 100644 --- a/util.h +++ b/util.h @@ -44,6 +44,12 @@ fprintf (file, "\n"); \ } while (0) +/* Return values for authorize_user_token and authorize_user_token_ldap */ +#define AUTH_NO_TOKENS -2 /* The user has no associated tokens */ +#define AUTH_ERROR 0 /* Internal error when looking up associated tokens */ +#define AUTH_FOUND 1 /* The requested token is associated to the user */ +#define AUTH_NOT_FOUND -1 /* The requested token is not associated to the user */ + int get_user_cfgfile_path(const char *common_path, const char *filename, const struct passwd *user, char **fn); int check_user_token(const char *authfile, const char *username, const char *otp_id, int verbose, FILE *debug_file); From fc3b1e0076eb230582da854578fb5c9a54891902 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Thomas?= Date: Sun, 26 Feb 2017 20:30:01 +0100 Subject: [PATCH 3/4] Compare OTP IDs against `yubi_attr` only Currently we trust the LDAP server to only return the `yubi_attr` attribute, yet we loop over all possible attributes when there should only be one. Since the bundled test LDAP server ignores the requested attributes list, we must make sure to only match against the `yubi_attr` attibute as opposed to "all of them". This also fixes an issue where AUTH_NOT_FOUND was returned instead of AUTH_NO_TOKENS when there were no values returned for `yubi_attr` but another attribute's value was considered as a candidate token. --- pam_yubico.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/pam_yubico.c b/pam_yubico.c index 49f7726..5a50250 100644 --- a/pam_yubico.c +++ b/pam_yubico.c @@ -234,7 +234,7 @@ authorize_user_token_ldap (struct cfg *cfg, LDAP *ld = NULL; LDAPMessage *result = NULL, *e; BerElement *ber; - char *a; + char *attr_name; char *attrs[2] = {NULL, NULL}; struct berval **vals; @@ -338,20 +338,30 @@ authorize_user_token_ldap (struct cfg *cfg, else { /* Iterate through each returned attribute. */ - for (a = ldap_first_attribute (ld, e, &ber); - a != NULL; a = ldap_next_attribute (ld, e, ber)) + for (attr_name = ldap_first_attribute (ld, e, &ber); + attr_name != NULL; attr_name = ldap_next_attribute (ld, e, ber)) { - if ((vals = ldap_get_values_len (ld, e, a)) != NULL) + if (strcmp(attr_name, cfg->yubi_attr) != 0) { + DBG("Ignored non-requested attribute: %s", attr_name); + continue; + } + if ((vals = ldap_get_values_len (ld, e, attr_name)) != NULL) { yubi_attr_prefix_len = cfg->yubi_attr_prefix ? strlen(cfg->yubi_attr_prefix) : 0; + DBG("LDAP : Found %i values for %s - checking if any of them match '%s:%s'", + ldap_count_values_len(vals), + attr_name, + cfg->yubi_attr_prefix ? cfg->yubi_attr_prefix : "", + token_id ? token_id : "(null)"); + /* Compare each value for the attribute against the token id. */ for (i = 0; vals[i] != NULL; i++) { - DBG("LDAP : Found %i values - checking if any of them match '%s:%s:%s'", - ldap_count_values_len(vals), - vals[i]->bv_val, - cfg->yubi_attr_prefix ? cfg->yubi_attr_prefix : "", token_id); + DBG("LDAP : Checking value %i: %s:%s", + i + 1, + cfg->yubi_attr_prefix ? cfg->yubi_attr_prefix : "", + vals[i]->bv_val); /* 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))) @@ -370,7 +380,7 @@ authorize_user_token_ldap (struct cfg *cfg, } ldap_value_free_len (vals); } - ldap_memfree (a); + ldap_memfree (attr_name); } if (ber != NULL) ber_free (ber, 0); From d048a4a6e2fbe55982c5fc819ab2016b89ca136b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Thomas?= Date: Sun, 26 Feb 2017 20:38:33 +0100 Subject: [PATCH 4/4] Add test for LDAP entries with empty token list --- tests/aux/ldap.pl | 1 + tests/pam_test.c | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/tests/aux/ldap.pl b/tests/aux/ldap.pl index 55be4d6..6007373 100755 --- a/tests/aux/ldap.pl +++ b/tests/aux/ldap.pl @@ -45,6 +45,7 @@ use constant RESULT_OK => { my %objects = ( 'base=uid=foo,ou=users,dc=example,dc=com' => {keys => ['vvincredible']}, 'base=uid=test,ou=users,dc=example,dc=com' => {keys => ['cccccccfhcbe', 'ccccccbchvth']}, + 'base=uid=nokeys,ou=users,dc=example,dc=com' => {keys => []}, 'sub:base=:(uid=test)' => {keys => ['cccccccfhcbe', 'ccccccbchvth'], dn => 'uid=test,out=users,dc=example,dc=com'}, ); diff --git a/tests/pam_test.c b/tests/pam_test.c index 6be0d1f..25c2b2e 100644 --- a/tests/pam_test.c +++ b/tests/pam_test.c @@ -66,6 +66,7 @@ static struct data { {"test", "ccccccbchvthlivuitriujjifivbvtrjkjfirllluurj"}, {"foo", ""}, {"bar", ""}, + {"nokeys", ""}, }; @@ -274,6 +275,10 @@ static int test_authenticate_ldap5(void) { return pam_sm_authenticate(6, 0, sizeof(ldap_cfg) / sizeof(char*), ldap_cfg); } +static int test_authenticate_ldap6(void) { + return pam_sm_authenticate(7, 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) { @@ -354,6 +359,10 @@ int main(void) { ret = 1007; goto out; } + if(test_authenticate_ldap6() != PAM_USER_UNKNOWN) { + ret = 1008; + goto out; + } #endif out: