From c4899ea427fe93b5b2beac8ab10bfee7dfbf2021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 3 Jan 2021 21:53:38 +0100 Subject: [PATCH 1/3] systemctl: print a warning when trying to import a nonexistent variable I was quite confused what is happening: $ XXX=xxx $ systemctl --user import-environment XXX $ systemctl --user show-environment | grep XXX (nothing) Obviously, 'export XXX' was missing. Without any indication why the export is not happening, this can be hard to figure out. Another option would be to error out. But so far we didn't, and doing that could break some script which optimistically tries to export some variables, if present. --- src/systemctl/systemctl-set-environment.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/systemctl/systemctl-set-environment.c b/src/systemctl/systemctl-set-environment.c index 3be2c57778..437236efb4 100644 --- a/src/systemctl/systemctl-set-environment.c +++ b/src/systemctl/systemctl-set-environment.c @@ -139,21 +139,26 @@ int import_environment(int argc, char *argv[], void *userdata) { STRV_FOREACH(a, strv_skip(argv, 1)) { if (!env_name_is_valid(*a)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Not a valid environment variable name: %s", *a); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Not a valid environment variable name: %s", *a); + bool found = false; STRV_FOREACH(b, environ) { const char *eq; eq = startswith(*b, *a); if (eq && *eq == '=') { - r = sd_bus_message_append(m, "s", *b); if (r < 0) return bus_log_create_error(r); + found = true; break; } } + + if (!found) + log_notice("Environment variable $%s not set, ignoring.", *a); } r = sd_bus_message_close_container(m); From 30927a24848c4d727f7619cc74b878f098cdd724 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sun, 3 Jan 2021 22:26:52 +0100 Subject: [PATCH 2/3] 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. --- man/systemctl.xml | 6 ++++-- src/basic/env-util.c | 21 ++++++++------------- src/test/test-env-util.c | 12 +++++++++--- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/man/systemctl.xml b/man/systemctl.xml index a954e8727b..6177d1a0dd 100644 --- 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 systemd 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 _SC_ARG_MAX value defined by + are allowed, but the whole sequence must be valid UTF-8. (Note that control characters like newline + (NL), tab (TAB), or the escape character + (ESC), are valid ASCII and thus valid UTF-8). The total + length of the environment block is limited to _SC_ARG_MAX value defined by sysconf3. diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 03bdba022d..96c024afd7 100644 --- 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 dd150b3068..f77b1cdbcb 100644 --- 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;\\x07\"")); 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) { From e9155cd0778499a7e20edd2af8429247bbb1daa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 19 Jan 2021 14:20:16 +0100 Subject: [PATCH 3/3] systemctl: warn when importing environment variables with control characters I don't think it is useful to warn about about environemnt variables where the user explicitly configured some value. If they went through the effort of escaping the cc to include it in the setting (e.g. Environment="VAR=\efoo"), and we pass this through, there isn't anything to warn about. This also applies to 'systemctl set-environment', where the variable name and value are passed as arguments. The only case where the warning *might* be useful is where the user might be surprised by the value. This occurs when importing variables from the inherited environment, i.e. in 'systemctl import-environment'. In not convinced that this is useful, since the user better control their shell environment anyway. $ systemctl import-environment Calling import-environment without a list of variable names is deprecated. Environment variable $LESS_TERMCAP_mb contains control characters, importing anyway. Environment variable $LESS_TERMCAP_md contains control characters, importing anyway. Environment variable $LESS_TERMCAP_me contains control characters, importing anyway. Environment variable $LESS_TERMCAP_se contains control characters, importing anyway. Environment variable $LESS_TERMCAP_so contains control characters, importing anyway. Environment variable $LESS_TERMCAP_ue contains control characters, importing anyway. Environment variable $LESS_TERMCAP_us contains control characters, importing anyway. Environment variable $ZZZ contains control characters, importing anyway. --- src/systemctl/systemctl-set-environment.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/systemctl/systemctl-set-environment.c b/src/systemctl/systemctl-set-environment.c index 437236efb4..bfd87c0cdd 100644 --- a/src/systemctl/systemctl-set-environment.c +++ b/src/systemctl/systemctl-set-environment.c @@ -127,6 +127,12 @@ int import_environment(int argc, char *argv[], void *userdata) { strv_env_clean_with_callback(copy, invalid_callback, NULL); + char **e; + STRV_FOREACH(e, copy) + if (string_has_cc(*e, NULL)) + log_notice("Environment variable $%.*s contains control characters, importing anyway.", + (int) strcspn(*e, "="), *e); + r = sd_bus_message_append_strv(m, copy); } else { @@ -148,6 +154,10 @@ int import_environment(int argc, char *argv[], void *userdata) { eq = startswith(*b, *a); if (eq && *eq == '=') { + if (string_has_cc(eq + 1, NULL)) + log_notice("Environment variable $%.*s contains control characters, importing anyway.", + (int) (eq - *b), *b); + r = sd_bus_message_append(m, "s", *b); if (r < 0) return bus_log_create_error(r);