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.

1 files changed, 8 insertions, 5 deletions

diff --git a/src/core/unit.h b/src/core/unit.h
index fc8edaade5..01cef89525 100644
--- 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 */