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, &copy2) > 0);

+                        assert_se(streq_ptr(copy1, copy2));

                         assert_se(isempty(eq + 1));

                         assert_se(!getenv(n));

                 }