shared/bus-unit-util: fix PrivateTmp=/PrivateUsers=/ProtectControlGroups= and Ex...

For some fields, we perform careful parsing and verification on the sender
side. For other fields, we accept any string or strv. I think that actually
this is fine: we should optimize for the correct case, i.e. the user runs a
command that is valid. The server must perform parsing in all cases, so doing
the verification on the sender side doesn't add value. When doing parsing
locally, in case of invalid or unsupported input, we would generate the error
message locally, so we would avoid the D-Bus call, but the message itself is
not better and from the user's point of view, the result is the same. And by
doing the parsing only on the server side, we deal better with the case where
the sender has an older version of the software. By not doing verification, we
implicitly "support" new values. And when the sender has a newer version that
supports additional fields, that does not help as long as the server uses an
older version. So in case of version mismatches, parsing on the server side is
as good or better.

Resolves https://github.com/systemd/systemd/issues/37174.

diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c
index 48bee05bb3bdc91d9383aef6ebe467d34f824915..97bf9d264ecfc7f0391dfbffd6cef9300cf6dd1a 100644 (file)
--- a/src/shared/bus-unit-util.c
+++ b/src/shared/bus-unit-util.c
@@ -2088,6 +2088,28 @@ static int bus_append_protect_hostname(sd_bus_message *m, const char *field, con
         return 1;
 }
 
+static int bus_append_boolean_or_ex_string(sd_bus_message *m, const char *field, const char *eq) {
+        int r;
+
+        r = parse_boolean(eq);
+        if (r >= 0) {
+                if (endswith(field, "Ex"))
+                        field = strndupa_safe(field, strlen(field) - 2);
+
+                r = sd_bus_message_append(m, "(sv)", field, "b", r);
+        } else {
+                if (!endswith(field, "Ex"))
+                        field = strjoina(field, "Ex");
+
+                /* We allow any string through and let the server perform the verification. */
+                r = sd_bus_message_append(m, "(sv)", field, "s", eq);
+        }
+        if (r < 0)
+                return bus_log_create_error(r);
+
+        return 1;
+}
+
 static int bus_append_paths(sd_bus_message *m, const char *field, const char *eq) {
         return bus_append_trivial_array(m, "Paths", eq,
                                         "a(ss)", field, eq);
@@ -2363,9 +2385,6 @@ static const BusProperty execute_properties[] = {
         { "SyslogIdentifier",                      bus_append_string                             },
         { "ProtectSystem",                         bus_append_string                             },
         { "ProtectHome",                           bus_append_string                             },
-        { "PrivateTmpEx",                          bus_append_string                             },
-        { "PrivateUsersEx",                        bus_append_string                             },
-        { "ProtectControlGroupsEx",                bus_append_string                             },
         { "SELinuxContext",                        bus_append_string                             },
         { "RootImage",                             bus_append_string                             },
         { "RootVerity",                            bus_append_string                             },
@@ -2385,10 +2404,8 @@ static const BusProperty execute_properties[] = {
         { "TTYVHangup",                            bus_append_parse_boolean                      },
         { "TTYReset",                              bus_append_parse_boolean                      },
         { "TTYVTDisallocate",                      bus_append_parse_boolean                      },
-        { "PrivateTmp",                            bus_append_parse_boolean                      },
         { "PrivateDevices",                        bus_append_parse_boolean                      },
         { "PrivateNetwork",                        bus_append_parse_boolean                      },
-        { "PrivateUsers",                          bus_append_parse_boolean                      },
         { "PrivateMounts",                         bus_append_parse_boolean                      },
         { "PrivateIPC",                            bus_append_parse_boolean                      },
         { "NoNewPrivileges",                       bus_append_parse_boolean                      },
@@ -2401,7 +2418,6 @@ static const BusProperty execute_properties[] = {
         { "ProtectKernelModules",                  bus_append_parse_boolean                      },
         { "ProtectKernelLogs",                     bus_append_parse_boolean                      },
         { "ProtectClock",                          bus_append_parse_boolean                      },
-        { "ProtectControlGroups",                  bus_append_parse_boolean                      },
         { "MountAPIVFS",                           bus_append_parse_boolean                      },
         { "BindLogSockets",                        bus_append_parse_boolean                      },
         { "CPUSchedulingResetOnFork",              bus_append_parse_boolean                      },
@@ -2456,7 +2472,7 @@ static const BusProperty execute_properties[] = {
         { "LoadCredential",                        bus_append_load_credential                    },
         { "LoadCredentialEncrypted",               bus_append_load_credential                    },
         { "ImportCredential",                      bus_append_import_credential                  },
-        { "ImportCredentialEx",                    bus_append_import_credential                  },
+        { "ImportCredentialEx",                    bus_append_import_credential                  }, /* compat */
         { "LogExtraFields",                        bus_append_log_extra_fields                   },
         { "LogFilterPatterns",                     bus_append_log_filter_patterns                },
         { "StandardInput",                         bus_append_standard_inputs                    },
@@ -2491,7 +2507,13 @@ static const BusProperty execute_properties[] = {
         { "CacheDirectory",                        bus_append_directory                          },
         { "LogsDirectory",                         bus_append_directory                          },
         { "ProtectHostname",                       bus_append_protect_hostname                   },
-        { "ProtectHostnameEx",                     bus_append_protect_hostname                   },
+        { "ProtectHostnameEx",                     bus_append_protect_hostname                   }, /* compat */
+        { "PrivateTmp",                            bus_append_boolean_or_ex_string               },
+        { "PrivateTmpEx",                          bus_append_boolean_or_ex_string               }, /* compat */
+        { "ProtectControlGroups",                  bus_append_boolean_or_ex_string               },
+        { "ProtectControlGroupsEx",                bus_append_boolean_or_ex_string               }, /* compat */
+        { "PrivateUsers",                          bus_append_boolean_or_ex_string               },
+        { "PrivateUsersEx",                        bus_append_boolean_or_ex_string               }, /* compat */
 
         { NULL, bus_try_append_resource_limit,     dump_resource_limits                          },
         {}

diff --git a/src/test/test-bus-unit-util.c b/src/test/test-bus-unit-util.c
index 677615a2d3ea87352afb0b8cff2139cdaee6c464..2b21f119e542e54f47940db9a8b876bd7edacf17 100644 (file)
--- a/src/test/test-bus-unit-util.c
+++ b/src/test/test-bus-unit-util.c
@@ -271,11 +271,17 @@ TEST(execute_properties) {
                         "ProtectHome=true",
                         "ProtectHome=read-only",
                         "ProtectHome=tmpfs",
-
-                        "PrivateTmpEx=true",
-                        "PrivateTmpEx=false",
+                        "PrivateTmp=true",
+                        "PrivateTmp=fAlSe",
+                        "PrivateTmpEx=0001",
+                        "PrivateTmpEx=0000",
+                        "PrivateTmp=asdf",
+                        "PrivateTmpEx=asdf",
+                        "PrivateUsers=no",
+                        "PrivateUsers=1",
                         "PrivateUsersEx=true",
                         "PrivateUsersEx=false",
+                        "PrivateUsers=whatever",
                         "ProtectControlGroupsEx=true",
                         "ProtectControlGroupsEx=false",
 
@@ -314,10 +320,8 @@ TEST(execute_properties) {
                         "TTYVHangup=true",
                         "TTYReset=true",
                         "TTYVTDisallocate=true",
-                        "PrivateTmp=true",
                         "PrivateDevices=true",
                         "PrivateNetwork=true",
-                        "PrivateUsers=true",
                         "PrivateMounts=true",
                         "PrivateIPC=true",
                         "NoNewPrivileges=true",