Tighten checking for variable validity

In the future we might want to allow additional syntax (for example
"unset VAR". But let's check that the data we're getting does not contain
anything unexpected.

5 files changed, 68 insertions, 2 deletions

diff --git a/man/environment.d.xml b/man/environment.d.xml
index 2302992fa5..4022c25c36 100644
--- a/man/environment.d.xml
+++ b/man/environment.d.xml
@@ -81,6 +81,9 @@
     and <literal>$OTHER_KEY</literal> format. No other elements of shell syntax are supported.
     </para>
 
+    <para>Each<replaceable>KEY</replaceable> must be a valid variable name. Empty lines
+    and lines beginning with the comment character <literal>#</literal> are ignored.</para>
+
     <refsect2>
       <title>Example</title>
       <example>
diff --git a/src/basic/exec-util.c b/src/basic/exec-util.c
index 207fac8dc1..aced9e8e3d 100644
--- a/src/basic/exec-util.c
+++ b/src/basic/exec-util.c
@@ -279,6 +279,11 @@ static int gather_environment_generate(int fd, void *arg) {
         STRV_FOREACH_PAIR(x, y, new) {
                 char *p;
 
+                if (!env_name_is_valid(*x)) {
+                        log_warning("Invalid variable assignment \"%s=...\", ignoring.", *x);
+                        continue;
+                }
+
                 p = strjoin(*x, "=", *y);
                 if (!p)
                         return -ENOMEM;
diff --git a/src/basic/fileio.c b/src/basic/fileio.c
index 3c2dab1855..8185f67e00 100644
--- a/src/basic/fileio.c
+++ b/src/basic/fileio.c
@@ -773,6 +773,16 @@ static int merge_env_file_push(
 
         assert(env);
 
+        if (!value) {
+                log_error("%s:%u: invalid syntax (around \"%s\"), ignoring.", strna(filename), line, key);
+                return 0;
+        }
+
+        if (!env_name_is_valid(key)) {
+                log_error("%s:%u: invalid variable name \"%s\", ignoring.", strna(filename), line, key);
+                return 0;
+        }
+
         expanded_value = replace_env(value, *env,
                                      REPLACE_ENV_USE_ENVIRONMENT|REPLACE_ENV_ALLOW_BRACELESS);
         if (!expanded_value)
diff --git a/src/test/test-exec-util.c b/src/test/test-exec-util.c
index 94fe042aa9..482b0751b9 100644
--- a/src/test/test-exec-util.c
+++ b/src/test/test-exec-util.c
@@ -305,8 +305,16 @@ static void test_environment_gathering(void) {
                                     "echo A=$A:24\n"
                                     "echo B=12\n"
                                     "echo C=000\n"
-                                    "echo C=001\n"                   /* variable overwriting */
-                                    "echo PATH=$PATH:/no/such/file", /* variable from manager */
+                                    "echo C=001\n"                    /* variable overwriting */
+                                     /* various invalid entries */
+                                    "echo unset A\n"
+                                    "echo unset A=\n"
+                                    "echo unset A=B\n"
+                                    "echo unset \n"
+                                    "echo A B=C\n"
+                                    "echo A\n"
+                                    /* test variable assignment without newline */
+                                    "echo PATH=$PATH:/no/such/file",   /* no newline */
                                     WRITE_STRING_FILE_CREATE) == 0);
 
         assert_se(chmod(name, 0755) == 0);
diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c
index c204cbae22..b117335db8 100644
--- a/src/test/test-fileio.c
+++ b/src/test/test-fileio.c
@@ -263,6 +263,45 @@ static void test_merge_env_file(void) {
         assert_se(a[6] == NULL);
 }
 
+static void test_merge_env_file_invalid(void) {
+        char t[] = "/tmp/test-fileio-XXXXXX";
+        int fd, r;
+        FILE *f;
+        _cleanup_strv_free_ char **a = NULL;
+        char **i;
+
+        fd = mkostemp_safe(t);
+        assert_se(fd >= 0);
+
+        log_info("/* %s (%s) */", __func__, t);
+
+        f = fdopen(fd, "w");
+        assert_se(f);
+
+        r = write_string_stream(f,
+                                "unset one   \n"
+                                "unset one=   \n"
+                                "unset one=1   \n"
+                                "one   \n"
+                                "one =  \n"
+                                "one two =\n"
+                                "\x20two=\n"
+                                "#comment=comment\n"
+                                ";comment2=comment2\n"
+                                "#\n"
+                                "\n\n"                  /* empty line */
+                                , false);
+        assert(r >= 0);
+
+        r = merge_env_file(&a, NULL, t);
+        assert_se(r >= 0);
+
+        STRV_FOREACH(i, a)
+                log_info("Got: <%s>", *i);
+
+        assert_se(strv_isempty(a));
+}
+
 static void test_executable_is_script(void) {
         char t[] = "/tmp/test-executable-XXXXXX";
         int fd, r;
@@ -620,6 +659,7 @@ int main(int argc, char *argv[]) {
         test_parse_env_file();
         test_parse_multiline_env_file();
         test_merge_env_file();
+        test_merge_env_file_invalid();
         test_executable_is_script();
         test_status_field();
         test_capeff();