core: skip ReadOnlyPaths= and other permission-related mounts on PermissionsStartOnly...

ReadOnlyPaths=, ProtectHome=, InaccessiblePaths= and ProtectSystem= are
about restricting access and little more, hence they should be disabled
if PermissionsStartOnly= is used or ExecStart= lines are prefixed with a
"+". Do that.

(Note that we will still create namespaces and stuff, since that's about
a lot more than just permissions. We'll simply disable the effect of
the four options mentioned above, but nothing else mount related.)

This also adds a test for this, to ensure this works as intended.

No documentation updates, as the documentation are already vague enough
to support the new behaviour ("If true, the permission-related execution
options…"). We could clarify this further, but I think we might want to
extend the switches' behaviour a bit more in future, hence leave it at
this for now.

Fixes: #5308

diff --git a/Makefile.am b/Makefile.am
index 447cf00..77e5aa7 100644 (file)
--- a/Makefile.am
+++ b/Makefile.am
@@ -1730,6 +1730,7 @@ EXTRA_DIST += \
        test/test-execute/exec-restrict-namespaces-yes.service \
        test/test-execute/exec-restrict-namespaces-mnt.service \
        test/test-execute/exec-restrict-namespaces-mnt-blacklist.service \
+       test/test-execute/exec-read-only-path-succeed.service \
        test/bus-policy/hello.conf \
        test/bus-policy/methods.conf \
        test/bus-policy/ownerships.conf \

diff --git a/src/core/execute.c b/src/core/execute.c
index 6041da4..4c2968f 100644 (file)
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -1938,10 +1938,13 @@ static int compile_read_write_paths(
         return 0;
 }
 
-static int apply_mount_namespace(Unit *u, const ExecContext *context,
-                                 const ExecParameters *params,
-                                 ExecRuntime *runtime) {
-        int r;
+static int apply_mount_namespace(
+                Unit *u,
+                ExecCommand *command,
+                const ExecContext *context,
+                const ExecParameters *params,
+                ExecRuntime *runtime) {
+
         _cleanup_strv_free_ char **rw = NULL;
         char *tmp = NULL, *var = NULL;
         const char *root_dir = NULL, *root_image = NULL;
@@ -1953,6 +1956,8 @@ static int apply_mount_namespace(Unit *u, const ExecContext *context,
                 .protect_kernel_modules = context->protect_kernel_modules,
                 .mount_apivfs = context->mount_apivfs,
         };
+        bool apply_restrictions;
+        int r;
 
         assert(context);
 
@@ -1986,16 +1991,18 @@ static int apply_mount_namespace(Unit *u, const ExecContext *context,
         if (!context->dynamic_user && root_dir)
                 ns_info.ignore_protect_paths = true;
 
+        apply_restrictions = (params->flags & EXEC_APPLY_PERMISSIONS) && !command->privileged;
+
         r = setup_namespace(root_dir, root_image,
                             &ns_info, rw,
-                            context->read_only_paths,
-                            context->inaccessible_paths,
+                            apply_restrictions ? context->read_only_paths : NULL,
+                            apply_restrictions ? context->inaccessible_paths : NULL,
                             context->bind_mounts,
                             context->n_bind_mounts,
                             tmp,
                             var,
-                            context->protect_home,
-                            context->protect_system,
+                            apply_restrictions ? context->protect_home : PROTECT_HOME_NO,
+                            apply_restrictions ? context->protect_system : PROTECT_SYSTEM_NO,
                             context->mount_flags,
                             DISSECT_IMAGE_DISCARD_ON_LOOP);
 
@@ -2606,7 +2613,7 @@ static int exec_child(
 
         needs_mount_namespace = exec_needs_mount_namespace(context, params, runtime);
         if (needs_mount_namespace) {
-                r = apply_mount_namespace(unit, context, params, runtime);
+                r = apply_mount_namespace(unit, command, context, params, runtime);
                 if (r < 0) {
                         *exit_status = EXIT_NAMESPACE;
                         return r;

diff --git a/src/test/test-execute.c b/src/test/test-execute.c
index bc9a202..1e479b9 100644 (file)
--- a/src/test/test-execute.c
+++ b/src/test/test-execute.c
@@ -422,6 +422,10 @@ static void test_exec_spec_interpolation(Manager *m) {
         test(m, "exec-spec-interpolation.service", 0, CLD_EXITED);
 }
 
+static void test_exec_read_only_path_suceed(Manager *m) {
+        test(m, "exec-read-only-path-succeed.service", 0, CLD_EXITED);
+}
+
 static int run_tests(UnitFileScope scope, const test_function_t *tests) {
         const test_function_t *test = NULL;
         Manager *m = NULL;
@@ -475,6 +479,7 @@ int main(int argc, char *argv[]) {
                 test_exec_oomscoreadjust,
                 test_exec_ioschedulingclass,
                 test_exec_spec_interpolation,
+                test_exec_read_only_path_suceed,
                 NULL,
         };
         static const test_function_t system_tests[] = {

diff --git a/test/test-execute/exec-read-only-path-succeed.service b/test/test-execute/exec-read-only-path-succeed.service
new file mode 100644 (file)
index 0000000..b54d48f
--- /dev/null
+++ b/test/test-execute/exec-read-only-path-succeed.service
@@ -0,0 +1,8 @@
+[Service]
+Type=oneshot
+# This should work, as we explicitly disable the effect of ReadOnlyPaths=
+ExecStart=+/bin/touch /tmp/thisisasimpletest
+# This should also work, as we do not disable the effect of ReadOnlyPaths= but invert the exit code
+ExecStart=/bin/sh -x -c '! /bin/touch /tmp/thisisasimpletest'
+ExecStart=+/bin/rm /tmp/thisisasimpletest
+ReadOnlyPaths=/tmp