From 881dbad1f10edc408490e9f2c53cd2e92c7ababc Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 3 Feb 2024 23:50:07 +0800 Subject: [PATCH 1/7] core/exec-credential: make param const where appropriate --- src/core/exec-credential.c | 7 +++---- src/core/exec-credential.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index 80ebd96f97..ef3033f67a 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -57,16 +57,15 @@ bool exec_context_has_credentials(const ExecContext *c) { !set_isempty(c->import_credentials); } -bool exec_context_has_encrypted_credentials(ExecContext *c) { - ExecLoadCredential *load_cred; - ExecSetCredential *set_cred; - +bool exec_context_has_encrypted_credentials(const ExecContext *c) { assert(c); + const ExecLoadCredential *load_cred; HASHMAP_FOREACH(load_cred, c->load_credentials) if (load_cred->encrypted) return true; + const ExecSetCredential *set_cred; HASHMAP_FOREACH(set_cred, c->set_credentials) if (set_cred->encrypted) return true; diff --git a/src/core/exec-credential.h b/src/core/exec-credential.h index 6f836fbd0b..9e4770d0b8 100644 --- a/src/core/exec-credential.h +++ b/src/core/exec-credential.h @@ -34,8 +34,8 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(ExecLoadCredential*, exec_load_credential_free); extern const struct hash_ops exec_set_credential_hash_ops; extern const struct hash_ops exec_load_credential_hash_ops; -bool exec_context_has_encrypted_credentials(ExecContext *c); bool exec_context_has_credentials(const ExecContext *c); +bool exec_context_has_encrypted_credentials(const ExecContext *c); int exec_context_get_credential_directory( const ExecContext *context, From d3131ea28c8ed46dbb33c72e42a64c0ca36fbbe8 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 3 Feb 2024 23:54:00 +0800 Subject: [PATCH 2/7] core/exec-invoke: don't duplicate needs_sandboxing condition --- src/core/exec-invoke.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index d11a915b45..3f64cc7827 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -3090,6 +3090,7 @@ static int apply_mount_namespace( const ExecParameters *params, ExecRuntime *runtime, const char *memory_pressure_path, + bool needs_sandboxing, char **error_path) { _cleanup_(verity_settings_done) VeritySettings verity = VERITY_SETTINGS_DEFAULT; @@ -3099,7 +3100,7 @@ static int apply_mount_namespace( *extension_dir = NULL, *host_os_release_stage = NULL, *root_image = NULL, *root_dir = NULL; const char *tmp_dir = NULL, *var_tmp_dir = NULL; char **read_write_paths; - bool needs_sandboxing, setup_os_release_symlink; + bool setup_os_release_symlink; BindMount *bind_mounts = NULL; size_t n_bind_mounts = 0; int r; @@ -3145,7 +3146,6 @@ static int apply_mount_namespace( } else read_write_paths = context->read_write_paths; - needs_sandboxing = (params->flags & EXEC_APPLY_SANDBOXING) && !(command_flags & EXEC_COMMAND_FULLY_PRIVILEGED); if (needs_sandboxing) { /* The runtime struct only contains the parent of the private /tmp, which is non-accessible * to world users. Inside of it there's a /tmp that is sticky, and that's the one we want to @@ -4738,7 +4738,13 @@ int exec_invoke( if (needs_mount_namespace) { _cleanup_free_ char *error_path = NULL; - r = apply_mount_namespace(command->flags, context, params, runtime, memory_pressure_path, &error_path); + r = apply_mount_namespace(command->flags, + context, + params, + runtime, + memory_pressure_path, + needs_sandboxing, + &error_path); if (r < 0) { *exit_status = EXIT_NAMESPACE; return log_exec_error_errno(context, params, r, "Failed to set up mount namespacing%s%s: %m", From a5801e9714636ec22a922520e045efd6e20249a6 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 4 Feb 2024 18:48:31 +0800 Subject: [PATCH 3/7] core/unit: use ASSERT_PTR and strdup_or_null more --- src/core/unit.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 0c48e5fa60..6dc13bcc9a 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4959,8 +4959,7 @@ int unit_setup_exec_runtime(Unit *u) { if (*rt) return 0; - ec = unit_get_exec_context(u); - assert(ec); + ec = ASSERT_PTR(unit_get_exec_context(u)); r = unit_get_transitive_dependency_set(u, UNIT_ATOM_JOINS_NAMESPACE_OF, &units); if (r < 0) @@ -5319,7 +5318,6 @@ int unit_acquire_invocation_id(Unit *u) { } int unit_set_exec_params(Unit *u, ExecParameters *p) { - const char *confirm_spawn; int r; assert(u); @@ -5332,12 +5330,9 @@ int unit_set_exec_params(Unit *u, ExecParameters *p) { p->runtime_scope = u->manager->runtime_scope; - confirm_spawn = manager_get_confirm_spawn(u->manager); - if (confirm_spawn) { - p->confirm_spawn = strdup(confirm_spawn); - if (!p->confirm_spawn) - return -ENOMEM; - } + r = strdup_or_null(manager_get_confirm_spawn(u->manager), &p->confirm_spawn); + if (r < 0) + return r; p->cgroup_supported = u->manager->cgroup_supported; p->prefix = u->manager->prefix; From 81006ebbd70f16f41b8717e3a3d383ad79bbf1cd Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 4 Feb 2024 00:25:03 +0800 Subject: [PATCH 4/7] core/service: introduce service_exec_flags As suggested in https://github.com/systemd/systemd/pull/31197#pullrequestreview-1861297477 Note that this slightly changes the behavior for ExecReload=, ExecCondition= and ExecStartPost=. Will be explained/corrected in later commits. --- src/core/service.c | 87 ++++++++++++++++++++++++++++++---------------- 1 file changed, 58 insertions(+), 29 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 8ea04c469d..a7bf1b8cfe 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -448,6 +448,7 @@ static void service_release_stdio_fd(Service *s) { s->stdout_fd = asynchronous_close(s->stdout_fd); s->stderr_fd = asynchronous_close(s->stderr_fd); } + static void service_done(Unit *u) { Service *s = SERVICE(u); @@ -1594,15 +1595,51 @@ static Service *service_get_triggering_service(Service *s) { return NULL; } +static ExecFlags service_exec_flags(ServiceExecCommand command_id) { + /* All service main/control processes honor sandboxing and namespacing options (except those + explicitly excluded in service_spawn()) */ + ExecFlags flags = EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT; + + assert(command_id >= 0); + assert(command_id < _SERVICE_EXEC_COMMAND_MAX); + + /* Control processes spawned before main process also get tty access */ + if (IN_SET(command_id, SERVICE_EXEC_CONDITION, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START, /* FIXME */ SERVICE_EXEC_STOP_POST)) + flags |= EXEC_APPLY_TTY_STDIN; + + /* All start phases get access to credentials */ + // FIXME: SERVICE_EXEC_START_POST + if (IN_SET(command_id, /* FIXME */ SERVICE_EXEC_CONDITION, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START, /* FIXME */ SERVICE_EXEC_RELOAD)) + flags |= EXEC_WRITE_CREDENTIALS; + + if (IN_SET(command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START)) + flags |= EXEC_SETENV_MONITOR_RESULT; + + if (command_id == SERVICE_EXEC_START) + return flags|EXEC_PASS_FDS|EXEC_SET_WATCHDOG; + + flags |= EXEC_IS_CONTROL; + + /* Put control processes spawned later than main process under .control sub-cgroup if appropriate */ + if (!IN_SET(command_id, SERVICE_EXEC_CONDITION, SERVICE_EXEC_START_PRE)) + flags |= EXEC_CONTROL_CGROUP; + + if (IN_SET(command_id, SERVICE_EXEC_STOP, SERVICE_EXEC_STOP_POST)) + flags |= EXEC_SETENV_RESULT; + + return flags; +} + static int service_spawn_internal( const char *caller, Service *s, + ServiceExecCommand command_id, ExecCommand *c, usec_t timeout, - ExecFlags flags, PidRef *ret_pid) { - _cleanup_(exec_params_shallow_clear) ExecParameters exec_params = EXEC_PARAMETERS_INIT(flags); + _cleanup_(exec_params_shallow_clear) ExecParameters exec_params = + EXEC_PARAMETERS_INIT(service_exec_flags(command_id)); _cleanup_(sd_event_source_unrefp) sd_event_source *exec_fd_source = NULL; _cleanup_strv_free_ char **final_env = NULL, **our_env = NULL; _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; @@ -1622,7 +1659,7 @@ static int service_spawn_internal( assert(!s->exec_fd_event_source); - if (flags & EXEC_IS_CONTROL) { + if (FLAGS_SET(exec_params.flags, EXEC_IS_CONTROL)) { /* If this is a control process, mask the permissions/chroot application if this is requested. */ if (s->permissions_start_only) exec_params.flags &= ~EXEC_APPLY_SANDBOXING; @@ -1630,7 +1667,7 @@ static int service_spawn_internal( exec_params.flags &= ~EXEC_APPLY_CHROOT; } - if ((flags & EXEC_PASS_FDS) || + if (FLAGS_SET(exec_params.flags, EXEC_PASS_FDS) || s->exec_context.std_input == EXEC_INPUT_SOCKET || s->exec_context.std_output == EXEC_OUTPUT_SOCKET || s->exec_context.std_error == EXEC_OUTPUT_SOCKET) { @@ -1648,7 +1685,7 @@ static int service_spawn_internal( log_unit_debug(UNIT(s), "Passing %zu fds to service", exec_params.n_socket_fds + exec_params.n_storage_fds); } - if (!FLAGS_SET(flags, EXEC_IS_CONTROL) && s->type == SERVICE_EXEC) { + if (!FLAGS_SET(exec_params.flags, EXEC_IS_CONTROL) && s->type == SERVICE_EXEC) { r = service_allocate_exec_fd(s, &exec_fd_source, &exec_params.exec_fd); if (r < 0) return r; @@ -1662,7 +1699,7 @@ static int service_spawn_internal( if (!our_env) return -ENOMEM; - if (service_exec_needs_notify_socket(s, flags)) { + if (service_exec_needs_notify_socket(s, exec_params.flags)) { if (asprintf(our_env + n_env++, "NOTIFY_SOCKET=%s", UNIT(s)->manager->notify_socket) < 0) return -ENOMEM; @@ -1721,10 +1758,10 @@ static int service_spawn_internal( Service *env_source = NULL; const char *monitor_prefix; - if (flags & EXEC_SETENV_RESULT) { + if (FLAGS_SET(exec_params.flags, EXEC_SETENV_RESULT)) { env_source = s; monitor_prefix = ""; - } else if (flags & EXEC_SETENV_MONITOR_RESULT) { + } else if (FLAGS_SET(exec_params.flags, EXEC_SETENV_MONITOR_RESULT)) { env_source = service_get_triggering_service(s); monitor_prefix = "MONITOR_"; } @@ -1742,18 +1779,15 @@ static int service_spawn_internal( r = asprintf(our_env + n_env++, "%sEXIT_STATUS=%i", monitor_prefix, env_source->main_exec_status.status); else r = asprintf(our_env + n_env++, "%sEXIT_STATUS=%s", monitor_prefix, signal_to_string(env_source->main_exec_status.status)); - if (r < 0) return -ENOMEM; } if (env_source != s) { - if (!sd_id128_is_null(UNIT(env_source)->invocation_id)) { - r = asprintf(our_env + n_env++, "%sINVOCATION_ID=" SD_ID128_FORMAT_STR, - monitor_prefix, SD_ID128_FORMAT_VAL(UNIT(env_source)->invocation_id)); - if (r < 0) + if (!sd_id128_is_null(UNIT(env_source)->invocation_id)) + if (asprintf(our_env + n_env++, "%sINVOCATION_ID=" SD_ID128_FORMAT_STR, + monitor_prefix, SD_ID128_FORMAT_VAL(UNIT(env_source)->invocation_id)) < 0) return -ENOMEM; - } if (asprintf(our_env + n_env++, "%sUNIT=%s", monitor_prefix, UNIT(env_source)->id) < 0) return -ENOMEM; @@ -2050,9 +2084,9 @@ static void service_enter_stop_post(Service *s, ServiceResult f) { pidref_done(&s->control_pid); r = service_spawn(s, + s->control_command_id, s->control_command, s->timeout_stop_usec, - EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_IS_CONTROL|EXEC_SETENV_RESULT|EXEC_CONTROL_CGROUP, &s->control_pid); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to spawn 'stop-post' task: %m"); @@ -2173,9 +2207,9 @@ static void service_enter_stop(Service *s, ServiceResult f) { pidref_done(&s->control_pid); r = service_spawn(s, + s->control_command_id, s->control_command, s->timeout_stop_usec, - EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL|EXEC_SETENV_RESULT|EXEC_CONTROL_CGROUP, &s->control_pid); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to spawn 'stop' task: %m"); @@ -2257,9 +2291,9 @@ static void service_enter_start_post(Service *s) { pidref_done(&s->control_pid); r = service_spawn(s, + s->control_command_id, s->control_command, s->timeout_start_usec, - EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL|EXEC_CONTROL_CGROUP, &s->control_pid); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to spawn 'start-post' task: %m"); @@ -2367,9 +2401,9 @@ static void service_enter_start(Service *s) { timeout = s->timeout_start_usec; r = service_spawn(s, + SERVICE_EXEC_START, c, timeout, - EXEC_PASS_FDS|EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_SET_WATCHDOG|EXEC_WRITE_CREDENTIALS|EXEC_SETENV_MONITOR_RESULT, &pidref); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to spawn 'start' task: %m"); @@ -2427,9 +2461,9 @@ static void service_enter_start_pre(Service *s) { s->control_command_id = SERVICE_EXEC_START_PRE; r = service_spawn(s, + s->control_command_id, s->control_command, s->timeout_start_usec, - EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL|EXEC_APPLY_TTY_STDIN|EXEC_SETENV_MONITOR_RESULT|EXEC_WRITE_CREDENTIALS, &s->control_pid); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to spawn 'start-pre' task: %m"); @@ -2464,9 +2498,9 @@ static void service_enter_condition(Service *s) { pidref_done(&s->control_pid); r = service_spawn(s, + s->control_command_id, s->control_command, s->timeout_start_usec, - EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL|EXEC_APPLY_TTY_STDIN, &s->control_pid); if (r < 0) { @@ -2575,9 +2609,9 @@ static void service_enter_reload(Service *s) { pidref_done(&s->control_pid); r = service_spawn(s, + s->control_command_id, s->control_command, s->timeout_start_usec, - EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL|EXEC_CONTROL_CGROUP, &s->control_pid); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to spawn 'reload' task: %m"); @@ -2631,14 +2665,9 @@ static void service_run_next_control(Service *s) { pidref_done(&s->control_pid); r = service_spawn(s, + s->control_command_id, s->control_command, timeout, - EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL| - (IN_SET(s->state, SERVICE_CONDITION, SERVICE_START_PRE, SERVICE_START, SERVICE_START_POST, SERVICE_RUNNING, SERVICE_RELOAD) ? EXEC_WRITE_CREDENTIALS : 0)| - (IN_SET(s->control_command_id, SERVICE_EXEC_CONDITION, SERVICE_EXEC_START_PRE, SERVICE_EXEC_STOP_POST) ? EXEC_APPLY_TTY_STDIN : 0)| - (IN_SET(s->control_command_id, SERVICE_EXEC_STOP, SERVICE_EXEC_STOP_POST) ? EXEC_SETENV_RESULT : 0)| - (IN_SET(s->control_command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START) ? EXEC_SETENV_MONITOR_RESULT : 0)| - (IN_SET(s->control_command_id, SERVICE_EXEC_START_POST, SERVICE_EXEC_RELOAD, SERVICE_EXEC_STOP, SERVICE_EXEC_STOP_POST) ? EXEC_CONTROL_CGROUP : 0), &s->control_pid); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to spawn next control task: %m"); @@ -2668,9 +2697,9 @@ static void service_run_next_main(Service *s) { service_unwatch_main_pid(s); r = service_spawn(s, + SERVICE_EXEC_START, s->main_command, s->timeout_start_usec, - EXEC_PASS_FDS|EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_SET_WATCHDOG|EXEC_SETENV_MONITOR_RESULT|EXEC_WRITE_CREDENTIALS, &pidref); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to spawn next main task: %m"); @@ -3896,7 +3925,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { s->control_command->command_next && f == SERVICE_SUCCESS) { - /* There is another command to * execute, so let's do that. */ + /* There is another command to execute, so let's do that. */ log_unit_debug(u, "Running next control command for state %s.", service_state_to_string(s->state)); service_run_next_control(s); From fe760177fe0c1d272f395c2fa144ec37194d80fa Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 4 Feb 2024 19:34:16 +0800 Subject: [PATCH 5/7] core/service: don't give ExecStopPost= commands tty access All tasks spawned later than ExecStart= (e.g. ExecReload=, ExecStop=, ...) don't get tty access. ExecStopPost= is the odd one out. Fix that. --- src/core/service.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/service.c b/src/core/service.c index a7bf1b8cfe..ac9986994d 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1604,7 +1604,7 @@ static ExecFlags service_exec_flags(ServiceExecCommand command_id) { assert(command_id < _SERVICE_EXEC_COMMAND_MAX); /* Control processes spawned before main process also get tty access */ - if (IN_SET(command_id, SERVICE_EXEC_CONDITION, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START, /* FIXME */ SERVICE_EXEC_STOP_POST)) + if (IN_SET(command_id, SERVICE_EXEC_CONDITION, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START)) flags |= EXEC_APPLY_TTY_STDIN; /* All start phases get access to credentials */ From a145623bc403e410f41808a8e5cb31d29a52567c Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 4 Feb 2024 19:36:06 +0800 Subject: [PATCH 6/7] core/service: don't setup credentials for ExecCondition= and ExecReload= This seems to be a mistake in #27279. I believe credentials should not be made available to condition or reload tasks. In most cases they're irrelevant from the actual job of the service. Also, currently the first ExecCondition= or ExecReload= cannot access creds anyway, making the incompatibility introduced negligible. If people actually come up with valid use cases, we can always revisit this. --- src/core/service.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/service.c b/src/core/service.c index ac9986994d..41fe842db6 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1609,7 +1609,7 @@ static ExecFlags service_exec_flags(ServiceExecCommand command_id) { /* All start phases get access to credentials */ // FIXME: SERVICE_EXEC_START_POST - if (IN_SET(command_id, /* FIXME */ SERVICE_EXEC_CONDITION, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START, /* FIXME */ SERVICE_EXEC_RELOAD)) + if (IN_SET(command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START)) flags |= EXEC_WRITE_CREDENTIALS; if (IN_SET(command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START)) From 39f4504de81c081e562139f1aab0a0388bc03b5f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 4 Feb 2024 19:38:20 +0800 Subject: [PATCH 7/7] core/service: allow ExecStartPost= cmds to access creds Fixes #31194 --- src/core/service.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 41fe842db6..38934390f0 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1608,8 +1608,7 @@ static ExecFlags service_exec_flags(ServiceExecCommand command_id) { flags |= EXEC_APPLY_TTY_STDIN; /* All start phases get access to credentials */ - // FIXME: SERVICE_EXEC_START_POST - if (IN_SET(command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START)) + if (IN_SET(command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START, SERVICE_EXEC_START_POST)) flags |= EXEC_WRITE_CREDENTIALS; if (IN_SET(command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START))