core: properly validate environment data from Environment= lines in unit files

4 files changed, 86 insertions, 88 deletions

diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4
index 1783ad058..0b6a5cc65 100644
--- a/src/core/load-fragment-gperf.gperf.m4
+++ b/src/core/load-fragment-gperf.gperf.m4
@@ -31,7 +31,7 @@ $1.CPUSchedulingPriority,        config_parse_exec_cpu_sched_prio,   0,
 $1.CPUSchedulingResetOnFork,     config_parse_bool,                  0,                             offsetof($1, exec_context.cpu_sched_reset_on_fork)
 $1.CPUAffinity,                  config_parse_exec_cpu_affinity,     0,                             offsetof($1, exec_context)
 $1.UMask,                        config_parse_mode,                  0,                             offsetof($1, exec_context.umask)
-$1.Environment,                  config_parse_unit_strv_printf,      0,                             offsetof($1, exec_context.environment)
+$1.Environment,                  config_parse_environ,               0,                             offsetof($1, exec_context.environment)
 $1.EnvironmentFile,              config_parse_unit_env_file,         0,                             offsetof($1, exec_context.environment_files)
 $1.StandardInput,                config_parse_input,                 0,                             offsetof($1, exec_context.std_input)
 $1.StandardOutput,               config_parse_output,                0,                             offsetof($1, exec_context.std_output)
diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
index 8436d4f95..44698d46e 100644
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -48,6 +48,7 @@
 #include "utf8.h"
 #include "path-util.h"
 #include "syscall-list.h"
+#include "env-util.h"
 
 #ifndef HAVE_SYSV_COMPAT
 int config_parse_warn_compat(
@@ -1486,9 +1487,10 @@ int config_parse_unit_env_file(
                 void *data,
                 void *userdata) {
 
-        char ***env = data, **k;
+        char ***env = data;
         Unit *u = userdata;
-        char *s;
+        _cleanup_free_ char *s = NULL;
+        int r;
 
         assert(filename);
         assert(lvalue);
@@ -1497,7 +1499,6 @@ int config_parse_unit_env_file(
 
         if (isempty(rvalue)) {
                 /* Empty assignment frees the list */
-
                 strv_free(*env);
                 *env = NULL;
                 return 0;
@@ -1509,17 +1510,67 @@ int config_parse_unit_env_file(
 
         if (!path_is_absolute(s[0] == '-' ? s + 1 : s)) {
                 log_error("[%s:%u] Path '%s' is not absolute, ignoring.", filename, line, s);
-                free(s);
                 return 0;
         }
 
-        k = strv_append(*env, s);
-        free(s);
+        r = strv_extend(env, s);
+        if (r < 0)
+                return log_oom();
+
+        return 0;
+}
+
+int config_parse_environ(
+                const char *filename,
+                unsigned line,
+                const char *section,
+                const char *lvalue,
+                int ltype,
+                const char *rvalue,
+                void *data,
+                void *userdata) {
+
+        Unit *u = userdata;
+        char*** env = data, *w, *state;
+        size_t l;
+        _cleanup_free_ char *k = NULL;
+
+        assert(filename);
+        assert(lvalue);
+        assert(rvalue);
+        assert(u);
+
+        if (isempty(rvalue)) {
+                /* Empty assignment resets the list */
+                strv_free(*env);
+                *env = NULL;
+                return 0;
+        }
+
+        k = unit_full_printf(u, rvalue);
         if (!k)
                 return log_oom();
 
-        strv_free(*env);
-        *env = k;
+        FOREACH_WORD_QUOTED(w, l, k, state) {
+                _cleanup_free_ char *n;
+                char **x;
+
+                n = cunescape_length(w, l);
+                if (!n)
+                        return log_oom();
+
+                if (!env_assignment_is_valid(n)) {
+                        log_error("[%s:%u] Invalid environment assignment, ignoring: %s", filename, line, rvalue);
+                        continue;
+                }
+
+                x = strv_env_set(*env, n);
+                if (!x)
+                        return log_oom();
+
+                strv_free(*env);
+                *env = x;
+        }
 
         return 0;
 }
diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h
index 24f738464..421b4c33e 100644
--- a/src/core/load-fragment.h
+++ b/src/core/load-fragment.h
@@ -82,6 +82,7 @@ int config_parse_unit_blkio_weight(const char *filename, unsigned line, const ch
 int config_parse_unit_blkio_bandwidth(const char *filename, unsigned line, const char *section, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
 int config_parse_unit_requires_mounts_for(const char *filename, unsigned line, const char *section, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
 int config_parse_syscall_filter(const char *filename, unsigned line, const char *section, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
+int config_parse_environ(const char *filename, unsigned line, const char *section, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
 
 /* gperf prototypes */
 const struct ConfigPerfItem* load_fragment_gperf_lookup(const char *key, unsigned length);
diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c
index 7f286096d..c5dd26db5 100644
--- a/src/shared/conf-parser.c
+++ b/src/shared/conf-parser.c
@@ -668,12 +668,8 @@ int config_parse_strv(
                 void *data,
                 void *userdata) {
 
-        char*** sv = data;
-        char **n;
-        char *w;
-        unsigned k;
+        char *** sv = data, *w, *state;
         size_t l;
-        char *state;
         int r;
 
         assert(filename);
@@ -685,50 +681,27 @@ int config_parse_strv(
                 /* Empty assignment resets the list */
                 strv_free(*sv);
                 *sv = NULL;
+                return 0;
         }
 
-        k = strv_length(*sv);
-        FOREACH_WORD_QUOTED(w, l, rvalue, state)
-                k++;
-
-        n = new(char*, k+1);
-        if (!n)
-                return log_oom();
-
-        if (*sv)
-                for (k = 0; (*sv)[k]; k++)
-                        n[k] = (*sv)[k];
-        else
-                k = 0;
-
         FOREACH_WORD_QUOTED(w, l, rvalue, state) {
-                n[k] = cunescape_length(w, l);
-                if (!n[k]) {
-                        r = log_oom();
-                        goto fail;
-                }
+                _cleanup_free_ char *n;
 
-                if (!utf8_is_valid(n[k])) {
-                        log_error("[%s:%u] String is not UTF-8 clean, ignoring assignment: %s", filename, line, rvalue);
-                        free(n[k]);
+                n = cunescape_length(w, l);
+                if (!n)
+                        return log_oom();
+
+                if (!utf8_is_valid(n)) {
+                        log_error("[%s:%u] String is not UTF-8 clean, ignoring: %s", filename, line, rvalue);
                         continue;
                 }
 
-                k++;
+                r = strv_extend(sv, n);
+                if (r < 0)
+                        return log_oom();
         }
 
-        n[k] = NULL;
-        free(*sv);
-        *sv = n;
-
         return 0;
-
-fail:
-        for (; k > 0; k--)
-                free(n[k-1]);
-        free(n);
-
-        return r;
 }
 
 int config_parse_path_strv(
@@ -741,12 +714,8 @@ int config_parse_path_strv(
                 void *data,
                 void *userdata) {
 
-        char*** sv = data;
-        char **n;
-        char *w;
-        unsigned k;
+        char*** sv = data, *w, *state;
         size_t l;
-        char *state;
         int r;
 
         assert(filename);
@@ -758,56 +727,33 @@ int config_parse_path_strv(
                 /* Empty assignment resets the list */
                 strv_free(*sv);
                 *sv = NULL;
+                return 0;
         }
 
-        k = strv_length(*sv);
-        FOREACH_WORD_QUOTED(w, l, rvalue, state)
-                k++;
-
-        n = new(char*, k+1);
-        if (!n)
-                return log_oom();
-
-        k = 0;
-        if (*sv)
-                for (; (*sv)[k]; k++)
-                        n[k] = (*sv)[k];
-
         FOREACH_WORD_QUOTED(w, l, rvalue, state) {
-                n[k] = strndup(w, l);
-                if (!n[k]) {
-                        r = log_oom();
-                        goto fail;
-                }
+                _cleanup_free_ char *n;
 
-                if (!utf8_is_valid(n[k])) {
+                n = strndup(w, l);
+                if (!n)
+                        return log_oom();
+
+                if (!utf8_is_valid(n)) {
                         log_error("[%s:%u] Path is not UTF-8 clean, ignoring assignment: %s", filename, line, rvalue);
-                        free(n[k]);
                         continue;
                 }
 
-                if (!path_is_absolute(n[k])) {
+                if (!path_is_absolute(n)) {
                         log_error("[%s:%u] Not an absolute path, ignoring: %s", filename, line, rvalue);
-                        free(n[k]);
                         continue;
                 }
 
-                path_kill_slashes(n[k]);
-                k++;
+                path_kill_slashes(n);
+                r = strv_extend(sv, n);
+                if (r < 0)
+                        return log_oom();
         }
 
-        n[k] = NULL;
-        free(*sv);
-        *sv = n;
-
         return 0;
-
-fail:
-        for (; k > 0; k--)
-                free(n[k-1]);
-        free(n);
-
-        return r;
 }
 
 int config_parse_usec(