core: escape ExecStart command-line received over d-bus

When some transient unit setting is received over D-Bus, we write it it to a
transient unit file. We escape backslashes and specifiers. For most settings
this is enough, because most settings only do parsing and interpolation of
specifiers. When systemd-run is called (or something equivalent that gives us a
command strv), we write ExecStart=, but when reading it, we not only do parsing
and interpolation of specifiers, but also split on semicolons and do variable
substitution. This results in an ugly situation where the commandline is
interpolated twice, once on the caller side, and once in the manager.

I think we need to treat this as a bug: current behaviour seems to be an
accident of implementation and hard to explain in a reasonable way. If we
*were* doing specifier expansion, then it'd be somewhat reasonable so say that
"the commandline is handled the same as ExecStart=". But since we explicitly
prevent specifier expansion, we best we could say is "the commandline has some
subset of features of ExecStart=". I think this is not useful, and unexpected
by users. Since most people use use a shell to call systemd-run, one level of
variable expansion is already done on the caller side, and having to take into
account another level of expansion (with slightly different rules), creates a
big mental overhead when the commandline needs to include a dollar character or
such. Not doing any expansion is much cleaner and easier to explain or use.
Thus I think it's better to change behaviour here, even though in principle
some people could be relying on current behaviour. I think it's more likely
that nobody noticed, because people generally don't use systemd-run for
complicated commandlines.

Thus this commit adds an additional mode of escaping that prevents variable
explansion and other elements of ExecStart= syntax. I looked over all the
places where unit_escape_setting() is called, and I think that only two need to
be changed to use the new flag.

Fixes #23631.

diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c
index 8de092ee4e23054aa6d43add41f09ea3ec54a8b1..37d0afb0cbe34529f859d7d324c7e2f811f2b119 100644 (file)
--- a/src/core/dbus-execute.c
+++ b/src/core/dbus-execute.c
@@ -1591,7 +1591,7 @@ int bus_set_transient_exec_command(
                         if (!exec_chars)
                                 return -ENOMEM;
 
-                        a = unit_concat_strv(c->argv, UNIT_ESCAPE_C|UNIT_ESCAPE_SPECIFIERS);
+                        a = unit_concat_strv(c->argv, UNIT_ESCAPE_SPECIFIERS|UNIT_ESCAPE_EXEC_SYNTAX);
                         if (!a)
                                 return -ENOMEM;
 
@@ -1601,7 +1601,8 @@ int bus_set_transient_exec_command(
                                 _cleanup_free_ char *t = NULL;
                                 const char *p;
 
-                                p = unit_escape_setting(c->path, UNIT_ESCAPE_C|UNIT_ESCAPE_SPECIFIERS, &t);
+                                p = unit_escape_setting(c->path,
+                                                        UNIT_ESCAPE_SPECIFIERS|UNIT_ESCAPE_EXEC_SYNTAX, &t);
                                 if (!p)
                                         return -ENOMEM;
 

diff --git a/src/core/unit.c b/src/core/unit.c
index 5f1c6109b0bb89d427c837b47befb241cefd6209..05cee225edbb3e5d2ecf46859748f1bbcb5c8c5a 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -4254,51 +4254,62 @@ static const char* unit_drop_in_dir(Unit *u, UnitWriteFlags flags) {
 }
 
 char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf) {
-        char *ret = NULL;
+        assert(!FLAGS_SET(flags, UNIT_ESCAPE_EXEC_SYNTAX | UNIT_ESCAPE_C));
+
+        _cleanup_free_ char *t = NULL;
 
         if (!s)
                 return NULL;
 
-        /* Escapes the input string as requested. Returns the escaped string. If 'buf' is specified then the allocated
-         * return buffer pointer is also written to *buf, except if no escaping was necessary, in which case *buf is
-         * set to NULL, and the input pointer is returned as-is. This means the return value always contains a properly
-         * escaped version, but *buf when passed only contains a pointer if an allocation was necessary. If *buf is
-         * not specified, then the return value always needs to be freed. Callers can use this to optimize memory
-         * allocations. */
+        /* Escapes the input string as requested. Returns the escaped string. If 'buf' is specified then the
+         * allocated return buffer pointer is also written to *buf, except if no escaping was necessary, in
+         * which case *buf is set to NULL, and the input pointer is returned as-is. This means the return
+         * value always contains a properly escaped version, but *buf when passed only contains a pointer if
+         * an allocation was necessary. If *buf is not specified, then the return value always needs to be
+         * freed. Callers can use this to optimize memory allocations. */
 
         if (flags & UNIT_ESCAPE_SPECIFIERS) {
-                ret = specifier_escape(s);
-                if (!ret)
+                t = specifier_escape(s);
+                if (!t)
                         return NULL;
 
-                s = ret;
+                s = t;
         }
 
-        if (flags & UNIT_ESCAPE_C) {
-                char *a;
+        /* We either do c-escaping or shell-escaping, to additionally escape characters that we parse for
+         * ExecStart= and friend, i.e. '$' and ';' and quotes. */
+
+        if (flags & UNIT_ESCAPE_EXEC_SYNTAX) {
+                char *t2 = shell_escape(s, "$;'\"");
+                if (!t2)
+                        return NULL;
+                free_and_replace(t, t2);
+
+                s = t;
 
-                a = cescape(s);
-                free(ret);
-                if (!a)
+        } else if (flags & UNIT_ESCAPE_C) {
+                char *t2 = cescape(s);
+                if (!t2)
                         return NULL;
+                free_and_replace(t, t2);
 
-                ret = a;
+                s = t;
         }
 
         if (buf) {
-                *buf = ret;
-                return ret ?: (char*) s;
+                *buf = TAKE_PTR(t);
+                return (char*) s;
         }
 
-        return ret ?: strdup(s);
+        return TAKE_PTR(t) ?: strdup(s);
 }
 
 char* unit_concat_strv(char **l, UnitWriteFlags flags) {
         _cleanup_free_ char *result = NULL;
         size_t n = 0;
 
-        /* Takes a list of strings, escapes them, and concatenates them. This may be used to format command lines in a
-         * way suitable for ExecStart= stanzas */
+        /* Takes a list of strings, escapes them, and concatenates them. This may be used to format command
+         * lines in a way suitable for ExecStart= stanzas. */
 
         STRV_FOREACH(i, l) {
                 _cleanup_free_ char *buf = NULL;

diff --git a/src/core/unit.h b/src/core/unit.h
index fc8edaade53ecea791b1e0249c9c7ac7056af878..01cef89525e8f30f03d7e717d83ac313c99f9568 100644 (file)
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -531,19 +531,22 @@ typedef struct UnitStatusMessageFormats {
 /* Flags used when writing drop-in files or transient unit files */
 typedef enum UnitWriteFlags {
         /* Write a runtime unit file or drop-in (i.e. one below /run) */
-        UNIT_RUNTIME           = 1 << 0,
+        UNIT_RUNTIME            = 1 << 0,
 
         /* Write a persistent drop-in (i.e. one below /etc) */
-        UNIT_PERSISTENT        = 1 << 1,
+        UNIT_PERSISTENT         = 1 << 1,
 
         /* Place this item in the per-unit-type private section, instead of [Unit] */
-        UNIT_PRIVATE           = 1 << 2,
+        UNIT_PRIVATE            = 1 << 2,
 
         /* Apply specifier escaping before writing */
-        UNIT_ESCAPE_SPECIFIERS = 1 << 3,
+        UNIT_ESCAPE_SPECIFIERS  = 1 << 3,
+
+        /* Escape elements of ExecStart= syntax before writing */
+        UNIT_ESCAPE_EXEC_SYNTAX = 1 << 4,
 
         /* Apply C escaping before writing */
-        UNIT_ESCAPE_C          = 1 << 4,
+        UNIT_ESCAPE_C           = 1 << 5,
 } UnitWriteFlags;
 
 /* Returns true if neither persistent, nor runtime storage is requested, i.e. this is a check invocation only */