From 75e45d84d9391e29d49f6ec05272c3fb9a92bbd8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 19 Feb 2022 00:08:39 +0100 Subject: [PATCH] env-util: replace unsetenv_erase() by new getenv_steal_erase() helper The new helper combines a bunch of steps every invocation of unsetenv_erase() did so far: getenv() + strdup() + unsetenv_erase(). Let's unify this into one helper that is harder to use incorrectly. It's in inspired by TAKE_PTR() in a way: get the env var out and invalidate where it was before. (cherry picked from commit e99ca1474145f7fad38bb0255d344f4ad7717ef5) Related: #2087652 --- src/basic/env-util.c | 27 ++++++++++--- src/basic/env-util.h | 2 + src/cryptenroll/cryptenroll-password.c | 15 ++----- src/cryptenroll/cryptenroll.c | 20 ++++------ src/cryptsetup/cryptsetup-fido2.c | 12 +++--- src/cryptsetup/cryptsetup.c | 15 ++++--- src/home/homectl.c | 55 ++++++++++++-------------- src/shared/pkcs11-util.c | 11 +++--- src/test/test-env-util.c | 16 +++++--- 9 files changed, 89 insertions(+), 84 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 455f5d76f5..b60c9f9fdc 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -857,19 +857,36 @@ int getenv_path_list(const char *name, char ***ret_paths) { return 1; } -int unsetenv_erase(const char *name) { - char *p; +int getenv_steal_erase(const char *name, char **ret) { + _cleanup_(erase_and_freep) char *a = NULL; + char *e; assert(name); - p = getenv(name); - if (!p) + /* Reads an environment variable, makes a copy of it, erases its memory in the environment block and removes + * it from there. Usecase: reading passwords from the env block (which is a bad idea, but useful for + * testing, and given that people are likely going to misuse this, be thorough) */ + + e = getenv(name); + if (!e) { + if (ret) + *ret = NULL; return 0; + } - string_erase(p); + if (ret) { + a = strdup(e); + if (!a) + return -ENOMEM; + } + + string_erase(e); if (unsetenv(name) < 0) return -errno; + if (ret) + *ret = TAKE_PTR(a); + return 1; } diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 38bfc8a3f2..e4084af9a5 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -70,3 +70,5 @@ int setenv_systemd_exec_pid(bool update_only); int getenv_path_list(const char *name, char ***ret_paths); int unsetenv_erase(const char *name); + +int getenv_steal_erase(const char *name, char **ret); diff --git a/src/cryptenroll/cryptenroll-password.c b/src/cryptenroll/cryptenroll-password.c index 1775912d8e..9b7c8b5400 100644 --- a/src/cryptenroll/cryptenroll-password.c +++ b/src/cryptenroll/cryptenroll-password.c @@ -17,20 +17,13 @@ int enroll_password( _cleanup_free_ char *error = NULL; const char *node; int r, keyslot; - char *e; assert_se(node = crypt_get_device_name(cd)); - e = getenv("NEWPASSWORD"); - if (e) { - - new_password = strdup(e); - if (!new_password) - return log_oom(); - - assert_se(unsetenv_erase("NEWPASSWORD") >= 0); - - } else { + r = getenv_steal_erase("NEWPASSWORD", &new_password); + if (r < 0) + return log_error_errno(r, "Failed to acquire password from environment: %m"); + if (r == 0) { _cleanup_free_ char *disk_path = NULL; unsigned i = 5; const char *id; diff --git a/src/cryptenroll/cryptenroll.c b/src/cryptenroll/cryptenroll.c index ed19f3f8f4..2e11ffe291 100644 --- a/src/cryptenroll/cryptenroll.c +++ b/src/cryptenroll/cryptenroll.c @@ -422,8 +422,8 @@ static int prepare_luks( size_t *ret_volume_key_size) { _cleanup_(crypt_freep) struct crypt_device *cd = NULL; + _cleanup_(erase_and_freep) char *envpw = NULL; _cleanup_(erase_and_freep) void *vk = NULL; - char *e = NULL; size_t vks; int r; @@ -458,23 +458,17 @@ static int prepare_luks( if (!vk) return log_oom(); - e = getenv("PASSWORD"); - if (e) { - _cleanup_(erase_and_freep) char *password = NULL; - - password = strdup(e); - if (!password) - return log_oom(); - - assert_se(unsetenv_erase("PASSWORD") >= 0); - + r = getenv_steal_erase("PASSWORD", &envpw); + if (r < 0) + return log_error_errno(r, "Failed to acquire password from environment: %m"); + if (r > 0) { r = crypt_volume_key_get( cd, CRYPT_ANY_SLOT, vk, &vks, - password, - strlen(password)); + envpw, + strlen(envpw)); if (r < 0) return log_error_errno(r, "Password from environment variable $PASSWORD did not work."); } else { diff --git a/src/cryptsetup/cryptsetup-fido2.c b/src/cryptsetup/cryptsetup-fido2.c index 35d5dbe007..74053b8ce3 100644 --- a/src/cryptsetup/cryptsetup-fido2.c +++ b/src/cryptsetup/cryptsetup-fido2.c @@ -30,12 +30,12 @@ int acquire_fido2_key( size_t *ret_decrypted_key_size, AskPasswordFlags ask_password_flags) { + _cleanup_(erase_and_freep) char *envpw = NULL; _cleanup_strv_free_erase_ char **pins = NULL; _cleanup_free_ void *loaded_salt = NULL; bool device_exists = false; const char *salt; size_t salt_size; - char *e; int r; ask_password_flags |= ASK_PASSWORD_PUSH_CACHE | ASK_PASSWORD_ACCEPT_CACHED; @@ -66,13 +66,13 @@ int acquire_fido2_key( salt = loaded_salt; } - e = getenv("PIN"); - if (e) { - pins = strv_new(e); + r = getenv_steal_erase("PIN", &envpw); + if (r < 0) + return log_error_errno(r, "Failed to acquire password from environment: %m"); + if (r > 0) { + pins = strv_new(envpw); if (!pins) return log_oom(); - - assert_se(unsetenv_erase("PIN") >= 0); } for (;;) { diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index fc1f37730f..692e1d137b 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -829,20 +829,19 @@ static bool libcryptsetup_plugins_support(void) { #if HAVE_LIBCRYPTSETUP_PLUGINS static int acquire_pins_from_env_variable(char ***ret_pins) { - char *e; + _cleanup_(erase_and_freep) char *envpin = NULL; _cleanup_strv_free_erase_ char **pins = NULL; + int r; assert(ret_pins); - e = getenv("PIN"); - if (e) { - pins = strv_new(e); + r = getenv_steal_erase("PIN", &envpin); + if (r < 0) + return log_error_errno(r, "Failed to acquire PIN from environment: %m"); + if (r > 0) { + pins = strv_new(envpin); if (!pins) return log_oom(); - - string_erase(e); - if (unsetenv("PIN") < 0) - return log_error_errno(errno, "Failed to unset $PIN: %m"); } *ret_pins = TAKE_PTR(pins); diff --git a/src/home/homectl.c b/src/home/homectl.c index 4aeaaf6b9a..f0d1dac6ab 100644 --- a/src/home/homectl.c +++ b/src/home/homectl.c @@ -201,24 +201,25 @@ static int acquire_existing_password( AskPasswordFlags flags) { _cleanup_(strv_free_erasep) char **password = NULL; + _cleanup_(erase_and_freep) char *envpw = NULL; _cleanup_free_ char *question = NULL; - char *e; int r; assert(user_name); assert(hr); - e = getenv("PASSWORD"); - if (e) { + r = getenv_steal_erase("PASSWORD", &envpw); + if (r < 0) + return log_error_errno(r, "Failed to acquire password from environment: %m"); + if (r > 0) { /* People really shouldn't use environment variables for passing passwords. We support this * only for testing purposes, and do not document the behaviour, so that people won't * actually use this outside of testing. */ - r = user_record_set_password(hr, STRV_MAKE(e), true); + r = user_record_set_password(hr, STRV_MAKE(envpw), true); if (r < 0) return log_error_errno(r, "Failed to store password: %m"); - assert_se(unsetenv_erase("PASSWORD") >= 0); return 1; } @@ -261,24 +262,25 @@ static int acquire_recovery_key( AskPasswordFlags flags) { _cleanup_(strv_free_erasep) char **recovery_key = NULL; + _cleanup_(erase_and_freep) char *envpw = NULL; _cleanup_free_ char *question = NULL; - char *e; int r; assert(user_name); assert(hr); - e = getenv("RECOVERY_KEY"); - if (e) { + r = getenv_steal_erase("PASSWORD", &envpw); + if (r < 0) + return log_error_errno(r, "Failed to acquire password from environment: %m"); + if (r > 0) { /* People really shouldn't use environment variables for passing secrets. We support this * only for testing purposes, and do not document the behaviour, so that people won't * actually use this outside of testing. */ - r = user_record_set_password(hr, STRV_MAKE(e), true); /* recovery keys are stored in the record exactly like regular passwords! */ + r = user_record_set_password(hr, STRV_MAKE(envpw), true); /* recovery keys are stored in the record exactly like regular passwords! */ if (r < 0) return log_error_errno(r, "Failed to store recovery key: %m"); - assert_se(unsetenv_erase("RECOVERY_KEY") >= 0); return 1; } @@ -318,20 +320,21 @@ static int acquire_token_pin( AskPasswordFlags flags) { _cleanup_(strv_free_erasep) char **pin = NULL; + _cleanup_(erase_and_freep) char *envpin = NULL; _cleanup_free_ char *question = NULL; - char *e; int r; assert(user_name); assert(hr); - e = getenv("PIN"); - if (e) { - r = user_record_set_token_pin(hr, STRV_MAKE(e), false); + r = getenv_steal_erase("PIN", &envpin); + if (r < 0) + return log_error_errno(r, "Failed to acquire PIN from environment: %m"); + if (r > 0) { + r = user_record_set_token_pin(hr, STRV_MAKE(envpin), false); if (r < 0) return log_error_errno(r, "Failed to store token PIN: %m"); - assert_se(unsetenv_erase("PIN") >= 0); return 1; } @@ -1147,33 +1150,25 @@ static int acquire_new_password( bool suggest, char **ret) { + _cleanup_(erase_and_freep) char *envpw = NULL; unsigned i = 5; - char *e; int r; assert(user_name); assert(hr); - e = getenv("NEWPASSWORD"); - if (e) { - _cleanup_(erase_and_freep) char *copy = NULL; - + r = getenv_steal_erase("NEWPASSWORD", &envpw); + if (r < 0) + return log_error_errno(r, "Failed to acquire password from environment: %m"); + if (r > 0) { /* As above, this is not for use, just for testing */ - if (ret) { - copy = strdup(e); - if (!copy) - return log_oom(); - } - - r = user_record_set_password(hr, STRV_MAKE(e), /* prepend = */ true); + r = user_record_set_password(hr, STRV_MAKE(envpw), /* prepend = */ true); if (r < 0) return log_error_errno(r, "Failed to store password: %m"); - assert_se(unsetenv_erase("NEWPASSWORD") >= 0); - if (ret) - *ret = TAKE_PTR(copy); + *ret = TAKE_PTR(envpw); return 0; } diff --git a/src/shared/pkcs11-util.c b/src/shared/pkcs11-util.c index 4f9ec1fbd6..80feeb1fae 100644 --- a/src/shared/pkcs11-util.c +++ b/src/shared/pkcs11-util.c @@ -275,15 +275,16 @@ int pkcs11_token_login( for (unsigned tries = 0; tries < 3; tries++) { _cleanup_strv_free_erase_ char **passwords = NULL; - char *e; + _cleanup_(erase_and_freep) char *envpin = NULL; - e = getenv("PIN"); - if (e) { - passwords = strv_new(e); + r = getenv_steal_erase("PIN", &envpin); + if (r < 0) + return log_error_errno(r, "Failed to acquire PIN from environment: %m"); + if (r > 0) { + passwords = strv_new(envpin); if (!passwords) return log_oom(); - assert_se(unsetenv_erase("PIN") >= 0); } else if (headless) return log_error_errno(SYNTHETIC_ERRNO(ENOPKG), "PIN querying disabled via 'headless' option. Use the 'PIN' environment variable."); else { diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index 4d5f39b5b7..cc37d96327 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -406,16 +406,16 @@ TEST(setenv_systemd_exec_pid) { assert_se(set_unset_env("SYSTEMD_EXEC_PID", saved, 1) >= 0); } -TEST(unsetenv_erase) { +TEST(getenv_steal_erase) { int r; - r = safe_fork("(sd-unsetenverase)", FORK_DEATHSIG|FORK_LOG|FORK_WAIT, NULL); + r = safe_fork("(sd-getenvstealerase)", FORK_DEATHSIG|FORK_LOG|FORK_WAIT, NULL); if (r == 0) { _cleanup_strv_free_ char **l = NULL; /* child */ - assert_se(unsetenv_erase("thisenvvardefinitelywontexist") == 0); + assert_se(getenv_steal_erase("thisenvvardefinitelywontexist", NULL) == 0); l = strv_new("FOO=BAR", "QUUX=PIFF", "ONE=TWO", "A=B"); assert_se(strv_length(l) == 4); @@ -423,7 +423,7 @@ TEST(unsetenv_erase) { environ = l; STRV_FOREACH(e, environ) { - _cleanup_free_ char *n = NULL; + _cleanup_free_ char *n = NULL, *copy1 = NULL, *copy2 = NULL; char *eq; eq = strchr(*e, '='); @@ -433,9 +433,13 @@ TEST(unsetenv_erase) { n = strndup(*e, eq - *e); assert_se(n); - assert_se(streq_ptr(getenv(n), eq + 1)); + copy1 = strdup(eq + 1); + assert_se(copy1); + + assert_se(streq_ptr(getenv(n), copy1)); assert_se(getenv(n) == eq + 1); - assert_se(unsetenv_erase(n) > 0); + assert_se(getenv_steal_erase(n, ©2) > 0); + assert_se(streq_ptr(copy1, copy2)); assert_se(isempty(eq + 1)); assert_se(!getenv(n)); }