Allow control characters in environment variable values

So far, we would allow certain control characters (NL since
b4346b9a77bc6129dd3e, TAB since 6294aa76d818e831de45), but not others. Having
other control characters in environment variable *value* is expected and widely
used, for various prompts like $LESS, $LESS_TERMCAP_*, and other similar
variables. The typical environment exported by bash already contains a dozen or
so such variables, so programs need to handle them.

We handle then correctly too, for example in 'systemctl show-environment',
since 804ee07c1370d49aa9a. But we would still disallow setting such variables
by the user, in unit file Environment= and in set-environment/import-environment
operations. This is unexpected and confusing and doesn't help with anything
because such variables are present in the environment through other means.

When printing such variables, 'show-environment' escapes all special
characters, so variables with control characters are plainly visible.
In other uses, e.g. 'cat -v' can be used in similar fashion. This would already
need to be done to suppress color codes starting with \[.

Note that we still forbid invalid utf-8 with this patch. (Control characters
are valid, since they are valid 7-bit ascii.) I'm not sure if we should do
that, but since people haven't been actually asking for invalid utf-8, and only
for control characters, and invalid utf-8 causes other issues, I think it's OK
to leave this unchanged.

Fixes #4446, https://gitlab.gnome.org/GNOME/gnome-session/-/issues/45.

diff --git a/man/systemctl.xml b/man/systemctl.xml
index a954e8727b6e5fedc7891a9d3e91fa748a33a029..6177d1a0dd5e5f22696963d0317b3a6a7c2ff7a6 100644 (file)
--- a/man/systemctl.xml
+++ b/man/systemctl.xml
@@ -1080,8 +1080,10 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err
       <para><command>systemd</command> supports an environment block that is passed to processes the manager
       spawns. The names of the variables can contain ASCII letters, digits, and the underscore
       character. Variable names cannot be empty or start with a digit. In variable values, most characters
-      are allowed, but non-printable characters are currently rejected. The total length of the environment
-      block is limited to <constant>_SC_ARG_MAX</constant> value defined by
+      are allowed, but the whole sequence must be valid UTF-8. (Note that control characters like newline
+      (<constant>NL</constant>), tab (<constant>TAB</constant>), or the escape character
+      (<constant>ESC</constant>), <emphasis>are</emphasis> valid ASCII and thus valid UTF-8). The total
+      length of the environment block is limited to <constant>_SC_ARG_MAX</constant> value defined by
       <citerefentry project='man-pages'><refentrytitle>sysconf</refentrytitle><manvolnum>3</manvolnum></citerefentry>.
       </para>
 

diff --git a/src/basic/env-util.c b/src/basic/env-util.c
index 03bdba022d7712e9785825079b5d3c94147c2351..96c024afd77d5638c2d75e2cfe1aeb8e53d503e2 100644 (file)
--- a/src/basic/env-util.c
+++ b/src/basic/env-util.c
@@ -57,16 +57,13 @@ bool env_value_is_valid(const char *e) {
         if (!utf8_is_valid(e))
                 return false;
 
-        /* bash allows tabs and newlines in environment variables, and so
-         * should we */
-        if (string_has_cc(e, "\t\n"))
-                return false;
+        /* Note that variable *values* may contain control characters, in particular NL, TAB, BS, DEL, ESC…
+         * When printing those variables with show-environment, we'll escape them. Make sure to print
+         * environment variables carefully! */
 
-        /* POSIX says the overall size of the environment block cannot
-         * be > ARG_MAX, an individual assignment hence cannot be
-         * either. Discounting the shortest possible variable name of
-         * length 1, the equal sign and trailing NUL this hence leaves
-         * ARG_MAX-3 as longest possible variable value. */
+        /* POSIX says the overall size of the environment block cannot be > ARG_MAX, an individual assignment
+         * hence cannot be either. Discounting the shortest possible variable name of length 1, the equal
+         * sign and trailing NUL this hence leaves ARG_MAX-3 as longest possible variable value. */
         if (strlen(e) > sc_arg_max() - 3)
                 return false;
 
@@ -86,10 +83,8 @@ bool env_assignment_is_valid(const char *e) {
         if (!env_value_is_valid(eq + 1))
                 return false;
 
-        /* POSIX says the overall size of the environment block cannot
-         * be > ARG_MAX, hence the individual variable assignments
-         * cannot be either, but let's leave room for one trailing NUL
-         * byte. */
+        /* POSIX says the overall size of the environment block cannot be > ARG_MAX, hence the individual
+         * variable assignments cannot be either, but let's leave room for one trailing NUL byte. */
         if (strlen(e) > sc_arg_max() - 1)
                 return false;
 

diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c
index dd150b30680c129e48ebb2070f1fff1055a073fb..f77b1cdbcbde6d841aaa58784462b7807bc6225a 100644 (file)
--- a/src/test/test-env-util.c
+++ b/src/test/test-env-util.c
@@ -265,6 +265,7 @@ static void test_env_clean(void) {
                                                 "another=one",
                                                 "another=final one",
                                                 "CRLF=\r\n",
+                                                "LESS_TERMCAP_mb=\x1b[01;31m",
                                                 "BASH_FUNC_foo%%=() {  echo foo\n}");
         assert_se(e);
         assert_se(!strv_env_is_valid(e));
@@ -277,7 +278,9 @@ static void test_env_clean(void) {
         assert_se(streq(e[3], "abcd=äöüß"));
         assert_se(streq(e[4], "xyz=xyz\n"));
         assert_se(streq(e[5], "another=final one"));
-        assert_se(e[6] == NULL);
+        assert_se(streq(e[6], "CRLF=\r\n"));
+        assert_se(streq(e[7], "LESS_TERMCAP_mb=\x1b[01;31m"));
+        assert_se(e[8] == NULL);
 }
 
 static void test_env_name_is_valid(void) {
@@ -302,8 +305,11 @@ static void test_env_value_is_valid(void) {
         assert_se(env_value_is_valid("printf \"\\x1b]0;<mock-chroot>\\x07<mock-chroot>\""));
         assert_se(env_value_is_valid("tab\tcharacter"));
         assert_se(env_value_is_valid("new\nline"));
-        assert_se(!env_value_is_valid("Show this?\rNope. Show that!"));
-        assert_se(!env_value_is_valid("new DOS\r\nline"));
+        assert_se(env_value_is_valid("Show this?\rNope. Show that!"));
+        assert_se(env_value_is_valid("new DOS\r\nline"));
+
+        assert_se(!env_value_is_valid("\xc5")); /* A truncated utf-8-encoded "ł".
+                                                 * We currently disallow that. */
 }
 
 static void test_env_assignment_is_valid(void) {