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.

diff --git a/src/basic/env-util.c b/src/basic/env-util.c
index 885967e7f3f3fe66f99054e209da778546c86552..96b9613d24ad1f715d5089c2e8695628582216da 100644 (file)
--- a/src/basic/env-util.c
+++ b/src/basic/env-util.c
@@ -868,19 +868,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 38bfc8a3f2c18df98393404835329bdca16f5664..e4084af9a53c183ace53b31d8047fbdc45f51cde 100644 (file)
--- 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 1775912d8e52711de35bf9df4d1cd4d909ff7fe0..9b7c8b5400c86dbacdd85e71ae7169e9a76dfcc6 100644 (file)
--- 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 c9bc9a2489112d32998e04cc2c65f0c258e6ea5b..e13f5b7ac8596cd77809dfb6bbc79571f0f18d01 100644 (file)
--- a/src/cryptenroll/cryptenroll.c
+++ b/src/cryptenroll/cryptenroll.c
@@ -409,8 +409,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;
 
@@ -445,23 +445,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 35d5dbe007568bde3b75ad93d7cece01d91b2d4e..74053b8ce3478ed7c562d24833b4daa99967e7b6 100644 (file)
--- 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 746d428a9bdd3673a30fb9b6aff4cbb32d12cadb..56a3eedacc9a04905f02e96a531d6bce0d3d3de1 100644 (file)
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -818,20 +818,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 1e3c96f5addf0f0873e51ac10b2fce527cfc62e6..3c179ca4aea673fb9943a2f4830fa5d3cb3ac327 100644 (file)
--- 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;
         }
 
@@ -1150,33 +1153,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 67ea44515ab9ba0b2874bf1339a89f19b606a6e8..b01b9983c2b137c6e17d986246b1a15ba3e07e93 100644 (file)
--- a/src/shared/pkcs11-util.c
+++ b/src/shared/pkcs11-util.c
@@ -275,15 +275,17 @@ int pkcs11_token_login(
 
         for (unsigned tries = 0; tries < 3; tries++) {
                 _cleanup_strv_free_erase_ char **passwords = NULL;
-                char **i, *e;
+                _cleanup_(erase_and_freep) char *envpin = NULL;
+                char **i;
 
-                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 19523aa0d9c75ca0af2a2c00e3abaed28e6f6e4f..99ed0d2a050f962a5e34437593ca61d5d726b96a 100644 (file)
--- a/src/test/test-env-util.c
+++ b/src/test/test-env-util.c
@@ -406,17 +406,17 @@ 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;
                 char **e;
 
                 /* 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);
@@ -424,7 +424,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, '=');
@@ -434,9 +434,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, &copy2) > 0);
+                        assert_se(streq_ptr(copy1, copy2));
                         assert_se(isempty(eq + 1));
                         assert_se(!getenv(n));
                 }