From 75e45d84d9391e29d49f6ec05272c3fb9a92bbd8 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
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));
}