From 73ff4d48de888d6f9908f5383b70a4d53e8edd7a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 25 Aug 2023 15:54:52 +0900 Subject: [PATCH 1/5] Revert "core: do not leak mount for credentials directory if mount namespace is enabled" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commits - 9ae3624889b98f75efa6fd0c5f4b4de3eaf328d4 "test-execute: add tests for credentials directory with mount namespace"↲ - 94fe4cf2557d1f70f20ee02d32f4c2ae6bc1fb3f "core: do not leak mount for credentials directory if mount namespace is enabled", - 7241b9cd72d6e6079d5140cf24c34e78d3cf43cc "core/credential: make setup_credentials() return path to credentials directory", - fbaf3b23ae4aa79110ebd37aada70ce6a044c692 "core: set $CREDENTIALS_DIRECTORY only when we set up credentials" Before the commits, credentials directory set up on ExecStart= was kept on e.g. ExecStop=. But, with the changes, if a service requests a private mount namespace, the credentials directory is discarded after ExecStart= is finished. Let's revert the change, and find better way later. Addresses the post-merge comment https://github.com/systemd/systemd/pull/28787#issuecomment-1690614202. --- src/core/credential.c | 78 ++----------------- src/core/credential.h | 4 +- src/core/execute.c | 41 ++++------ src/core/namespace.c | 34 ++------ src/core/namespace.h | 1 - src/test/test-execute.c | 4 - src/test/test-namespace.c | 1 - src/test/test-ns.c | 1 - ...ad-credential-with-mount-namespace.service | 9 --- .../exec-load-credential-with-seccomp.service | 9 --- ...et-credential-with-mount-namespace.service | 9 --- .../exec-set-credential-with-seccomp.service | 9 --- 12 files changed, 30 insertions(+), 170 deletions(-) delete mode 100644 test/test-execute/exec-load-credential-with-mount-namespace.service delete mode 100644 test/test-execute/exec-load-credential-with-seccomp.service delete mode 100644 test/test-execute/exec-set-credential-with-mount-namespace.service delete mode 100644 test/test-execute/exec-set-credential-with-seccomp.service diff --git a/src/core/credential.c b/src/core/credential.c index d9a88004c5..b8b8b4edaa 100644 --- a/src/core/credential.c +++ b/src/core/credential.c @@ -10,7 +10,6 @@ #include "glob-util.h" #include "io-util.h" #include "label-util.h" -#include "missing_syscall.h" #include "mkdir-label.h" #include "mount-util.h" #include "mountpoint-util.h" @@ -18,7 +17,6 @@ #include "random-util.h" #include "recurse-dir.h" #include "rm-rf.h" -#include "socket-util.h" #include "tmpfile-util.h" ExecSetCredential *exec_set_credential_free(ExecSetCredential *sc) { @@ -732,8 +730,7 @@ static int setup_credentials_internal( bool reuse_workspace, /* Whether to reuse any existing workspace mount if it already is a mount */ bool must_mount, /* Whether to require that we mount something, it's not OK to use the plain directory fall back */ uid_t uid, - gid_t gid, - int *ret_mount_fd) { + gid_t gid) { int r, workspace_mounted; /* negative if we don't know yet whether we have/can mount something; true * if we mounted something; false if we definitely can't mount anything */ @@ -851,27 +848,6 @@ static int setup_credentials_internal( if (r < 0) return r; - if (ret_mount_fd) { - _cleanup_close_ int mount_fd = -EBADF; - - r = mount_fd = RET_NERRNO(open_tree(AT_FDCWD, workspace, OPEN_TREE_CLONE | OPEN_TREE_CLOEXEC)); - if (r >= 0) { - /* The workspace is already cloned in the above, and not necessary - * anymore. Even though the workspace is unmounted when the short-lived - * child process exits, let's explicitly unmount it here for safety. */ - r = umount_verbose(LOG_DEBUG, workspace, MNT_DETACH|UMOUNT_NOFOLLOW); - if (r < 0) - return r; - - *ret_mount_fd = TAKE_FD(mount_fd); - return 0; - } - - /* Old kernel? Unprivileged? */ - if (!ERRNO_IS_NOT_SUPPORTED(r) && !ERRNO_IS_PRIVILEGE(r)) - return r; - } - /* And mount it to the final place, read-only */ r = mount_nofollow_verbose(LOG_DEBUG, workspace, final, NULL, MS_MOVE, NULL); } else @@ -892,8 +868,6 @@ static int setup_credentials_internal( return -errno; } - if (ret_mount_fd) - *ret_mount_fd = -EBADF; return 0; } @@ -902,25 +876,16 @@ int setup_credentials( const ExecParameters *params, const char *unit, uid_t uid, - gid_t gid, - char **ret_path, - int *ret_mount_fd) { + gid_t gid) { - _cleanup_close_pair_ int socket_pair[2] = PIPE_EBADF; _cleanup_free_ char *p = NULL, *q = NULL; - _cleanup_close_ int mount_fd = -EBADF; int r; assert(context); assert(params); - assert(ret_path); - assert(ret_mount_fd); - if (!exec_context_has_credentials(context)) { - *ret_path = NULL; - *ret_mount_fd = -EBADF; + if (!exec_context_has_credentials(context)) return 0; - } if (!params->prefix[EXEC_DIRECTORY_RUNTIME]) return -EINVAL; @@ -943,12 +908,7 @@ int setup_credentials( if (r < 0 && r != -EEXIST) return r; - if (socketpair(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0, socket_pair) < 0) - return -errno; - - r = safe_fork_full("(sd-mkdcreds)", - NULL, &socket_pair[1], 1, - FORK_DEATHSIG|FORK_CLOSE_ALL_FDS|FORK_WAIT|FORK_NEW_MOUNTNS|FORK_REOPEN_LOG, NULL); + r = safe_fork("(sd-mkdcreds)", FORK_DEATHSIG|FORK_WAIT|FORK_NEW_MOUNTNS, NULL); if (r < 0) { _cleanup_free_ char *t = NULL, *u = NULL; @@ -984,15 +944,14 @@ int setup_credentials( true, /* reuse the workspace if it is already a mount */ false, /* it's OK to fall back to a plain directory if we can't mount anything */ uid, - gid, - NULL); + gid); (void) rmdir(u); /* remove the workspace again if we can. */ if (r < 0) return r; - } else if (r == 0) { /* child */ + } else if (r == 0) { /* We managed to set up a mount namespace, and are now in a child. That's great. In this case * we can use the same directory for all cases, after turning off propagation. Question @@ -1011,8 +970,6 @@ int setup_credentials( * given that we do this in a privately namespaced short-lived single-threaded process that * no one else sees this should be OK to do. */ - _cleanup_close_ int fd = -EBADF; - /* Turn off propagation from our namespace to host */ r = mount_nofollow_verbose(LOG_DEBUG, NULL, "/dev", NULL, MS_SLAVE|MS_REC, NULL); if (r < 0) @@ -1027,14 +984,7 @@ int setup_credentials( false, /* do not reuse /dev/shm if it is already a mount, under no circumstances */ true, /* insist that something is mounted, do not allow fallback to plain directory */ uid, - gid, - &fd); - if (r < 0) - goto child_fail; - - r = send_one_fd_iov(socket_pair[1], fd, - &IOVEC_MAKE((int[]) { fd >= 0 }, sizeof(int)), 1, - MSG_DONTWAIT); + gid); if (r < 0) goto child_fail; @@ -1042,17 +992,6 @@ int setup_credentials( child_fail: _exit(EXIT_FAILURE); - - } else { /* parent */ - - int ret; - struct iovec iov = IOVEC_MAKE(&ret, sizeof(int)); - - r = receive_one_fd_iov(socket_pair[0], &iov, 1, MSG_DONTWAIT, &mount_fd); - if (r < 0) - return r; - if (ret > 0 && mount_fd < 0) - return -EIO; } /* If the credentials dir is empty and not a mount point, then there's no point in having it. Let's @@ -1060,8 +999,5 @@ int setup_credentials( * actually end up mounting anything on it. In that case we'd rather have ENOENT than EACCESS being * seen by users when trying access this inode. */ (void) rmdir(p); - - *ret_path = TAKE_PTR(p); - *ret_mount_fd = TAKE_FD(mount_fd); return 0; } diff --git a/src/core/credential.h b/src/core/credential.h index 4b936b32d6..54155f515b 100644 --- a/src/core/credential.h +++ b/src/core/credential.h @@ -45,6 +45,4 @@ int setup_credentials( const ExecParameters *params, const char *unit, uid_t uid, - gid_t gid, - char **ret_path, - int *ret_mount_fd); + gid_t gid); diff --git a/src/core/execute.c b/src/core/execute.c index eb452a45ec..14e196d4c7 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -7,7 +7,6 @@ #include #include #include -#include #include #include #include @@ -73,7 +72,6 @@ #include "missing_fs.h" #include "missing_ioprio.h" #include "missing_prctl.h" -#include "missing_syscall.h" #include "mkdir-label.h" #include "namespace.h" #include "parse-util.h" @@ -1866,7 +1864,6 @@ static int build_environment( dev_t journal_stream_dev, ino_t journal_stream_ino, const char *memory_pressure_path, - const char *creds_path, char ***ret) { _cleanup_strv_free_ char **our_env = NULL; @@ -2044,8 +2041,8 @@ static int build_environment( our_env[n_env++] = x; } - if (creds_path) { - x = strjoin("CREDENTIALS_DIRECTORY=", creds_path); + if (exec_context_has_credentials(c) && p->prefix[EXEC_DIRECTORY_RUNTIME]) { + x = strjoin("CREDENTIALS_DIRECTORY=", p->prefix[EXEC_DIRECTORY_RUNTIME], "/credentials/", u->id); if (!x) return -ENOMEM; @@ -3113,14 +3110,12 @@ static int apply_mount_namespace( const ExecParameters *params, ExecRuntime *runtime, const char *memory_pressure_path, - const char *creds_path, - int creds_fd, char **error_path) { _cleanup_(verity_settings_done) VeritySettings verity = VERITY_SETTINGS_DEFAULT; _cleanup_strv_free_ char **empty_directories = NULL, **symlinks = NULL, **read_write_paths_cleanup = NULL; - _cleanup_free_ char *incoming_dir = NULL, *propagate_dir = NULL, + _cleanup_free_ char *creds_path = NULL, *incoming_dir = NULL, *propagate_dir = NULL, *extension_dir = NULL, *host_os_release_stage = NULL; const char *root_dir = NULL, *root_image = NULL, *tmp_dir = NULL, *var_tmp_dir = NULL; char **read_write_paths; @@ -3222,6 +3217,14 @@ static int apply_mount_namespace( if (context->mount_propagation_flag == MS_SHARED) log_unit_debug(u, "shared mount propagation hidden by other fs namespacing unit settings: ignoring"); + if (exec_context_has_credentials(context) && + params->prefix[EXEC_DIRECTORY_RUNTIME] && + FLAGS_SET(params->flags, EXEC_WRITE_CREDENTIALS)) { + creds_path = path_join(params->prefix[EXEC_DIRECTORY_RUNTIME], "credentials", u->id); + if (!creds_path) + return -ENOMEM; + } + if (params->runtime_scope == RUNTIME_SCOPE_SYSTEM) { propagate_dir = path_join("/run/systemd/propagate/", u->id); if (!propagate_dir) @@ -3290,7 +3293,6 @@ static int apply_mount_namespace( tmp_dir, var_tmp_dir, creds_path, - creds_fd, context->log_namespace, context->mount_propagation_flag, &verity, @@ -3944,7 +3946,7 @@ static int exec_child( int r, ngids = 0, exec_fd; _cleanup_free_ gid_t *supplementary_gids = NULL; const char *username = NULL, *groupname = NULL; - _cleanup_free_ char *home_buffer = NULL, *memory_pressure_path = NULL, *creds_path = NULL; + _cleanup_free_ char *home_buffer = NULL, *memory_pressure_path = NULL; const char *home = NULL, *shell = NULL; char **final_argv = NULL; dev_t journal_stream_dev = 0; @@ -3975,7 +3977,6 @@ static int exec_child( int ngids_after_pam = 0; _cleanup_free_ int *fds = NULL; _cleanup_strv_free_ char **fdnames = NULL; - _cleanup_close_ int creds_fd = -EBADF; assert(unit); assert(command); @@ -4426,7 +4427,7 @@ static int exec_child( } if (FLAGS_SET(params->flags, EXEC_WRITE_CREDENTIALS)) { - r = setup_credentials(context, params, unit->id, uid, gid, &creds_path, &creds_fd); + r = setup_credentials(context, params, unit->id, uid, gid); if (r < 0) { *exit_status = EXIT_CREDENTIALS; return log_unit_error_errno(unit, r, "Failed to set up credentials: %m"); @@ -4446,7 +4447,6 @@ static int exec_child( journal_stream_dev, journal_stream_ino, memory_pressure_path, - creds_path, &our_env); if (r < 0) { *exit_status = EXIT_MEMORY; @@ -4640,7 +4640,7 @@ static int exec_child( if (needs_mount_namespace) { _cleanup_free_ char *error_path = NULL; - r = apply_mount_namespace(unit, command->flags, context, params, runtime, memory_pressure_path, creds_path, creds_fd, &error_path); + r = apply_mount_namespace(unit, command->flags, context, params, runtime, memory_pressure_path, &error_path); if (r < 0) { *exit_status = EXIT_NAMESPACE; return log_unit_error_errno(unit, r, "Failed to set up mount namespacing%s%s: %m", @@ -4648,19 +4648,6 @@ static int exec_child( } } - if (creds_fd >= 0) { - assert(creds_path); - - /* When a mount namespace is not requested, then the target directory may not exist yet. - * Here, we ignore the failure, as if it fails, the subsequent move_mount() will fail. */ - (void) mkdir_p_label(creds_path, 0755); - - if (move_mount(creds_fd, "", AT_FDCWD, creds_path, MOVE_MOUNT_F_EMPTY_PATH) < 0) { - *exit_status = EXIT_CREDENTIALS; - return log_unit_error_errno(unit, errno, "Failed to mount credentials directory on %s: %m", creds_path); - } - } - if (needs_sandboxing) { r = apply_protect_hostname(unit, context, exit_status); if (r < 0) diff --git a/src/core/namespace.c b/src/core/namespace.c index 2197287fd0..e2304f5d06 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -74,8 +74,7 @@ typedef enum MountMode { EXTENSION_DIRECTORIES, /* Bind-mounted outside the root directory, and used by subsequent mounts */ EXTENSION_IMAGES, /* Mounted outside the root directory, and used by subsequent mounts */ MQUEUEFS, - READWRITE_IMPLICIT, /* Should have the 2nd lowest priority. */ - MKDIR, /* Should have the lowest priority. */ + READWRITE_IMPLICIT, /* Should have the lowest priority. */ _MOUNT_MODE_MAX, } MountMode; @@ -232,7 +231,6 @@ static const char * const mount_mode_table[_MOUNT_MODE_MAX] = { [EXTENSION_IMAGES] = "extension-images", [MQUEUEFS] = "mqueuefs", [READWRITE_IMPLICIT] = "read-write-implicit", - [MKDIR] = "mkdir", }; /* Helper struct for naming simplicity and reusability */ @@ -1549,12 +1547,6 @@ static int apply_one_mount( case OVERLAY_MOUNT: return mount_overlay(m); - case MKDIR: - r = mkdir_p_label(mount_entry_path(m), 0755); - if (r < 0) - return r; - return 1; - default: assert_not_reached(); } @@ -2021,7 +2013,6 @@ int setup_namespace( const char* tmp_dir, const char* var_tmp_dir, const char *creds_path, - int creds_fd, const char *log_namespace, unsigned long mount_propagation_flag, VeritySettings *verity, @@ -2344,22 +2335,13 @@ int setup_namespace( .flags = MS_NODEV|MS_STRICTATIME|MS_NOSUID|MS_NOEXEC, }; - /* If we have mount fd for credentials directory, then it will be mounted after - * namespace is set up. So, here we only create the mount point. */ - - if (creds_fd < 0) - *(m++) = (MountEntry) { - .path_const = creds_path, - .mode = BIND_MOUNT, - .read_only = true, - .source_const = creds_path, - .ignore = true, - }; - else - *(m++) = (MountEntry) { - .path_const = creds_path, - .mode = MKDIR, - }; + *(m++) = (MountEntry) { + .path_const = creds_path, + .mode = BIND_MOUNT, + .read_only = true, + .source_const = creds_path, + .ignore = true, + }; } else { /* If our service has no credentials store configured, then make the whole * credentials tree inaccessible wholesale. */ diff --git a/src/core/namespace.h b/src/core/namespace.h index b8615cbd98..b6132154c5 100644 --- a/src/core/namespace.h +++ b/src/core/namespace.h @@ -122,7 +122,6 @@ int setup_namespace( const char *tmp_dir, const char *var_tmp_dir, const char *creds_path, - int creds_fd, const char *log_namespace, unsigned long mount_propagation_flag, VeritySettings *verity, diff --git a/src/test/test-execute.c b/src/test/test-execute.c index 847415b9ae..0be66c2c7b 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -282,11 +282,7 @@ static void test_exec_cpuaffinity(Manager *m) { static void test_exec_credentials(Manager *m) { test(m, "exec-set-credential.service", 0, CLD_EXITED); - test(m, "exec-set-credential-with-mount-namespace.service", 0, CLD_EXITED); - test(m, "exec-set-credential-with-seccomp.service", 0, CLD_EXITED); test(m, "exec-load-credential.service", MANAGER_IS_SYSTEM(m) ? 0 : EXIT_CREDENTIALS, CLD_EXITED); - test(m, "exec-load-credential-with-mount-namespace.service", MANAGER_IS_SYSTEM(m) ? 0 : EXIT_CREDENTIALS, CLD_EXITED); - test(m, "exec-load-credential-with-seccomp.service", MANAGER_IS_SYSTEM(m) ? 0 : EXIT_CREDENTIALS, CLD_EXITED); test(m, "exec-credentials-dir-specifier.service", MANAGER_IS_SYSTEM(m) ? 0 : EXIT_CREDENTIALS, CLD_EXITED); } diff --git a/src/test/test-namespace.c b/src/test/test-namespace.c index 436a784e86..25aafc35ca 100644 --- a/src/test/test-namespace.c +++ b/src/test/test-namespace.c @@ -194,7 +194,6 @@ TEST(protect_kernel_logs) { NULL, NULL, NULL, - -EBADF, NULL, 0, NULL, diff --git a/src/test/test-ns.c b/src/test/test-ns.c index 56f3de83b6..77afd2f6b9 100644 --- a/src/test/test-ns.c +++ b/src/test/test-ns.c @@ -96,7 +96,6 @@ int main(int argc, char *argv[]) { tmp_dir, var_tmp_dir, NULL, - -EBADF, NULL, 0, NULL, diff --git a/test/test-execute/exec-load-credential-with-mount-namespace.service b/test/test-execute/exec-load-credential-with-mount-namespace.service deleted file mode 100644 index fd71cf6717..0000000000 --- a/test/test-execute/exec-load-credential-with-mount-namespace.service +++ /dev/null @@ -1,9 +0,0 @@ -# SPDX-License-Identifier: LGPL-2.1-or-later -[Unit] -Description=Test for LoadCredential= - -[Service] -ExecStart=/bin/sh -x -c 'test "$$(cat %d/test-execute.load-credential)" = "foo"' -Type=oneshot -LoadCredential=test-execute.load-credential -PrivateMounts=yes diff --git a/test/test-execute/exec-load-credential-with-seccomp.service b/test/test-execute/exec-load-credential-with-seccomp.service deleted file mode 100644 index 67303f2713..0000000000 --- a/test/test-execute/exec-load-credential-with-seccomp.service +++ /dev/null @@ -1,9 +0,0 @@ -# SPDX-License-Identifier: LGPL-2.1-or-later -[Unit] -Description=Test for LoadCredential= - -[Service] -ExecStart=/bin/sh -x -c 'test "$$(cat %d/test-execute.load-credential)" = "foo"' -Type=oneshot -LoadCredential=test-execute.load-credential -SystemCallFilter=~open_tree move_mount diff --git a/test/test-execute/exec-set-credential-with-mount-namespace.service b/test/test-execute/exec-set-credential-with-mount-namespace.service deleted file mode 100644 index 67d15e5dbb..0000000000 --- a/test/test-execute/exec-set-credential-with-mount-namespace.service +++ /dev/null @@ -1,9 +0,0 @@ -# SPDX-License-Identifier: LGPL-2.1-or-later -[Unit] -Description=Test for SetCredential= - -[Service] -ExecStart=/bin/sh -x -c 'test "$$(cat %d/test-execute.set-credential)" = "hoge"' -Type=oneshot -SetCredential=test-execute.set-credential:hoge -PrivateMounts=yes diff --git a/test/test-execute/exec-set-credential-with-seccomp.service b/test/test-execute/exec-set-credential-with-seccomp.service deleted file mode 100644 index 778777b947..0000000000 --- a/test/test-execute/exec-set-credential-with-seccomp.service +++ /dev/null @@ -1,9 +0,0 @@ -# SPDX-License-Identifier: LGPL-2.1-or-later -[Unit] -Description=Test for SetCredential= - -[Service] -ExecStart=/bin/sh -x -c 'test "$$(cat %d/test-execute.set-credential)" = "hoge"' -Type=oneshot -SetCredential=test-execute.set-credential:hoge -SystemCallFilter=~open_tree move_mount From 43962c30fbfd2e76bf948f00d3562f004154ce16 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 24 Aug 2023 23:52:51 +0900 Subject: [PATCH 2/5] core: rename credential.[ch] -> exec-credential.[ch] Also rename setup_credentials() -> exec_setup_credentials(). Addresses the post-merge review https://github.com/systemd/systemd/pull/28787#pullrequestreview-1592065048. --- src/core/dbus-execute.c | 2 +- src/core/{credential.c => exec-credential.c} | 4 ++-- src/core/{credential.h => exec-credential.h} | 2 +- src/core/execute.c | 4 ++-- src/core/load-fragment.c | 2 +- src/core/meson.build | 2 +- src/core/unit.c | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) rename src/core/{credential.c => exec-credential.c} (99%) rename src/core/{credential.h => exec-credential.h} (98%) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 2a32063ddd..c1050b7030 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -9,13 +9,13 @@ #include "cap-list.h" #include "capability-util.h" #include "cpu-set-util.h" -#include "credential.h" #include "creds-util.h" #include "dbus-execute.h" #include "dbus-util.h" #include "env-util.h" #include "errno-list.h" #include "escape.h" +#include "exec-credential.h" #include "execute.h" #include "fd-util.h" #include "fileio.h" diff --git a/src/core/credential.c b/src/core/exec-credential.c similarity index 99% rename from src/core/credential.c rename to src/core/exec-credential.c index b8b8b4edaa..e7bab891b8 100644 --- a/src/core/credential.c +++ b/src/core/exec-credential.c @@ -3,8 +3,8 @@ #include #include "acl-util.h" -#include "credential.h" #include "creds-util.h" +#include "exec-credential.h" #include "execute.h" #include "fileio.h" #include "glob-util.h" @@ -871,7 +871,7 @@ static int setup_credentials_internal( return 0; } -int setup_credentials( +int exec_setup_credentials( const ExecContext *context, const ExecParameters *params, const char *unit, diff --git a/src/core/credential.h b/src/core/exec-credential.h similarity index 98% rename from src/core/credential.h rename to src/core/exec-credential.h index 54155f515b..db8e4ec3a5 100644 --- a/src/core/credential.h +++ b/src/core/exec-credential.h @@ -40,7 +40,7 @@ bool exec_context_has_credentials(const ExecContext *c); int unit_add_default_credential_dependencies(Unit *u, const ExecContext *c); int exec_context_destroy_credentials(const ExecContext *c, const char *runtime_root, const char *unit); -int setup_credentials( +int exec_setup_credentials( const ExecContext *context, const ExecParameters *params, const char *unit, diff --git a/src/core/execute.c b/src/core/execute.c index 14e196d4c7..81be3a642e 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -49,12 +49,12 @@ #include "chown-recursive.h" #include "constants.h" #include "cpu-set-util.h" -#include "credential.h" #include "data-fd-util.h" #include "env-file.h" #include "env-util.h" #include "errno-list.h" #include "escape.h" +#include "exec-credential.h" #include "execute.h" #include "exit-status.h" #include "fd-util.h" @@ -4427,7 +4427,7 @@ static int exec_child( } if (FLAGS_SET(params->flags, EXEC_WRITE_CREDENTIALS)) { - r = setup_credentials(context, params, unit->id, uid, gid); + r = exec_setup_credentials(context, params, unit->id, uid, gid); if (r < 0) { *exit_status = EXIT_CREDENTIALS; return log_unit_error_errno(unit, r, "Failed to set up credentials: %m"); diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 3c931568a0..cbc6da49af 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -28,11 +28,11 @@ #include "conf-parser.h" #include "core-varlink.h" #include "cpu-set-util.h" -#include "credential.h" #include "creds-util.h" #include "env-util.h" #include "errno-list.h" #include "escape.h" +#include "exec-credential.h" #include "fd-util.h" #include "fileio.h" #include "fs-util.h" diff --git a/src/core/meson.build b/src/core/meson.build index 045ad31a51..12c7d7c6ed 100644 --- a/src/core/meson.build +++ b/src/core/meson.build @@ -11,7 +11,6 @@ libcore_sources = files( 'bpf-socket-bind.c', 'cgroup.c', 'core-varlink.c', - 'credential.c', 'dbus-automount.c', 'dbus-cgroup.c', 'dbus-device.c', @@ -35,6 +34,7 @@ libcore_sources = files( 'dynamic-user.c', 'efi-random.c', 'emergency-action.c', + 'exec-credential.c', 'execute.c', 'generator-setup.c', 'ima-setup.c', diff --git a/src/core/unit.c b/src/core/unit.c index f1b27d30d3..56af1253ad 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -20,12 +20,12 @@ #include "cgroup-util.h" #include "chase.h" #include "core-varlink.h" -#include "credential.h" #include "dbus-unit.h" #include "dbus.h" #include "dropin.h" #include "env-util.h" #include "escape.h" +#include "exec-credential.h" #include "execute.h" #include "fd-util.h" #include "fileio-label.h" From 133e4de23fda6c69d692c5f4f16a69dc80f5893b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 25 Aug 2023 16:11:02 +0900 Subject: [PATCH 3/5] core/exec-credential: introduce exec_context_get_credential_directory() helper function No functional change, just refactoring. --- src/core/exec-credential.c | 19 +++++++++++++++++++ src/core/exec-credential.h | 6 ++++++ src/core/execute.c | 18 ++++++++++-------- 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index e7bab891b8..e69c4a9fa6 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -94,6 +94,25 @@ static int get_credential_directory( return 1; } +int exec_context_get_credential_directory( + const ExecContext *context, + const ExecParameters *params, + const char *unit, + char **ret) { + + assert(context); + assert(params); + assert(unit); + assert(ret); + + if (!exec_context_has_credentials(context)) { + *ret = NULL; + return 0; + } + + return get_credential_directory(params->prefix[EXEC_DIRECTORY_RUNTIME], unit, ret); +} + int unit_add_default_credential_dependencies(Unit *u, const ExecContext *c) { _cleanup_free_ char *p = NULL, *m = NULL; int r; diff --git a/src/core/exec-credential.h b/src/core/exec-credential.h index db8e4ec3a5..9e6f665621 100644 --- a/src/core/exec-credential.h +++ b/src/core/exec-credential.h @@ -37,6 +37,12 @@ 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); +int exec_context_get_credential_directory( + const ExecContext *context, + const ExecParameters *params, + const char *unit, + char **ret); + int unit_add_default_credential_dependencies(Unit *u, const ExecContext *c); int exec_context_destroy_credentials(const ExecContext *c, const char *runtime_root, const char *unit); diff --git a/src/core/execute.c b/src/core/execute.c index 81be3a642e..701e1ead2a 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2041,8 +2041,12 @@ static int build_environment( our_env[n_env++] = x; } - if (exec_context_has_credentials(c) && p->prefix[EXEC_DIRECTORY_RUNTIME]) { - x = strjoin("CREDENTIALS_DIRECTORY=", p->prefix[EXEC_DIRECTORY_RUNTIME], "/credentials/", u->id); + _cleanup_free_ char *creds_dir = NULL; + r = exec_context_get_credential_directory(c, p, u->id, &creds_dir); + if (r < 0) + return r; + if (r > 0) { + x = strjoin("CREDENTIALS_DIRECTORY=", creds_dir); if (!x) return -ENOMEM; @@ -3217,12 +3221,10 @@ static int apply_mount_namespace( if (context->mount_propagation_flag == MS_SHARED) log_unit_debug(u, "shared mount propagation hidden by other fs namespacing unit settings: ignoring"); - if (exec_context_has_credentials(context) && - params->prefix[EXEC_DIRECTORY_RUNTIME] && - FLAGS_SET(params->flags, EXEC_WRITE_CREDENTIALS)) { - creds_path = path_join(params->prefix[EXEC_DIRECTORY_RUNTIME], "credentials", u->id); - if (!creds_path) - return -ENOMEM; + if (FLAGS_SET(params->flags, EXEC_WRITE_CREDENTIALS)) { + r = exec_context_get_credential_directory(context, params, u->id, &creds_path); + if (r < 0) + return r; } if (params->runtime_scope == RUNTIME_SCOPE_SYSTEM) { From 25033cca08485607aacf22ab67baf70e35eae30b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 25 Aug 2023 16:23:14 +0900 Subject: [PATCH 4/5] test-execute: check credentials can be read on ExecStartPost= and friends Prompted by https://github.com/systemd/systemd/pull/28787#issuecomment-1690614202. --- test/test-execute/exec-credentials-dir-specifier.service | 6 ++++++ test/test-execute/exec-load-credential.service | 3 +++ test/test-execute/exec-set-credential.service | 3 +++ 3 files changed, 12 insertions(+) diff --git a/test/test-execute/exec-credentials-dir-specifier.service b/test/test-execute/exec-credentials-dir-specifier.service index 818619acaa..5e71eee246 100644 --- a/test/test-execute/exec-credentials-dir-specifier.service +++ b/test/test-execute/exec-credentials-dir-specifier.service @@ -10,3 +10,9 @@ ExecStart=test %d/very_top_secret = "${CREDENTIALS_DIRECTORY}/very_top_secret" LoadCredential=very_top_secret ExecStart=test %d/very_top_secret = "${CREDENTIALS_DIRECTORY}/very_top_secret" ExecStart=sh -c 'test %d/very_top_secret = "$TOP_SECRET"' +ExecStartPost=test %d/very_top_secret = "${CREDENTIALS_DIRECTORY}/very_top_secret" +ExecStartPost=sh -c 'test %d/very_top_secret = "$TOP_SECRET"' +ExecStop=test %d/very_top_secret = "${CREDENTIALS_DIRECTORY}/very_top_secret" +ExecStop=sh -c 'test %d/very_top_secret = "$TOP_SECRET"' +ExecStopPost=test %d/very_top_secret = "${CREDENTIALS_DIRECTORY}/very_top_secret" +ExecStopPost=sh -c 'test %d/very_top_secret = "$TOP_SECRET"' diff --git a/test/test-execute/exec-load-credential.service b/test/test-execute/exec-load-credential.service index b3e3650075..3a29b6d13d 100644 --- a/test/test-execute/exec-load-credential.service +++ b/test/test-execute/exec-load-credential.service @@ -4,5 +4,8 @@ Description=Test for LoadCredential= [Service] ExecStart=/bin/sh -x -c 'test "$$(cat %d/test-execute.load-credential)" = "foo"' +ExecStartPost=/bin/sh -x -c 'test "$$(cat %d/test-execute.load-credential)" = "foo"' +ExecStop=/bin/sh -x -c 'test "$$(cat %d/test-execute.load-credential)" = "foo"' +ExecStopPost=/bin/sh -x -c 'test "$$(cat %d/test-execute.load-credential)" = "foo"' Type=oneshot LoadCredential=test-execute.load-credential diff --git a/test/test-execute/exec-set-credential.service b/test/test-execute/exec-set-credential.service index 2af236dc4b..9db6c5f3d4 100644 --- a/test/test-execute/exec-set-credential.service +++ b/test/test-execute/exec-set-credential.service @@ -4,5 +4,8 @@ Description=Test for SetCredential= [Service] ExecStart=/bin/sh -x -c 'test "$$(cat %d/test-execute.set-credential)" = "hoge"' +ExecStartPost=/bin/sh -x -c 'test "$$(cat %d/test-execute.set-credential)" = "hoge"' +ExecStop=/bin/sh -x -c 'test "$$(cat %d/test-execute.set-credential)" = "hoge"' +ExecStopPost=/bin/sh -x -c 'test "$$(cat %d/test-execute.set-credential)" = "hoge"' Type=oneshot SetCredential=test-execute.set-credential:hoge From 1e1225614ca1106116dcad9fb37aaeb6106408ab Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 24 Aug 2023 23:41:05 +0900 Subject: [PATCH 5/5] core/credential,mount: re-read /proc/self/mountinfo before invoking umount command When a unit has credentials, stopping the service unmounts the credentials directory. On shutdown, stopping the service and the corresponding mount unit may be done mostly simultaneously, and if we invoke umount command soon after umount() being called on stopping the service, the mount unit will fail. This makes Mount.invalidated_state flag set when umount() is called for a path, and re-read /proc/self/mouninfo before invoking umount command if the flag is set. Fixes #25527. Replaces #26959. --- src/core/exec-credential.c | 11 +++++++---- src/core/exec-credential.h | 2 +- src/core/mount.c | 32 ++++++++++++++++++++++++++++++++ src/core/mount.h | 4 ++++ src/core/unit.c | 2 +- 5 files changed, 45 insertions(+), 6 deletions(-) diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index e69c4a9fa6..2a72f1c396 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -12,6 +12,7 @@ #include "label-util.h" #include "mkdir-label.h" #include "mount-util.h" +#include "mount.h" #include "mountpoint-util.h" #include "process-util.h" #include "random-util.h" @@ -138,19 +139,21 @@ int unit_add_default_credential_dependencies(Unit *u, const ExecContext *c) { return unit_add_dependency_by_name(u, UNIT_AFTER, m, /* add_reference= */ true, UNIT_DEPENDENCY_FILE); } -int exec_context_destroy_credentials(const ExecContext *c, const char *runtime_prefix, const char *unit) { +int exec_context_destroy_credentials(Unit *u) { _cleanup_free_ char *p = NULL; int r; - assert(c); + assert(u); - r = get_credential_directory(runtime_prefix, unit, &p); + r = get_credential_directory(u->manager->prefix[EXEC_DIRECTORY_RUNTIME], u->id, &p); if (r <= 0) return r; /* This is either a tmpfs/ramfs of its own, or a plain directory. Either way, let's first try to * unmount it, and afterwards remove the mount point */ - (void) umount2(p, MNT_DETACH|UMOUNT_NOFOLLOW); + if (umount2(p, MNT_DETACH|UMOUNT_NOFOLLOW) >= 0) + (void) mount_invalidate_state_by_path(u->manager, p); + (void) rm_rf(p, REMOVE_ROOT|REMOVE_CHMOD); return 0; diff --git a/src/core/exec-credential.h b/src/core/exec-credential.h index 9e6f665621..6f836fbd0b 100644 --- a/src/core/exec-credential.h +++ b/src/core/exec-credential.h @@ -45,7 +45,7 @@ int exec_context_get_credential_directory( int unit_add_default_credential_dependencies(Unit *u, const ExecContext *c); -int exec_context_destroy_credentials(const ExecContext *c, const char *runtime_root, const char *unit); +int exec_context_destroy_credentials(Unit *u); int exec_setup_credentials( const ExecContext *context, const ExecParameters *params, diff --git a/src/core/mount.c b/src/core/mount.c index f93bad7074..931075a1ee 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -1284,6 +1284,11 @@ static int mount_stop(Unit *u) { assert(m); + /* When we directly call umount() for a path, then the state of the corresponding mount unit may be + * outdated. Let's re-read mountinfo now and update the state. */ + if (m->invalidated_state) + (void) mount_process_proc_self_mountinfo(u->manager); + switch (m->state) { case MOUNT_UNMOUNTING: @@ -1318,6 +1323,11 @@ static int mount_stop(Unit *u) { mount_enter_signal(m, MOUNT_UNMOUNTING_SIGKILL, MOUNT_SUCCESS); return 0; + case MOUNT_DEAD: + case MOUNT_FAILED: + /* The mount has just been unmounted by somebody else. */ + return 0; + default: assert_not_reached(); } @@ -2067,6 +2077,8 @@ static int mount_process_proc_self_mountinfo(Manager *m) { LIST_FOREACH(units_by_type, u, m->units_by_type[UNIT_MOUNT]) { Mount *mount = MOUNT(u); + mount->invalidated_state = false; + if (!mount_is_mounted(mount)) { /* A mount point is not around right now. It might be gone, or might never have @@ -2160,6 +2172,26 @@ static int mount_dispatch_io(sd_event_source *source, int fd, uint32_t revents, return mount_process_proc_self_mountinfo(m); } +int mount_invalidate_state_by_path(Manager *manager, const char *path) { + _cleanup_free_ char *name = NULL; + Unit *u; + int r; + + assert(manager); + assert(path); + + r = unit_name_from_path(path, ".mount", &name); + if (r < 0) + return log_debug_errno(r, "Failed to generate unit name from path \"%s\", ignoring: %m", path); + + u = manager_get_unit(manager, name); + if (!u) + return -ENOENT; + + MOUNT(u)->invalidated_state = true; + return 0; +} + static void mount_reset_failed(Unit *u) { Mount *m = MOUNT(u); diff --git a/src/core/mount.h b/src/core/mount.h index d6d6d335a4..2560674719 100644 --- a/src/core/mount.h +++ b/src/core/mount.h @@ -49,6 +49,8 @@ struct Mount { MountParameters parameters_proc_self_mountinfo; MountParameters parameters_fragment; + bool invalidated_state:1; /* Set when the 'state' of the mount unit may be outdated, and we need to + * re-read /proc/self/mountinfo. */ bool from_proc_self_mountinfo:1; bool from_fragment:1; @@ -92,6 +94,8 @@ extern const UnitVTable mount_vtable; void mount_fd_event(Manager *m, int events); +int mount_invalidate_state_by_path(Manager *manager, const char *path); + const char* mount_exec_command_to_string(MountExecCommand i) _const_; MountExecCommand mount_exec_command_from_string(const char *s) _pure_; diff --git a/src/core/unit.c b/src/core/unit.c index 56af1253ad..34cbde690b 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5994,7 +5994,7 @@ void unit_destroy_runtime_data(Unit *u, const ExecContext *context) { if (context->runtime_directory_preserve_mode == EXEC_PRESERVE_NO) exec_context_destroy_runtime_directory(context, u->manager->prefix[EXEC_DIRECTORY_RUNTIME]); - exec_context_destroy_credentials(context, u->manager->prefix[EXEC_DIRECTORY_RUNTIME], u->id); + exec_context_destroy_credentials(u); exec_context_destroy_mount_ns_dir(u); }