Follow (mostly) POSIX rules for environment variables #17188

keszybz · 2020-09-28T15:40:49Z

No description provided.

poettering · 2020-09-28T16:03:24Z

Humpf, why? This sounds very wrong. Bash functions should stay in bash, I really don't grok why we would accept them as exported env vars?

keszybz · 2020-09-28T16:19:58Z

Humpf, why? This sounds very wrong. Bash functions should stay in bash, I really don't grok why we would accept them as exported env vars?

I'd look at it the other way around: why should we use bash rules for user variables names for anything else? Actually bash itself depends on the fact that it can export variables with names outside of this set to keep the user envvars and some other things that needs to be exported separate. Other shells have different sets of rules of what they allow as variables (I only checked zsh, but I'd expect e.g. fish to be creative in this regard). But it doesn't matter: we shouldn't limit the environment block to shell syntax.

poettering

I must say I am not a fan of this. uploading bash functions as env vars into systemd sounds like a really bad idea. I am not sure we should be a transport for something like that.

The two linked usecases seem borked to me. the first one i don#t grok at all... and the second one is strange: it appears they want to patch around in bash' environment to switch between alternatives of implementations of some tools? first of all, this switching doesn't appear to be something you want to do for bash only, if you really want to do that at all... other shells and services spawned by systemd --user probably should be covered too...

So, if they only care about bash: they can import these bash functions into their bash invocations by making them in ~/.bashrc, which would magically make them appear in all instances of bash without uploading them into anything...

if they care about user services, it appears they should use the environment generator stuff instead.

Which makes me wonder why they would need this PR?

poettering · 2020-09-28T16:14:36Z

src/basic/env-util.c

        if (!e)
                return false;

        if (n <= 0)
                return false;

-        if (e[0] >= '0' && e[0] <= '9')
-                return false;


urks. this is a security problem... Why this? did anyone ask for this?

Please, let's sanitize data. This is expressly mentioned in POSIX to be problematic and noone asked so far to allow this, so why change this?

poettering · 2020-09-28T16:15:53Z

src/basic/env-util.c

        DIGITS LETTERS                          \
        "_"

-static bool env_name_is_valid_n(const char *e, size_t n) {
-        const char *p;
+static bool printable_portable_character(char c) {


the function is a misnomer... the portable charset is larger...

Suggestions? nonwhitespace_portable_character?

poettering · 2020-09-28T16:17:20Z

src/systemctl/systemctl.c

+static void invalid_callback(const char *p, void *userdata) {
+        _cleanup_free_ char *t = cescape(p);
+
+        log_debug("Ignoring invalid environment assignment \"%s\".", strnull(t));


should this really just be debug?

I used debug because on my system there's quite a few variables which we refuse to export:
Ignoring invalid environment assignment "LESS_TERMCAP_mb=\033[01;31m".
Ignoring invalid environment assignment "LESS_TERMCAP_md=\033[01;31m".
Ignoring invalid environment assignment "LESS_TERMCAP_me=\033[0m".
Ignoring invalid environment assignment "LESS_TERMCAP_se=\033[0m".
Ignoring invalid environment assignment "LESS_TERMCAP_so=\033[01;44;33m".
Ignoring invalid environment assignment "LESS_TERMCAP_ue=\033[0m".
Ignoring invalid environment assignment "LESS_TERMCAP_us=\033[01;32m".
Logging at non-debug level about all of them would be annoying. So we should either relax our rules to allow the rvalues used here, or keep logging at debug level.

keszybz · 2020-09-28T16:35:54Z

uploading bash functions as env vars into systemd sounds like a really bad idea. I am not sure we should be a transport for something like that.

So... I think the more general question is what function is the environment block in systemd is supposed to have. Is it supposed to be like os-release, a limited set of restricted variable names, it is supposed to be like a storage for bash variables, is it supposed to be like storage for arbitrary shell variables, or is it supposed to be a general storage for posix envvars? I really don't want to be in the game of specifying which shell is the most important and what types of variables should be allowed. After all, privileges are required to insert variables here, there is no trust boundary, and just allowing stuff that POSIX says is portable seems like the most rational choice.

urks. this is a security problem... Why this? did anyone ask for this?
This is expressly mentioned in POSIX to be problematic and noone asked so far to allow this, so why change this?

I think this is a misunderstanding. Why would a variable name with digits be relevant to security? Such variables are special in shell syntax, because of various magic variables with digits there. For that reason it'd also be awkward to require users to set or consume variables with leading digits, for example as a way to modify systemd behaviour. But once exported into the environment block, it seems to me the variable is as safe as any other. I'm not saying that using such variables is a particularly useful thing, but if a user wants to, let them knock themselves out.

…owed There was some confusion about what POSIX says about variable names: names shall not contain the character '='. For values to be portable across systems conforming to POSIX.1-2008, the value shall be composed of characters from the portable character set (except NUL and as indicated below). i.e. it allows almost all ASCII in variable names (without NUL and DEL and '='). OTOH, it says that *utilities* use a smaller set of characters: Environment variable names used by the utilities in the Shell and Utilities volume of POSIX.1-2008 consist solely of uppercase letters, digits, and the <underscore> ( '_' ) from the characters defined in Portable Character Set and do not begin with a digit. When enforcing variable names in environment blocks, we need to use this first definition, so that we can propagate all valid variables. I think having non-printable characters in variable names is too much, so I took out the whitespace stuff from the first definition. OTOH, when we use *shell syntax*, for example doing variable expansion, it seems enough to support expansion of variables that the shell would allow. Fixes systemd#14878, https://bugzilla.redhat.com/show_bug.cgi?id=1754395, https://bugzilla.redhat.com/show_bug.cgi?id=1879216.

When doing import-environment, we shouldn't fail if some assignment is invalid. OTOH, if the invalid assignment is specified as a positional argument, we should keep failing. This would also fix https://bugzilla.redhat.com/show_bug.cgi?id=1754395, by ignoring certain variables which are not important in that scenario. It seems like the right thing to do in general.

keszybz · 2020-10-12T16:29:18Z

Rebased.

poettering · 2020-10-14T16:32:15Z

I still think Lmod or whatever package actually relies on this, and uploads bash code into the env block is really just broken. They shouldn't do that, and using systemd as a conduit for such terrible things is just sad.

That said, it probably is OK if we following POSIX naming rules on this, even though it's really a bad bad idea to pass potentially large pieces of bash-specific code around of this. It's conceptually a bad idea to pass literal, potentially large pieces of code around like this.

I really don#t grok why the offending package thinks its fun to dirty every process' env block with large code blobs like this as if they all where bash prorgams. They should just load that into bash (via .bashrc or .bash_profile), and not everything under the sun.

Summary: terrible usecase, but will merge this anyway.

kirelagin · 2020-10-14T21:43:05Z

Does this mean that show-environment is unsafe again, because it is now possible to have a shell injection in variable names, since, e.g. a semicolon is allowed?

keszybz · 2020-10-15T06:43:01Z

Oh, I forgot about show-environment. Yeah, we need to escape or skip those. I'll make a follow-up PR.

… are allowed" This reverts commit b45c068. I think the idea was generally sound, but didn't take into account the limitations of show-environment and how it is used. People expect to be able to eval systemctl show-environment output in bash, and no escaping syntax is defined for environment *names* (we only do escaping for *values*). We could skip such problematic variables in 'systemctl show-environment', and only allow them to be inherited directly. But this would be confusing and ugly. The original motivation for this change was that various import operations would fail. a4ccce2 changed systemctl to filter invalid variables in import-environment. https://gitlab.gnome.org/GNOME/gnome-session/-/issues/71 does a similar change in GNOME. So those problematic variables should not cause failures, but just be silently ignored. Finally, the environment block is becoming a dumping ground. In my gnome session 'systemctl show-environment --user' includes stuff like PWD, FPATH (from zsh), SHLVL=0 (no idea what that is). This is not directly related to variable names (since all those are allowed under the stricter rules too), but I think we should start pushing people away from running import-environment and towards importing only select variables. systemd#17188 (comment)

… are allowed" This reverts commit b45c068dd8fac7661a15e99e7cf699ff06010b13. I think the idea was generally sound, but didn't take into account the limitations of show-environment and how it is used. People expect to be able to eval systemctl show-environment output in bash, and no escaping syntax is defined for environment *names* (we only do escaping for *values*). We could skip such problematic variables in 'systemctl show-environment', and only allow them to be inherited directly. But this would be confusing and ugly. The original motivation for this change was that various import operations would fail. a4ccce22d9552dc74b6916cc5ec57f2a0b686b4f changed systemctl to filter invalid variables in import-environment. https://gitlab.gnome.org/GNOME/gnome-session/-/issues/71 does a similar change in GNOME. So those problematic variables should not cause failures, but just be silently ignored. Finally, the environment block is becoming a dumping ground. In my gnome session 'systemctl show-environment --user' includes stuff like PWD, FPATH (from zsh), SHLVL=0 (no idea what that is). This is not directly related to variable names (since all those are allowed under the stricter rules too), but I think we should start pushing people away from running import-environment and towards importing only select variables. systemd/systemd#17188 (comment)

… are allowed" This reverts commit b45c068. I think the idea was generally sound, but didn't take into account the limitations of show-environment and how it is used. People expect to be able to eval systemctl show-environment output in bash, and no escaping syntax is defined for environment *names* (we only do escaping for *values*). We could skip such problematic variables in 'systemctl show-environment', and only allow them to be inherited directly. But this would be confusing and ugly. The original motivation for this change was that various import operations would fail. a4ccce2 changed systemctl to filter invalid variables in import-environment. https://gitlab.gnome.org/GNOME/gnome-session/-/issues/71 does a similar change in GNOME. So those problematic variables should not cause failures, but just be silently ignored. Finally, the environment block is becoming a dumping ground. In my gnome session 'systemctl show-environment --user' includes stuff like PWD, FPATH (from zsh), SHLVL=0 (no idea what that is). This is not directly related to variable names (since all those are allowed under the stricter rules too), but I think we should start pushing people away from running import-environment and towards importing only select variables. systemd/systemd#17188 (comment) (cherry picked from commit ff46157)

keszybz added pid1 systemctl labels Sep 28, 2020

keszybz linked an issue Sep 28, 2020 that may be closed by this pull request

exporting bash functions as env vars are not compatible with systemctl's --import-environment operation #14878

Closed

poettering requested changes Sep 28, 2020

View reviewed changes

mrc0mmand added the needs-rebase label Oct 12, 2020

keszybz added 3 commits October 12, 2020 18:23

test-env-util: print function headers

0b34564

keszybz force-pushed the envvars-posix branch from 9f323e1 to a4ccce2 Compare October 12, 2020 16:29

keszybz removed the needs-rebase label Oct 12, 2020

poettering merged commit b0eb40c into systemd:master Oct 14, 2020

keszybz deleted the envvars-posix branch October 15, 2020 06:25

gbcox mentioned this pull request Jan 3, 2021

Plasma 5.20 has Black desktop screen when using wayland in conjunction with SDDM sddm/sddm#1335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow (mostly) POSIX rules for environment variables #17188

Follow (mostly) POSIX rules for environment variables #17188

keszybz commented Sep 28, 2020

poettering commented Sep 28, 2020

keszybz commented Sep 28, 2020

poettering left a comment •

edited

Loading

poettering Sep 28, 2020

poettering Sep 28, 2020

keszybz Sep 28, 2020

poettering Sep 28, 2020

keszybz Sep 28, 2020

keszybz commented Sep 28, 2020

keszybz commented Oct 12, 2020

poettering commented Oct 14, 2020

kirelagin commented Oct 14, 2020

keszybz commented Oct 15, 2020

Follow (mostly) POSIX rules for environment variables #17188

Follow (mostly) POSIX rules for environment variables #17188

Conversation

keszybz commented Sep 28, 2020

poettering commented Sep 28, 2020

keszybz commented Sep 28, 2020

poettering left a comment • edited Loading

Choose a reason for hiding this comment

poettering Sep 28, 2020

Choose a reason for hiding this comment

poettering Sep 28, 2020

Choose a reason for hiding this comment

keszybz Sep 28, 2020

Choose a reason for hiding this comment

poettering Sep 28, 2020

Choose a reason for hiding this comment

keszybz Sep 28, 2020

Choose a reason for hiding this comment

keszybz commented Sep 28, 2020

keszybz commented Oct 12, 2020

poettering commented Oct 14, 2020

kirelagin commented Oct 14, 2020

keszybz commented Oct 15, 2020

poettering left a comment •

edited

Loading