From ea086f0610030b12a04baee2b88fd81b661610b1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Apr 2021 14:53:32 +0200 Subject: [PATCH 1/3] homectl: don't use cached passwords when re-requesting password because wrong Asking repeatedly for a password is pointless if we always use the same cached one. Let's thus disable cache use whenever we failed already once. --- src/home/homectl.c | 105 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 86 insertions(+), 19 deletions(-) diff --git a/src/home/homectl.c b/src/home/homectl.c index a187a75d86..34363c4f70 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -190,7 +190,12 @@ static int list_homes(int argc, char *argv[], void *userdata) { return 0; } -static int acquire_existing_password(const char *user_name, UserRecord *hr, bool emphasize_current) { +static int acquire_existing_password( + const char *user_name, + UserRecord *hr, + bool emphasize_current, + AskPasswordFlags flags) { + _cleanup_(strv_free_erasep) char **password = NULL; _cleanup_free_ char *question = NULL; char *e; @@ -212,7 +217,7 @@ static int acquire_existing_password(const char *user_name, UserRecord *hr, bool string_erase(e); assert_se(unsetenv("PASSWORD") == 0); - return 0; + return 1; } if (asprintf(&question, emphasize_current ? @@ -221,7 +226,14 @@ static int acquire_existing_password(const char *user_name, UserRecord *hr, bool user_name) < 0) return log_oom(); - r = ask_password_auto(question, "user-home", NULL, "home-password", "home.password", USEC_INFINITY, ASK_PASSWORD_ACCEPT_CACHED|ASK_PASSWORD_PUSH_CACHE, &password); + r = ask_password_auto(question, + /* icon= */ "user-home", + NULL, + /* key_name= */ "home-password", + /* credential_name= */ "home.password", + USEC_INFINITY, + flags, + &password); if (r < 0) return log_error_errno(r, "Failed to acquire password: %m"); @@ -229,10 +241,14 @@ static int acquire_existing_password(const char *user_name, UserRecord *hr, bool if (r < 0) return log_error_errno(r, "Failed to store password: %m"); - return 0; + return 1; } -static int acquire_token_pin(const char *user_name, UserRecord *hr) { +static int acquire_token_pin( + const char *user_name, + UserRecord *hr, + AskPasswordFlags flags) { + _cleanup_(strv_free_erasep) char **pin = NULL; _cleanup_free_ char *question = NULL; char *e; @@ -250,14 +266,21 @@ static int acquire_token_pin(const char *user_name, UserRecord *hr) { string_erase(e); assert_se(unsetenv("PIN") == 0); - return 0; + return 1; } if (asprintf(&question, "Please enter security token PIN for user %s:", user_name) < 0) return log_oom(); - /* We never cache or use cached PINs, since usually there are only very few attempts allowed before the PIN is blocked */ - r = ask_password_auto(question, "user-home", NULL, "token-pin", "home.token-pin", USEC_INFINITY, 0, &pin); + r = ask_password_auto( + question, + /* icon= */ "user-home", + NULL, + /* key_name= */ "token-pin", + /* credential_name= */ "home.token-pin", + USEC_INFINITY, + flags, + &pin); if (r < 0) return log_error_errno(r, "Failed to acquire security token PIN: %m"); @@ -265,7 +288,7 @@ static int acquire_token_pin(const char *user_name, UserRecord *hr) { if (r < 0) return log_error_errno(r, "Failed to store security token PIN: %m"); - return 0; + return 1; } static int handle_generic_user_record_error( @@ -292,7 +315,13 @@ static int handle_generic_user_record_error( if (!strv_isempty(hr->password)) log_notice("Password incorrect or not sufficient, please try again."); - r = acquire_existing_password(user_name, hr, emphasize_current_password); + /* Don't consume cache entries or credentials here, we already tried that unsuccessfully. But + * let's push what we acquire here into the cache */ + r = acquire_existing_password( + user_name, + hr, + emphasize_current_password, + ASK_PASSWORD_PUSH_CACHE | ASK_PASSWORD_NO_CREDENTIAL); if (r < 0) return r; @@ -303,13 +332,21 @@ static int handle_generic_user_record_error( else log_notice("Password incorrect or not sufficient, and configured security token not inserted, please try again."); - r = acquire_existing_password(user_name, hr, emphasize_current_password); + r = acquire_existing_password( + user_name, + hr, + emphasize_current_password, + ASK_PASSWORD_PUSH_CACHE | ASK_PASSWORD_NO_CREDENTIAL); if (r < 0) return r; } else if (sd_bus_error_has_name(error, BUS_ERROR_TOKEN_PIN_NEEDED)) { - r = acquire_token_pin(user_name, hr); + /* First time the PIN is requested, let's accept cached data, and allow using credential store */ + r = acquire_token_pin( + user_name, + hr, + ASK_PASSWORD_ACCEPT_CACHED | ASK_PASSWORD_PUSH_CACHE); if (r < 0) return r; @@ -340,7 +377,11 @@ static int handle_generic_user_record_error( log_notice("Security token PIN incorrect, please try again."); - r = acquire_token_pin(user_name, hr); + /* If the previous PIN was wrong don't accept cached info anymore, but add to cache. Also, don't use the credential data */ + r = acquire_token_pin( + user_name, + hr, + ASK_PASSWORD_PUSH_CACHE | ASK_PASSWORD_NO_CREDENTIAL); if (r < 0) return r; @@ -348,7 +389,10 @@ static int handle_generic_user_record_error( log_notice("Security token PIN incorrect, please try again (only a few tries left!)."); - r = acquire_token_pin(user_name, hr); + r = acquire_token_pin( + user_name, + hr, + ASK_PASSWORD_PUSH_CACHE | ASK_PASSWORD_NO_CREDENTIAL); if (r < 0) return r; @@ -356,7 +400,10 @@ static int handle_generic_user_record_error( log_notice("Security token PIN incorrect, please try again (only one try left!)."); - r = acquire_token_pin(user_name, hr); + r = acquire_token_pin( + user_name, + hr, + ASK_PASSWORD_PUSH_CACHE | ASK_PASSWORD_NO_CREDENTIAL); if (r < 0) return r; } else @@ -399,7 +446,7 @@ static int activate_home(int argc, char *argv[], void *userdata) { r = sd_bus_call(bus, m, HOME_SLOW_BUS_CALL_TIMEOUT_USEC, &error, NULL); if (r < 0) { - r = handle_generic_user_record_error(*i, secret, &error, r, false); + r = handle_generic_user_record_error(*i, secret, &error, r, /* emphasize_current_password= */ false); if (r < 0) { if (ret == 0) ret = r; @@ -1010,7 +1057,15 @@ static int acquire_new_password( if (asprintf(&question, "Please enter new password for user %s:", user_name) < 0) return log_oom(); - r = ask_password_auto(question, "user-home", NULL, "home-password", "home.new-password", USEC_INFINITY, 0, &first); + r = ask_password_auto( + question, + /* icon= */ "user-home", + NULL, + /* key_name= */ "home-password", + /* credential_name= */ "home.new-password", + USEC_INFINITY, + 0, /* no caching, we want to collect a new password here after all */ + &first); if (r < 0) return log_error_errno(r, "Failed to acquire password: %m"); @@ -1018,7 +1073,15 @@ static int acquire_new_password( if (asprintf(&question, "Please enter new password for user %s (repeat):", user_name) < 0) return log_oom(); - r = ask_password_auto(question, "user-home", NULL, "home-password", "home.new-password", USEC_INFINITY, 0, &second); + r = ask_password_auto( + question, + /* icon= */ "user-home", + NULL, + /* key_name= */ "home-password", + /* credential_name= */ "home.new-password", + USEC_INFINITY, + 0, /* no caching */ + &second); if (r < 0) return log_error_errno(r, "Failed to acquire password: %m"); @@ -1106,7 +1169,11 @@ static int create_home(int argc, char *argv[], void *userdata) { return log_error_errno(r, "Failed to hash password: %m"); } else { /* There's a hash password set in the record, acquire the unhashed version of it. */ - r = acquire_existing_password(hr->user_name, hr, /* emphasize_current= */ false); + r = acquire_existing_password( + hr->user_name, + hr, + /* emphasize_current= */ false, + ASK_PASSWORD_ACCEPT_CACHED | ASK_PASSWORD_PUSH_CACHE); if (r < 0) return r; } From 7bdbafc261f807b68fd5622500c70bbf363ae7c0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Apr 2021 16:14:57 +0200 Subject: [PATCH 2/3] homectl: don't use password cache if we operate on other user --- src/basic/user-util.c | 13 +++++++++++++ src/basic/user-util.h | 2 ++ src/home/homectl.c | 8 ++++++++ 3 files changed, 23 insertions(+) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 0ea4e409fa..3756e80c30 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -1072,3 +1072,16 @@ int fgetsgent_sane(FILE *stream, struct sgrp **sg) { return !!s; } #endif + +int is_this_me(const char *username) { + uid_t uid; + int r; + + /* Checks if the specified username is our current one. Passed string might be a UID or a user name. */ + + r = get_user_creds(&username, &uid, NULL, NULL, NULL, USER_CREDS_ALLOW_MISSING); + if (r < 0) + return r; + + return uid == getuid(); +} diff --git a/src/basic/user-util.h b/src/basic/user-util.h index 20ff415e2e..636c392870 100644 --- a/src/basic/user-util.h +++ b/src/basic/user-util.h @@ -109,3 +109,5 @@ int putsgent_sane(const struct sgrp *sg, FILE *stream); #endif bool is_nologin_shell(const char *shell); + +int is_this_me(const char *username); diff --git a/src/home/homectl.c b/src/home/homectl.c index 34363c4f70..3e2de319bf 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -220,6 +220,10 @@ static int acquire_existing_password( return 1; } + /* If this is not our own user, then don't use the password cache */ + if (is_this_me(user_name) <= 0) + SET_FLAG(flags, ASK_PASSWORD_ACCEPT_CACHED|ASK_PASSWORD_PUSH_CACHE, false); + if (asprintf(&question, emphasize_current ? "Please enter current password for user %s:" : "Please enter password for user %s:", @@ -269,6 +273,10 @@ static int acquire_token_pin( return 1; } + /* If this is not our own user, then don't use the password cache */ + if (is_this_me(user_name) <= 0) + SET_FLAG(flags, ASK_PASSWORD_ACCEPT_CACHED|ASK_PASSWORD_PUSH_CACHE, false); + if (asprintf(&question, "Please enter security token PIN for user %s:", user_name) < 0) return log_oom(); From 57bb9bcba5563c040ee0c41f58e3730a006a8de2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Apr 2021 16:31:12 +0200 Subject: [PATCH 3/3] homectl: pick up cached/credential store/env var passwords *before* issuing first request Previously, we'd generally attempt the operation first, without any passwords, and only query for a password if that operation then fails and asks for one. This is done to improve compatibility with password-less authentication schemes, such as security tokens and similar. This patch modifies this slightly: if a password can be acquired cheaply via the keyring password cache, the $CREDENTIALS_PATH credential store, or the $PASSWORD/$PIN environment variables, acquire it *before* issuing the first requested. This should save us a pointless roundtrip, and should never hurt. --- src/home/homectl.c | 73 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 15 deletions(-) diff --git a/src/home/homectl.c b/src/home/homectl.c index 3e2de319bf..09d4247347 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -238,6 +238,11 @@ static int acquire_existing_password( USEC_INFINITY, flags, &password); + if (r == -EUNATCH) { /* EUNATCH is returned if no password was found and asking interactively was + * disabled via the flags. Not an error for us. */ + log_debug_errno(r, "No passwords acquired."); + return 0; + } if (r < 0) return log_error_errno(r, "Failed to acquire password: %m"); @@ -289,6 +294,11 @@ static int acquire_token_pin( USEC_INFINITY, flags, &pin); + if (r == -EUNATCH) { /* EUNATCH is returned if no PIN was found and asking interactively was disabled + * via the flags. Not an error for us. */ + log_debug_errno(r, "No security token PINs acquired."); + return 0; + } if (r < 0) return log_error_errno(r, "Failed to acquire security token PIN: %m"); @@ -420,6 +430,39 @@ static int handle_generic_user_record_error( return 0; } +static int acquire_passed_secrets(const char *user_name, UserRecord **ret) { + _cleanup_(user_record_unrefp) UserRecord *secret = NULL; + int r; + + assert(ret); + + /* Generates an initial secret objects that contains passwords supplied via $PASSWORD, the password + * cache or the credentials subsystem, but excluding any interactive stuff. If nothing is passed, + * returns an empty secret object. */ + + secret = user_record_new(); + if (!secret) + return log_oom(); + + r = acquire_existing_password( + user_name, + secret, + /* emphasize_current_password = */ false, + ASK_PASSWORD_ACCEPT_CACHED | ASK_PASSWORD_NO_TTY | ASK_PASSWORD_NO_AGENT); + if (r < 0) + return r; + + r = acquire_token_pin( + user_name, + secret, + ASK_PASSWORD_ACCEPT_CACHED | ASK_PASSWORD_NO_TTY | ASK_PASSWORD_NO_AGENT); + if (r < 0) + return r; + + *ret = TAKE_PTR(secret); + return 0; +} + static int activate_home(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; int r, ret = 0; @@ -432,9 +475,9 @@ static int activate_home(int argc, char *argv[], void *userdata) { STRV_FOREACH(i, strv_skip(argv, 1)) { _cleanup_(user_record_unrefp) UserRecord *secret = NULL; - secret = user_record_new(); - if (!secret) - return log_oom(); + r = acquire_passed_secrets(*i, &secret); + if (r < 0) + return r; for (;;) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; @@ -658,9 +701,9 @@ static int authenticate_home(int argc, char *argv[], void *userdata) { STRV_FOREACH(i, items) { _cleanup_(user_record_unrefp) UserRecord *secret = NULL; - secret = user_record_new(); - if (!secret) - return log_oom(); + r = acquire_passed_secrets(*i, &secret); + if (r < 0) + return r; for (;;) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; @@ -1660,9 +1703,9 @@ static int resize_home(int argc, char *argv[], void *userdata) { ds = arg_disk_size; } - secret = user_record_new(); - if (!secret) - return log_oom(); + r = acquire_passed_secrets(argv[1], &secret); + if (r < 0) + return r; for (;;) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; @@ -1736,9 +1779,9 @@ static int unlock_home(int argc, char *argv[], void *userdata) { STRV_FOREACH(i, strv_skip(argv, 1)) { _cleanup_(user_record_unrefp) UserRecord *secret = NULL; - secret = user_record_new(); - if (!secret) - return log_oom(); + r = acquire_passed_secrets(*i, &secret); + if (r < 0) + return r; for (;;) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; @@ -1802,9 +1845,9 @@ static int with_home(int argc, char *argv[], void *userdata) { if (!cmdline) return log_oom(); - secret = user_record_new(); - if (!secret) - return log_oom(); + r = acquire_passed_secrets(argv[1], &secret); + if (r < 0) + return r; for (;;) { r = bus_message_new_method_call(bus, &m, bus_mgr, "AcquireHome");