From 3ed0cd26ea96e4bcc09846fb76851095c4ec85a8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 10:16:42 +0200 Subject: [PATCH 01/19] execute: replace command flag bools by a flags field This way, we can extend it later on in an easier way, and can pass it along nicely. --- src/core/dbus-execute.c | 2 +- src/core/dbus-service.c | 4 ++-- src/core/execute.c | 6 +++--- src/core/execute.h | 8 ++++++-- src/core/load-fragment.c | 5 +++-- src/core/service.c | 6 +++--- src/core/socket.c | 2 +- src/test/test-unit-file.c | 2 +- 8 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index bd4e113bec..45497ca024 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -904,7 +904,7 @@ static int append_exec_command(sd_bus_message *reply, ExecCommand *c) { return r; r = sd_bus_message_append(reply, "bttttuii", - c->ignore, + !!(c->flags & EXEC_COMMAND_IGNORE_FAILURE), c->exec_status.start_timestamp.realtime, c->exec_status.start_timestamp.monotonic, c->exec_status.exit_timestamp.realtime, diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c index 0b81d085fe..05bdc0a748 100644 --- a/src/core/dbus-service.c +++ b/src/core/dbus-service.c @@ -280,7 +280,7 @@ static int bus_service_set_transient_property( c->argv = argv; argv = NULL; - c->ignore = b; + c->flags = b ? EXEC_COMMAND_IGNORE_FAILURE : 0; path_kill_slashes(c->path); exec_command_append_list(&s->exec_command[SERVICE_EXEC_START], c); @@ -319,7 +319,7 @@ static int bus_service_set_transient_property( return -ENOMEM; fprintf(f, "ExecStart=%s@%s %s\n", - c->ignore ? "-" : "", + c->flags & EXEC_COMMAND_IGNORE_FAILURE ? "-" : "", c->path, a); } diff --git a/src/core/execute.c b/src/core/execute.c index 55f9aab4b2..b485509bdf 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2033,7 +2033,7 @@ static int apply_mount_namespace( if (!context->dynamic_user && root_dir) ns_info.ignore_protect_paths = true; - apply_restrictions = (params->flags & EXEC_APPLY_PERMISSIONS) && !command->privileged; + apply_restrictions = (params->flags & EXEC_APPLY_PERMISSIONS) && !(command->flags & EXEC_COMMAND_FULLY_PRIVILEGED); r = setup_namespace(root_dir, root_image, &ns_info, rw, @@ -2647,7 +2647,7 @@ static int exec_child( return r; } - needs_exec_restrictions = (params->flags & EXEC_APPLY_PERMISSIONS) && !command->privileged; + needs_exec_restrictions = (params->flags & EXEC_APPLY_PERMISSIONS) && !(command->flags & EXEC_COMMAND_FULLY_PRIVILEGED); if (needs_exec_restrictions) { if (context->pam_name && username) { @@ -3068,7 +3068,7 @@ int exec_spawn(Unit *unit, error_message), "EXECUTABLE=%s", command->path, NULL); - else if (r == -ENOENT && command->ignore) + else if (r == -ENOENT && (command->flags & EXEC_COMMAND_IGNORE_FAILURE)) log_struct_errno(LOG_INFO, r, "MESSAGE_ID=" SD_MESSAGE_SPAWN_FAILED_STR, LOG_UNIT_ID(unit), diff --git a/src/core/execute.h b/src/core/execute.h index f7fa3176b1..4742f4e6c1 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -88,13 +88,17 @@ struct ExecStatus { int status; /* as in sigingo_t::si_status */ }; +typedef enum ExecCommandFlags { + EXEC_COMMAND_IGNORE_FAILURE = 1, + EXEC_COMMAND_FULLY_PRIVILEGED = 2, +} ExecCommandFlags; + struct ExecCommand { char *path; char **argv; ExecStatus exec_status; + ExecCommandFlags flags; LIST_FIELDS(ExecCommand, command); /* useful for chaining commands */ - bool ignore:1; - bool privileged:1; }; struct ExecRuntime { diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index d9da1a422e..07ad3c5def 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -752,8 +752,9 @@ int config_parse_exec( nce->argv = n; nce->path = path; - nce->ignore = ignore; - nce->privileged = privileged; + nce->flags = + (ignore ? EXEC_COMMAND_IGNORE_FAILURE : 0) | + (privileged ? EXEC_COMMAND_FULLY_PRIVILEGED : 0); exec_command_append_list(e, nce); diff --git a/src/core/service.c b/src/core/service.c index e576f4ba83..4510ac2423 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2913,7 +2913,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { s->main_command->exec_status = s->main_exec_status; - if (s->main_command->ignore) + if (s->main_command->flags & EXEC_COMMAND_IGNORE_FAILURE) f = SERVICE_SUCCESS; } else if (s->exec_command[SERVICE_EXEC_START]) { @@ -2921,7 +2921,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { * ignore the return value if this was * configured for the starter process */ - if (s->exec_command[SERVICE_EXEC_START]->ignore) + if (s->exec_command[SERVICE_EXEC_START]->flags & EXEC_COMMAND_IGNORE_FAILURE) f = SERVICE_SUCCESS; } @@ -3026,7 +3026,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { if (s->control_command) { exec_status_exit(&s->control_command->exec_status, &s->exec_context, pid, code, status); - if (s->control_command->ignore) + if (s->control_command->flags & EXEC_COMMAND_IGNORE_FAILURE) f = SERVICE_SUCCESS; } diff --git a/src/core/socket.c b/src/core/socket.c index ca59a13b66..3ab92f72c3 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2776,7 +2776,7 @@ static void socket_sigchld_event(Unit *u, pid_t pid, int code, int status) { if (s->control_command) { exec_status_exit(&s->control_command->exec_status, &s->exec_context, pid, code, status); - if (s->control_command->ignore) + if (s->control_command->flags & EXEC_COMMAND_IGNORE_FAILURE) f = SOCKET_SUCCESS; } diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index d0c844dddb..aa45727986 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -93,7 +93,7 @@ static void check_execcommand(ExecCommand *c, assert_se(streq_ptr(c->argv[1], argv1)); if (n > 1) assert_se(streq_ptr(c->argv[2], argv2)); - assert_se(c->ignore == ignore); + assert_se(!!(c->flags & EXEC_COMMAND_IGNORE_FAILURE) == ignore); } static void test_config_parse_exec(void) { From af635cf3776a502edab6ebf6723381dd672ebf16 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 10:28:20 +0200 Subject: [PATCH 02/19] execute: let's decouple execute.c a bit from the unit logic Let's try to decouple the execution engine a bit from the Unit/Manager concept, and hence pass one more flag as part of the ExecParameters flags field. --- src/core/execute.c | 6 ++++-- src/core/execute.h | 9 +++++---- src/core/manager.c | 2 ++ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index b485509bdf..5a2c095e85 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -321,6 +321,7 @@ static int connect_journal_socket(int fd, uid_t uid, gid_t gid) { static int connect_logger_as( Unit *unit, const ExecContext *context, + const ExecParameters *params, ExecOutput output, const char *ident, int nfd, @@ -330,6 +331,7 @@ static int connect_logger_as( int fd, r; assert(context); + assert(params); assert(output < _EXEC_OUTPUT_MAX); assert(ident); assert(nfd >= 0); @@ -358,7 +360,7 @@ static int connect_logger_as( "%i\n" "%i\n", context->syslog_identifier ?: ident, - MANAGER_IS_SYSTEM(unit->manager) ? unit->id : "", + params->flags & EXEC_PASS_LOG_UNIT ? unit->id : "", context->syslog_priority, !!context->syslog_level_prefix, output == EXEC_OUTPUT_SYSLOG || output == EXEC_OUTPUT_SYSLOG_AND_CONSOLE, @@ -572,7 +574,7 @@ static int setup_output( case EXEC_OUTPUT_KMSG_AND_CONSOLE: case EXEC_OUTPUT_JOURNAL: case EXEC_OUTPUT_JOURNAL_AND_CONSOLE: - r = connect_logger_as(unit, context, o, ident, fileno, uid, gid); + r = connect_logger_as(unit, context, params, o, ident, fileno, uid, gid); if (r < 0) { log_unit_error_errno(unit, r, "Failed to connect %s to the journal socket, ignoring: %m", fileno == STDOUT_FILENO ? "stdout" : "stderr"); r = open_null_as(O_WRONLY, fileno); diff --git a/src/core/execute.h b/src/core/execute.h index 4742f4e6c1..8b2b80d34e 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -259,12 +259,13 @@ typedef enum ExecFlags { EXEC_APPLY_CHROOT = 1U << 1, EXEC_APPLY_TTY_STDIN = 1U << 2, EXEC_NEW_KEYRING = 1U << 3, + EXEC_PASS_LOG_UNIT = 1U << 4, /* Whether to pass the unit name to the service's journal stream connection */ /* The following are not used by execute.c, but by consumers internally */ - EXEC_PASS_FDS = 1U << 4, - EXEC_IS_CONTROL = 1U << 5, - EXEC_SETENV_RESULT = 1U << 6, - EXEC_SET_WATCHDOG = 1U << 7, + EXEC_PASS_FDS = 1U << 5, + EXEC_IS_CONTROL = 1U << 6, + EXEC_SETENV_RESULT = 1U << 7, + EXEC_SET_WATCHDOG = 1U << 8, } ExecFlags; struct ExecParameters { diff --git a/src/core/manager.c b/src/core/manager.c index 7f9c6164c7..6ddaae8c91 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3422,6 +3422,8 @@ int manager_set_exec_params(Manager *m, ExecParameters *p) { p->cgroup_supported = m->cgroup_supported; p->prefix = m->prefix; + SET_FLAG(p->flags, EXEC_PASS_LOG_UNIT, MANAGER_IS_SYSTEM(m)); + return 0; } From 8679efde21fd26dca385a579da05a4318fbba678 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 10:35:10 +0200 Subject: [PATCH 03/19] execute: add one more ExecFlags flag, for controlling unconditional directory chowning Let's decouple the Manager object from the execution logic a bit more here too, and simply pass along the fact whether we should unconditionally chown the runtime/... directories via the ExecFlags field too. --- src/core/execute.c | 5 ++--- src/core/execute.h | 9 +++++---- src/core/manager.c | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 5a2c095e85..cefe9b11c2 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1843,7 +1843,6 @@ static int setup_exec_directory( const ExecParameters *params, uid_t uid, gid_t gid, - bool manager_is_system, ExecDirectoryType type, int *exit_status) { @@ -1865,7 +1864,7 @@ static int setup_exec_directory( if (!params->prefix[type]) return 0; - if (manager_is_system) { + if (params->flags & EXEC_CHOWN_DIRECTORIES) { if (!uid_is_valid(uid)) uid = 0; if (!gid_is_valid(gid)) @@ -2601,7 +2600,7 @@ static int exec_child( } for (dt = 0; dt < _EXEC_DIRECTORY_MAX; dt++) { - r = setup_exec_directory(context, params, uid, gid, MANAGER_IS_SYSTEM(unit->manager), dt, exit_status); + r = setup_exec_directory(context, params, uid, gid, dt, exit_status); if (r < 0) return r; } diff --git a/src/core/execute.h b/src/core/execute.h index 8b2b80d34e..1560aff7fd 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -260,12 +260,13 @@ typedef enum ExecFlags { EXEC_APPLY_TTY_STDIN = 1U << 2, EXEC_NEW_KEYRING = 1U << 3, EXEC_PASS_LOG_UNIT = 1U << 4, /* Whether to pass the unit name to the service's journal stream connection */ + EXEC_CHOWN_DIRECTORIES = 1U << 5, /* chown() the runtime/state/cache/log directories to the user we run as, under all conditions */ /* The following are not used by execute.c, but by consumers internally */ - EXEC_PASS_FDS = 1U << 5, - EXEC_IS_CONTROL = 1U << 6, - EXEC_SETENV_RESULT = 1U << 7, - EXEC_SET_WATCHDOG = 1U << 8, + EXEC_PASS_FDS = 1U << 6, + EXEC_IS_CONTROL = 1U << 7, + EXEC_SETENV_RESULT = 1U << 8, + EXEC_SET_WATCHDOG = 1U << 9, } ExecFlags; struct ExecParameters { diff --git a/src/core/manager.c b/src/core/manager.c index 6ddaae8c91..91cb3ab43f 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3422,7 +3422,7 @@ int manager_set_exec_params(Manager *m, ExecParameters *p) { p->cgroup_supported = m->cgroup_supported; p->prefix = m->prefix; - SET_FLAG(p->flags, EXEC_PASS_LOG_UNIT, MANAGER_IS_SYSTEM(m)); + SET_FLAG(p->flags, EXEC_PASS_LOG_UNIT|EXEC_CHOWN_DIRECTORIES, MANAGER_IS_SYSTEM(m)); return 0; } From c71b2eb77e206ad622c9b2ab1807fb99725d572c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 10:36:33 +0200 Subject: [PATCH 04/19] core: don't chown() the configuration directory The configuration directory is commonly not owned by a service, but remains root-owned, hence don't change the owner automatically for it. --- src/core/execute.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/core/execute.c b/src/core/execute.c index cefe9b11c2..2453cb298c 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1888,6 +1888,11 @@ static int setup_exec_directory( if (r < 0) goto fail; + /* Don't change the owner of the configuration directory, as in the common case it is not written to by + * a service, and shall not be writable. */ + if (type == EXEC_DIRECTORY_CONFIGURATION) + continue; + r = chmod_and_chown(p, context->directories[type].mode, uid, gid); if (r < 0) goto fail; From 5bf7569cf885b0af940f5c3945b3f7cb13bd666f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 10:37:17 +0200 Subject: [PATCH 05/19] service: let's set EXEC_NEW_KEYRING through SET_FLAG() Not that it really matters, but it matches how we set the flags in manager_set_exec_params() too. --- src/core/service.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/service.c b/src/core/service.c index 4510ac2423..04ec3ac90e 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1358,7 +1358,8 @@ static int service_spawn( } else path = UNIT(s)->cgroup_path; - exec_params.flags |= MANAGER_IS_SYSTEM(UNIT(s)->manager) ? EXEC_NEW_KEYRING : 0; + /* System services should get a new keyring by default. */ + SET_FLAG(exec_params.flags, EXEC_NEW_KEYRING, MANAGER_IS_SYSTEM(UNIT(s)->manager)); exec_params.argv = c->argv; exec_params.environment = final_env; exec_params.fds = fds; From ac6479781e0ceae33d32f3040dbbaea797884089 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 10:43:04 +0200 Subject: [PATCH 06/19] execute: also control the SYSTEMD_NSS_BYPASS_BUS through an ExecFlags field Also, correct the logic while we are at it: the variable is only required for system services, not user services. --- src/core/execute.c | 2 +- src/core/execute.h | 9 +++++---- src/core/service.c | 5 +++++ 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 2453cb298c..9498ebe098 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1536,7 +1536,7 @@ static int build_environment( /* If this is D-Bus, tell the nss-systemd module, since it relies on being able to use D-Bus look up dynamic * users via PID 1, possibly dead-locking the dbus daemon. This way it will not use D-Bus to resolve names, but * check the database directly. */ - if (unit_has_name(u, SPECIAL_DBUS_SERVICE)) { + if (p->flags & EXEC_NSS_BYPASS_BUS) { x = strdup("SYSTEMD_NSS_BYPASS_BUS=1"); if (!x) return -ENOMEM; diff --git a/src/core/execute.h b/src/core/execute.h index 1560aff7fd..14a3bdde1f 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -261,12 +261,13 @@ typedef enum ExecFlags { EXEC_NEW_KEYRING = 1U << 3, EXEC_PASS_LOG_UNIT = 1U << 4, /* Whether to pass the unit name to the service's journal stream connection */ EXEC_CHOWN_DIRECTORIES = 1U << 5, /* chown() the runtime/state/cache/log directories to the user we run as, under all conditions */ + EXEC_NSS_BYPASS_BUS = 1U << 6, /* Set the SYSTEMD_NSS_BYPASS_BUS environment variable, to disable nss-systemd for dbus */ /* The following are not used by execute.c, but by consumers internally */ - EXEC_PASS_FDS = 1U << 6, - EXEC_IS_CONTROL = 1U << 7, - EXEC_SETENV_RESULT = 1U << 8, - EXEC_SET_WATCHDOG = 1U << 9, + EXEC_PASS_FDS = 1U << 7, + EXEC_IS_CONTROL = 1U << 8, + EXEC_SETENV_RESULT = 1U << 9, + EXEC_SET_WATCHDOG = 1U << 10, } ExecFlags; struct ExecParameters { diff --git a/src/core/service.c b/src/core/service.c index 04ec3ac90e..01cc0a5d2a 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1360,6 +1360,11 @@ static int service_spawn( /* System services should get a new keyring by default. */ SET_FLAG(exec_params.flags, EXEC_NEW_KEYRING, MANAGER_IS_SYSTEM(UNIT(s)->manager)); + + /* System D-Bus needs nss-systemd disabled, so that we don't deadlock */ + SET_FLAG(exec_params.flags, EXEC_NSS_BYPASS_BUS, + MANAGER_IS_SYSTEM(UNIT(s)->manager) && unit_has_name(UNIT(s), SPECIAL_DBUS_SERVICE)); + exec_params.argv = c->argv; exec_params.environment = final_env; exec_params.fds = fds; From 584b8688d18244a40a57160627580ab33ff976a3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 10:51:18 +0200 Subject: [PATCH 07/19] execute: also fold the cgroup delegate bit into ExecFlags --- src/core/execute.c | 2 +- src/core/execute.h | 10 +++++----- src/core/mount.c | 2 +- src/core/service.c | 3 ++- src/core/socket.c | 3 ++- src/core/swap.c | 2 +- 6 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 9498ebe098..194fb4b4c0 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2589,7 +2589,7 @@ static int exec_child( /* If delegation is enabled we'll pass ownership of the cgroup * (but only in systemd's own controller hierarchy!) to the * user of the new process. */ - if (params->cgroup_path && context->user && params->cgroup_delegate) { + if (params->cgroup_path && context->user && (params->flags & EXEC_CGROUP_DELEGATE)) { r = cg_set_task_access(SYSTEMD_CGROUP_CONTROLLER, params->cgroup_path, 0644, uid, gid); if (r < 0) { *exit_status = EXIT_CGROUP; diff --git a/src/core/execute.h b/src/core/execute.h index 14a3bdde1f..69cb0e5646 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -262,12 +262,13 @@ typedef enum ExecFlags { EXEC_PASS_LOG_UNIT = 1U << 4, /* Whether to pass the unit name to the service's journal stream connection */ EXEC_CHOWN_DIRECTORIES = 1U << 5, /* chown() the runtime/state/cache/log directories to the user we run as, under all conditions */ EXEC_NSS_BYPASS_BUS = 1U << 6, /* Set the SYSTEMD_NSS_BYPASS_BUS environment variable, to disable nss-systemd for dbus */ + EXEC_CGROUP_DELEGATE = 1U << 7, /* The following are not used by execute.c, but by consumers internally */ - EXEC_PASS_FDS = 1U << 7, - EXEC_IS_CONTROL = 1U << 8, - EXEC_SETENV_RESULT = 1U << 9, - EXEC_SET_WATCHDOG = 1U << 10, + EXEC_PASS_FDS = 1U << 8, + EXEC_IS_CONTROL = 1U << 9, + EXEC_SETENV_RESULT = 1U << 10, + EXEC_SET_WATCHDOG = 1U << 11, } ExecFlags; struct ExecParameters { @@ -282,7 +283,6 @@ struct ExecParameters { ExecFlags flags; bool selinux_context_net:1; - bool cgroup_delegate:1; CGroupMask cgroup_supported; const char *cgroup_path; diff --git a/src/core/mount.c b/src/core/mount.c index 25f27909dd..0114efb648 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -775,7 +775,7 @@ static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) { return r; exec_params.cgroup_path = UNIT(m)->cgroup_path; - exec_params.cgroup_delegate = m->cgroup_context.delegate; + SET_FLAG(exec_params.flags, EXEC_CGROUP_DELEGATE, m->cgroup_context.delegate); r = exec_spawn(UNIT(m), c, diff --git a/src/core/service.c b/src/core/service.c index 01cc0a5d2a..c35a41bbe0 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1365,6 +1365,8 @@ static int service_spawn( SET_FLAG(exec_params.flags, EXEC_NSS_BYPASS_BUS, MANAGER_IS_SYSTEM(UNIT(s)->manager) && unit_has_name(UNIT(s), SPECIAL_DBUS_SERVICE)); + SET_FLAG(exec_params.flags, EXEC_CGROUP_DELEGATE, s->cgroup_context.delegate); + exec_params.argv = c->argv; exec_params.environment = final_env; exec_params.fds = fds; @@ -1372,7 +1374,6 @@ static int service_spawn( exec_params.n_storage_fds = n_storage_fds; exec_params.n_socket_fds = n_socket_fds; exec_params.cgroup_path = path; - exec_params.cgroup_delegate = s->cgroup_context.delegate; exec_params.watchdog_usec = s->watchdog_usec; exec_params.selinux_context_net = s->socket_fd_selinux_context_net; if (s->type == SERVICE_IDLE) diff --git a/src/core/socket.c b/src/core/socket.c index 3ab92f72c3..871f09825c 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1796,7 +1796,8 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { exec_params.argv = c->argv; exec_params.cgroup_path = UNIT(s)->cgroup_path; - exec_params.cgroup_delegate = s->cgroup_context.delegate; + + SET_FLAG(exec_params.flags, EXEC_CGROUP_DELEGATE, s->cgroup_context.delegate); r = exec_spawn(UNIT(s), c, diff --git a/src/core/swap.c b/src/core/swap.c index e839c26141..7bea2e8505 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -641,7 +641,7 @@ static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) { goto fail; exec_params.cgroup_path = UNIT(s)->cgroup_path; - exec_params.cgroup_delegate = s->cgroup_context.delegate; + SET_FLAG(exec_params.flags, EXEC_CGROUP_DELEGATE, s->cgroup_context.delegate); r = exec_spawn(UNIT(s), c, From 19bbdd985ec517926177b677b6f6cee31e0a8ab8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 10:53:46 +0200 Subject: [PATCH 08/19] core: manager_set_exec_params() cannot fail, hence make it void Let's simplify things a bit. --- src/core/manager.c | 4 +--- src/core/manager.h | 2 +- src/core/mount.c | 4 +--- src/core/service.c | 4 +--- src/core/socket.c | 4 +--- src/core/swap.c | 4 +--- 6 files changed, 6 insertions(+), 16 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 91cb3ab43f..4d215d5692 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3413,7 +3413,7 @@ Set *manager_get_units_requiring_mounts_for(Manager *m, const char *path) { return hashmap_get(m->units_requiring_mounts_for, streq(p, "/") ? "" : p); } -int manager_set_exec_params(Manager *m, ExecParameters *p) { +void manager_set_exec_params(Manager *m, ExecParameters *p) { assert(m); assert(p); @@ -3423,8 +3423,6 @@ int manager_set_exec_params(Manager *m, ExecParameters *p) { p->prefix = m->prefix; SET_FLAG(p->flags, EXEC_PASS_LOG_UNIT|EXEC_CHOWN_DIRECTORIES, MANAGER_IS_SYSTEM(m)); - - return 0; } int manager_update_failed_units(Manager *m, Unit *u, bool failed) { diff --git a/src/core/manager.h b/src/core/manager.h index 5146a795f1..8d621da4e1 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -384,7 +384,7 @@ void manager_flip_auto_status(Manager *m, bool enable); Set *manager_get_units_requiring_mounts_for(Manager *m, const char *path); -int manager_set_exec_params(Manager *m, ExecParameters *p); +void manager_set_exec_params(Manager *m, ExecParameters *p); ManagerState manager_state(Manager *m); diff --git a/src/core/mount.c b/src/core/mount.c index 0114efb648..6b045939d6 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -770,9 +770,7 @@ static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) { if (r < 0) return r; - r = manager_set_exec_params(UNIT(m)->manager, &exec_params); - if (r < 0) - return r; + manager_set_exec_params(UNIT(m)->manager, &exec_params); exec_params.cgroup_path = UNIT(m)->cgroup_path; SET_FLAG(exec_params.flags, EXEC_CGROUP_DELEGATE, m->cgroup_context.delegate); diff --git a/src/core/service.c b/src/core/service.c index c35a41bbe0..a97ebb1642 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1344,9 +1344,7 @@ static int service_spawn( } } - r = manager_set_exec_params(UNIT(s)->manager, &exec_params); - if (r < 0) - return r; + manager_set_exec_params(UNIT(s)->manager, &exec_params); final_env = strv_env_merge(2, exec_params.environment, our_env, NULL); if (!final_env) diff --git a/src/core/socket.c b/src/core/socket.c index 871f09825c..078c20fc91 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1790,9 +1790,7 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { if (r < 0) return r; - r = manager_set_exec_params(UNIT(s)->manager, &exec_params); - if (r < 0) - return r; + manager_set_exec_params(UNIT(s)->manager, &exec_params); exec_params.argv = c->argv; exec_params.cgroup_path = UNIT(s)->cgroup_path; diff --git a/src/core/swap.c b/src/core/swap.c index 7bea2e8505..255076839a 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -636,9 +636,7 @@ static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) { if (r < 0) goto fail; - r = manager_set_exec_params(UNIT(s)->manager, &exec_params); - if (r < 0) - goto fail; + manager_set_exec_params(UNIT(s)->manager, &exec_params); exec_params.cgroup_path = UNIT(s)->cgroup_path; SET_FLAG(exec_params.flags, EXEC_CGROUP_DELEGATE, s->cgroup_context.delegate); From f0d477979e0d759c4160df010ec9c44863c7032a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 11:02:30 +0200 Subject: [PATCH 09/19] core: introduce unit_set_exec_params() The new unit_set_exec_params() call is to units what manager_set_exec_params() is to the manager object: it initializes the various fields from the relevant generic properties set. --- src/core/mount.c | 4 +--- src/core/service.c | 12 ++++-------- src/core/socket.c | 4 +--- src/core/swap.c | 4 +--- src/core/unit.c | 12 ++++++++++++ src/core/unit.h | 2 ++ 6 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 6b045939d6..d0b0cfe4b6 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -771,9 +771,7 @@ static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) { return r; manager_set_exec_params(UNIT(m)->manager, &exec_params); - - exec_params.cgroup_path = UNIT(m)->cgroup_path; - SET_FLAG(exec_params.flags, EXEC_CGROUP_DELEGATE, m->cgroup_context.delegate); + unit_set_exec_params(UNIT(m), &exec_params); r = exec_spawn(UNIT(m), c, diff --git a/src/core/service.c b/src/core/service.c index a97ebb1642..2e94c4aea5 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1218,7 +1218,6 @@ static int service_spawn( _cleanup_strv_free_ char **final_env = NULL, **our_env = NULL, **fd_names = NULL; _cleanup_free_ int *fds = NULL; unsigned n_storage_fds = 0, n_socket_fds = 0, n_env = 0; - const char *path; pid_t pid; ExecParameters exec_params = { @@ -1345,16 +1344,16 @@ static int service_spawn( } manager_set_exec_params(UNIT(s)->manager, &exec_params); + unit_set_exec_params(UNIT(s), &exec_params); final_env = strv_env_merge(2, exec_params.environment, our_env, NULL); if (!final_env) return -ENOMEM; if ((flags & EXEC_IS_CONTROL) && UNIT(s)->cgroup_path) { - path = strjoina(UNIT(s)->cgroup_path, "/control"); - (void) cg_create(SYSTEMD_CGROUP_CONTROLLER, path); - } else - path = UNIT(s)->cgroup_path; + exec_params.cgroup_path = strjoina(UNIT(s)->cgroup_path, "/control"); + (void) cg_create(SYSTEMD_CGROUP_CONTROLLER, exec_params.cgroup_path); + } /* System services should get a new keyring by default. */ SET_FLAG(exec_params.flags, EXEC_NEW_KEYRING, MANAGER_IS_SYSTEM(UNIT(s)->manager)); @@ -1363,15 +1362,12 @@ static int service_spawn( SET_FLAG(exec_params.flags, EXEC_NSS_BYPASS_BUS, MANAGER_IS_SYSTEM(UNIT(s)->manager) && unit_has_name(UNIT(s), SPECIAL_DBUS_SERVICE)); - SET_FLAG(exec_params.flags, EXEC_CGROUP_DELEGATE, s->cgroup_context.delegate); - exec_params.argv = c->argv; exec_params.environment = final_env; exec_params.fds = fds; exec_params.fd_names = fd_names; exec_params.n_storage_fds = n_storage_fds; exec_params.n_socket_fds = n_socket_fds; - exec_params.cgroup_path = path; exec_params.watchdog_usec = s->watchdog_usec; exec_params.selinux_context_net = s->socket_fd_selinux_context_net; if (s->type == SERVICE_IDLE) diff --git a/src/core/socket.c b/src/core/socket.c index 078c20fc91..aed78613f6 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1791,11 +1791,9 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { return r; manager_set_exec_params(UNIT(s)->manager, &exec_params); + unit_set_exec_params(UNIT(s), &exec_params); exec_params.argv = c->argv; - exec_params.cgroup_path = UNIT(s)->cgroup_path; - - SET_FLAG(exec_params.flags, EXEC_CGROUP_DELEGATE, s->cgroup_context.delegate); r = exec_spawn(UNIT(s), c, diff --git a/src/core/swap.c b/src/core/swap.c index 255076839a..86995a45d0 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -637,9 +637,7 @@ static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) { goto fail; manager_set_exec_params(UNIT(s)->manager, &exec_params); - - exec_params.cgroup_path = UNIT(s)->cgroup_path; - SET_FLAG(exec_params.flags, EXEC_CGROUP_DELEGATE, s->cgroup_context.delegate); + unit_set_exec_params(UNIT(s), &exec_params); r = exec_spawn(UNIT(s), c, diff --git a/src/core/unit.c b/src/core/unit.c index a42c667754..c6d96c69dc 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4399,3 +4399,15 @@ int unit_acquire_invocation_id(Unit *u) { return 0; } + +void unit_set_exec_params(Unit *s, ExecParameters *p) { + CGroupContext *c; + + assert(s); + assert(s); + + p->cgroup_path = s->cgroup_path; + + c = unit_get_cgroup_context(s); + SET_FLAG(p->flags, EXEC_CGROUP_DELEGATE, c && c->delegate); +} diff --git a/src/core/unit.h b/src/core/unit.h index a384f264ee..4d9751a406 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -659,6 +659,8 @@ int unit_acquire_invocation_id(Unit *u); bool unit_shall_confirm_spawn(Unit *u); +void unit_set_exec_params(Unit *s, ExecParameters *p); + /* Macros which append UNIT= or USER_UNIT= to the message */ #define log_unit_full(unit, level, error, ...) \ From 1703fa41a73ff2225ca38e84df522ada49d05753 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 11:30:44 +0200 Subject: [PATCH 10/19] =?UTF-8?q?core:=20rename=20EXEC=5FAPPLY=5FPERMISSIO?= =?UTF-8?q?NS=20=E2=86=92=20EXEC=5FAPPLY=5FSANDBOXING?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Permissions" was a bit of a misnomer, as it suggests that UNIX file permission bits are adjusted, which aren't really changed here. Instead, this is about UNIX credentials such as users or groups, as well as namespacing, hence let's use a more generic term here, without any misleading reference to UNIX file permissions: "sandboxing", which shall refer to all kinds of sandboxing technologies, including UID/GID dropping, selinux relabelling, namespacing, seccomp, and so on. --- src/core/execute.c | 16 ++++++++-------- src/core/execute.h | 2 +- src/core/mount.c | 2 +- src/core/service.c | 18 +++++++++--------- src/core/socket.c | 2 +- src/core/swap.c | 2 +- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 194fb4b4c0..0f20765f22 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2039,7 +2039,7 @@ static int apply_mount_namespace( if (!context->dynamic_user && root_dir) ns_info.ignore_protect_paths = true; - apply_restrictions = (params->flags & EXEC_APPLY_PERMISSIONS) && !(command->flags & EXEC_COMMAND_FULLY_PRIVILEGED); + apply_restrictions = (params->flags & EXEC_APPLY_SANDBOXING) && !(command->flags & EXEC_COMMAND_FULLY_PRIVILEGED); r = setup_namespace(root_dir, root_image, &ns_info, rw, @@ -2302,7 +2302,7 @@ static int exec_child( const char *home = NULL, *shell = NULL; dev_t journal_stream_dev = 0; ino_t journal_stream_ino = 0; - bool needs_exec_restrictions, needs_mount_namespace; + bool needs_sandboxing, needs_mount_namespace; #ifdef HAVE_SELINUX bool needs_selinux = false; #endif @@ -2653,9 +2653,9 @@ static int exec_child( return r; } - needs_exec_restrictions = (params->flags & EXEC_APPLY_PERMISSIONS) && !(command->flags & EXEC_COMMAND_FULLY_PRIVILEGED); + needs_sandboxing = (params->flags & EXEC_APPLY_SANDBOXING) && !(command->flags & EXEC_COMMAND_FULLY_PRIVILEGED); - if (needs_exec_restrictions) { + if (needs_sandboxing) { if (context->pam_name && username) { r = setup_pam(context->pam_name, username, uid, gid, context->tty_path, &accum_env, fds, n_fds); if (r < 0) { @@ -2705,7 +2705,7 @@ static int exec_child( return r; /* Drop groups as early as possbile */ - if (needs_exec_restrictions) { + if (needs_sandboxing) { r = enforce_groups(context, gid, supplementary_gids, ngids); if (r < 0) { *exit_status = EXIT_GROUP; @@ -2714,7 +2714,7 @@ static int exec_child( } #ifdef HAVE_SELINUX - if (needs_exec_restrictions && needs_selinux && params->selinux_context_net && socket_fd >= 0) { + if (needs_sandboxing && needs_selinux && params->selinux_context_net && socket_fd >= 0) { r = mac_selinux_get_child_mls_label(socket_fd, command->path, context->selinux_context, &mac_selinux_context_net); if (r < 0) { *exit_status = EXIT_SELINUX_CONTEXT; @@ -2723,7 +2723,7 @@ static int exec_child( } #endif - if ((params->flags & EXEC_APPLY_PERMISSIONS) && context->private_users) { + if ((params->flags & EXEC_APPLY_SANDBOXING) && context->private_users) { r = setup_private_users(uid, gid); if (r < 0) { *exit_status = EXIT_USER; @@ -2747,7 +2747,7 @@ static int exec_child( return r; } - if (needs_exec_restrictions) { + if (needs_sandboxing) { int secure_bits = context->secure_bits; diff --git a/src/core/execute.h b/src/core/execute.h index 69cb0e5646..0c5811c5bd 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -255,7 +255,7 @@ static inline bool exec_context_restrict_namespaces_set(const ExecContext *c) { } typedef enum ExecFlags { - EXEC_APPLY_PERMISSIONS = 1U << 0, + EXEC_APPLY_SANDBOXING = 1U << 0, EXEC_APPLY_CHROOT = 1U << 1, EXEC_APPLY_TTY_STDIN = 1U << 2, EXEC_NEW_KEYRING = 1U << 3, diff --git a/src/core/mount.c b/src/core/mount.c index d0b0cfe4b6..c3805ee055 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -742,7 +742,7 @@ static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) { pid_t pid; int r; ExecParameters exec_params = { - .flags = EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN, + .flags = EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN, .stdin_fd = -1, .stdout_fd = -1, .stderr_fd = -1, diff --git a/src/core/service.c b/src/core/service.c index 2e94c4aea5..a9c8a2ee15 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1236,7 +1236,7 @@ static int service_spawn( if (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_PERMISSIONS; + exec_params.flags &= ~EXEC_APPLY_SANDBOXING; if (s->root_directory_start_only) exec_params.flags &= ~EXEC_APPLY_CHROOT; } @@ -1570,7 +1570,7 @@ static void service_enter_stop_post(Service *s, ServiceResult f) { r = service_spawn(s, s->control_command, s->timeout_stop_usec, - EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_IS_CONTROL|EXEC_SETENV_RESULT, + EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_IS_CONTROL|EXEC_SETENV_RESULT, &s->control_pid); if (r < 0) goto fail; @@ -1681,7 +1681,7 @@ static void service_enter_stop(Service *s, ServiceResult f) { r = service_spawn(s, s->control_command, s->timeout_stop_usec, - EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL|EXEC_SETENV_RESULT, + EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL|EXEC_SETENV_RESULT, &s->control_pid); if (r < 0) goto fail; @@ -1760,7 +1760,7 @@ static void service_enter_start_post(Service *s) { r = service_spawn(s, s->control_command, s->timeout_start_usec, - EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL, + EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL, &s->control_pid); if (r < 0) goto fail; @@ -1838,7 +1838,7 @@ static void service_enter_start(Service *s) { r = service_spawn(s, c, timeout, - EXEC_PASS_FDS|EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_SET_WATCHDOG, + EXEC_PASS_FDS|EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_SET_WATCHDOG, &pid); if (r < 0) goto fail; @@ -1897,7 +1897,7 @@ static void service_enter_start_pre(Service *s) { r = service_spawn(s, s->control_command, s->timeout_start_usec, - EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL|EXEC_APPLY_TTY_STDIN, + EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL|EXEC_APPLY_TTY_STDIN, &s->control_pid); if (r < 0) goto fail; @@ -1995,7 +1995,7 @@ static void service_enter_reload(Service *s) { r = service_spawn(s, s->control_command, s->timeout_start_usec, - EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL, + EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL, &s->control_pid); if (r < 0) goto fail; @@ -2033,7 +2033,7 @@ static void service_run_next_control(Service *s) { r = service_spawn(s, s->control_command, timeout, - EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL| + EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL| (IN_SET(s->control_command_id, 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), &s->control_pid); @@ -2071,7 +2071,7 @@ static void service_run_next_main(Service *s) { r = service_spawn(s, s->main_command, s->timeout_start_usec, - EXEC_PASS_FDS|EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_SET_WATCHDOG, + EXEC_PASS_FDS|EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_SET_WATCHDOG, &pid); if (r < 0) goto fail; diff --git a/src/core/socket.c b/src/core/socket.c index aed78613f6..9d8367e90b 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1762,7 +1762,7 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { pid_t pid; int r; ExecParameters exec_params = { - .flags = EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN, + .flags = EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN, .stdin_fd = -1, .stdout_fd = -1, .stderr_fd = -1, diff --git a/src/core/swap.c b/src/core/swap.c index 86995a45d0..9553ee16a8 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -608,7 +608,7 @@ static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) { pid_t pid; int r; ExecParameters exec_params = { - .flags = EXEC_APPLY_PERMISSIONS|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN, + .flags = EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN, .stdin_fd = -1, .stdout_fd = -1, .stderr_fd = -1, From 937ccce94cf2204a7bc78749d7b32c8ecbc71022 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 11:44:37 +0200 Subject: [PATCH 11/19] execute: simplify needs_sandboxing checking Let's merge three if blocks that shall only run when sandboxing is applied into one. Note that this changes behaviour in one corner case: PrivateUsers=1 is now honours both PermissionsStartOnly= and the "+" modifier in ExecStart=, and not just the former, as before. This was an oversight, so let's fix this now, at a point in time the option isn't used much yet. --- src/core/execute.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 0f20765f22..96bf799f66 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2711,23 +2711,23 @@ static int exec_child( *exit_status = EXIT_GROUP; return r; } - } #ifdef HAVE_SELINUX - if (needs_sandboxing && needs_selinux && params->selinux_context_net && socket_fd >= 0) { - r = mac_selinux_get_child_mls_label(socket_fd, command->path, context->selinux_context, &mac_selinux_context_net); - if (r < 0) { - *exit_status = EXIT_SELINUX_CONTEXT; - return r; + if (needs_selinux && params->selinux_context_net && socket_fd >= 0) { + r = mac_selinux_get_child_mls_label(socket_fd, command->path, context->selinux_context, &mac_selinux_context_net); + if (r < 0) { + *exit_status = EXIT_SELINUX_CONTEXT; + return r; + } } - } #endif - if ((params->flags & EXEC_APPLY_SANDBOXING) && context->private_users) { - r = setup_private_users(uid, gid); - if (r < 0) { - *exit_status = EXIT_USER; - return r; + if (context->private_users) { + r = setup_private_users(uid, gid); + if (r < 0) { + *exit_status = EXIT_USER; + return r; + } } } From 9f6444eb92586d9e8c2f690053ce68df957be810 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Aug 2017 11:52:36 +0200 Subject: [PATCH 12/19] execute: make use of IN_SET() where we can --- src/core/execute.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 96bf799f66..8a394e2f47 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -3582,18 +3582,20 @@ void exec_context_dump(ExecContext *c, FILE* f, const char *prefix) { prefix, yes_no(c->tty_vhangup), prefix, yes_no(c->tty_vt_disallocate)); - if (c->std_output == EXEC_OUTPUT_SYSLOG || - c->std_output == EXEC_OUTPUT_KMSG || - c->std_output == EXEC_OUTPUT_JOURNAL || - c->std_output == EXEC_OUTPUT_SYSLOG_AND_CONSOLE || - c->std_output == EXEC_OUTPUT_KMSG_AND_CONSOLE || - c->std_output == EXEC_OUTPUT_JOURNAL_AND_CONSOLE || - c->std_error == EXEC_OUTPUT_SYSLOG || - c->std_error == EXEC_OUTPUT_KMSG || - c->std_error == EXEC_OUTPUT_JOURNAL || - c->std_error == EXEC_OUTPUT_SYSLOG_AND_CONSOLE || - c->std_error == EXEC_OUTPUT_KMSG_AND_CONSOLE || - c->std_error == EXEC_OUTPUT_JOURNAL_AND_CONSOLE) { + if (IN_SET(c->std_output, + EXEC_OUTPUT_SYSLOG, + EXEC_OUTPUT_KMSG, + EXEC_OUTPUT_JOURNAL, + EXEC_OUTPUT_SYSLOG_AND_CONSOLE, + EXEC_OUTPUT_KMSG_AND_CONSOLE, + EXEC_OUTPUT_JOURNAL_AND_CONSOLE) || + IN_SET(c->std_error, + EXEC_OUTPUT_SYSLOG, + EXEC_OUTPUT_KMSG, + EXEC_OUTPUT_JOURNAL, + EXEC_OUTPUT_SYSLOG_AND_CONSOLE, + EXEC_OUTPUT_KMSG_AND_CONSOLE, + EXEC_OUTPUT_JOURNAL_AND_CONSOLE)) { _cleanup_free_ char *fac_str = NULL, *lvl_str = NULL; From 43b1f7092d2016699d2f2cc4de0dacd654e0be0e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 8 Aug 2017 19:49:04 +0200 Subject: [PATCH 13/19] =?UTF-8?q?execute:=20needs=5F{selinux,apparmor,smac?= =?UTF-8?q?k}=20=E2=86=92=20use=5F{selinux,apparmor,smack}?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These booleans simply store whether selinux/apparmor/smack are supposed ot be used, and chache the various mac_xyz_use() calls before we transition into the namespace, hence let's use the same verb for the variables and the functions: "use" --- src/core/execute.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 8a394e2f47..6caf13baf6 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2304,13 +2304,13 @@ static int exec_child( ino_t journal_stream_ino = 0; bool needs_sandboxing, needs_mount_namespace; #ifdef HAVE_SELINUX - bool needs_selinux = false; + bool use_selinux = false; #endif #ifdef HAVE_SMACK - bool needs_smack = false; + bool use_smack = false; #endif #ifdef HAVE_APPARMOR - bool needs_apparmor = false; + bool use_apparmor = false; #endif uid_t uid = UID_INVALID; gid_t gid = GID_INVALID; @@ -2669,15 +2669,15 @@ static int exec_child( * impacting our own code paths. */ #ifdef HAVE_SELINUX - needs_selinux = mac_selinux_use(); + use_selinux = mac_selinux_use(); #endif #ifdef HAVE_SMACK - needs_smack = mac_smack_use(); + use_smack = mac_smack_use(); #endif #ifdef HAVE_APPARMOR - needs_apparmor = context->apparmor_profile && mac_apparmor_use(); + use_apparmor = mac_apparmor_use(); #endif } @@ -2713,7 +2713,7 @@ static int exec_child( } #ifdef HAVE_SELINUX - if (needs_selinux && params->selinux_context_net && socket_fd >= 0) { + if (use_selinux && params->selinux_context_net && socket_fd >= 0) { r = mac_selinux_get_child_mls_label(socket_fd, command->path, context->selinux_context, &mac_selinux_context_net); if (r < 0) { *exit_status = EXIT_SELINUX_CONTEXT; @@ -2825,7 +2825,7 @@ static int exec_child( * are restricted. */ #ifdef HAVE_SELINUX - if (needs_selinux) { + if (use_selinux) { char *exec_context = mac_selinux_context_net ?: context->selinux_context; if (exec_context) { @@ -2840,7 +2840,7 @@ static int exec_child( #endif #ifdef HAVE_SMACK - if (needs_smack) { + if (use_smack) { r = setup_smack(context, command); if (r < 0) { *exit_status = EXIT_SMACK_PROCESS_LABEL; @@ -2851,7 +2851,7 @@ static int exec_child( #endif #ifdef HAVE_APPARMOR - if (needs_apparmor) { + if (use_apparmor && context->apparmor_profile) { r = aa_change_onexec(context->apparmor_profile); if (r < 0 && !context->apparmor_profile_ignore) { *exit_status = EXIT_APPARMOR_PROFILE; From 8f2c2f20b6383d7528ad86a26780b36f2a02e2d3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Aug 2017 12:46:09 +0200 Subject: [PATCH 14/19] mkdir: chmod_and_chown() returns errors as "return -errno", not in errno itself --- src/basic/mkdir.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/basic/mkdir.c b/src/basic/mkdir.c index 6b1a98402c..7db09fc6a1 100644 --- a/src/basic/mkdir.c +++ b/src/basic/mkdir.c @@ -31,10 +31,13 @@ int mkdir_safe_internal(const char *path, mode_t mode, uid_t uid, gid_t gid, mkdir_func_t _mkdir) { struct stat st; + int r; - if (_mkdir(path, mode) >= 0) - if (chmod_and_chown(path, mode, uid, gid) < 0) - return -errno; + if (_mkdir(path, mode) >= 0) { + r = chmod_and_chown(path, mode, uid, gid); + if (r < 0) + return r; + } if (lstat(path, &st) < 0) return -errno; From 6eaaeee93a07eaa3108f99cd71d06af1adb68786 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Aug 2017 15:04:05 +0200 Subject: [PATCH 15/19] seccomp: add new @setuid seccomp group This new group lists all UID/GID credential changing syscalls (which are quite a number these days). This will become particularly useful in a later commit, which uses this group to optionally permit user credential changing to daemons in case ambient capabilities are not available. --- man/systemd.exec.xml | 4 ++++ src/shared/seccomp-util.c | 19 +++++++++++++++++++ src/shared/seccomp-util.h | 1 + 3 files changed, 24 insertions(+) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index b3495c9785..f138dedacf 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -1505,6 +1505,10 @@ @resources System calls for changing resource limits, memory and scheduling parameters (setrlimit2, setpriority2, …) + + @setuid + System calls for changing user ID and group ID credentials, (setuid2, setgid2, setresuid2, …) + @swap System calls for enabling/disabling swap devices (swapon2, swapoff2) diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index 147b1b2ab2..e80d98ea0c 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -639,6 +639,25 @@ const SyscallFilterSet syscall_filter_sets[_SYSCALL_FILTER_SET_MAX] = { "sched_setattr\0" "prlimit64\0" }, + [SYSCALL_FILTER_SET_SETUID] = { + .name = "@setuid", + .help = "Operations for changing user/group credentials", + .value = + "setgid32\0" + "setgid\0" + "setgroups32\0" + "setgroups\0" + "setregid32\0" + "setregid\0" + "setresgid32\0" + "setresgid\0" + "setresuid32\0" + "setresuid\0" + "setreuid32\0" + "setreuid\0" + "setuid32\0" + "setuid\0" + }, [SYSCALL_FILTER_SET_SWAP] = { .name = "@swap", .help = "Enable/disable swap devices", diff --git a/src/shared/seccomp-util.h b/src/shared/seccomp-util.h index 596539e8f5..f6b6889460 100644 --- a/src/shared/seccomp-util.h +++ b/src/shared/seccomp-util.h @@ -58,6 +58,7 @@ enum { SYSCALL_FILTER_SET_RAW_IO, SYSCALL_FILTER_SET_REBOOT, SYSCALL_FILTER_SET_RESOURCES, + SYSCALL_FILTER_SET_SETUID, SYSCALL_FILTER_SET_SWAP, _SYSCALL_FILTER_SET_MAX }; From 6067611a0847d5b8d4f322cf6540b016c95d4cb4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Aug 2017 15:05:36 +0200 Subject: [PATCH 16/19] capability: change capability_bounding_set_drop() to be work without privileges when executing a NOP This way daemons which already dropped all caps may use the call to drop priviliges again, which becomes a non-failing NOP. --- src/basic/capability-util.c | 93 ++++++++++++++++++++----------------- 1 file changed, 51 insertions(+), 42 deletions(-) diff --git a/src/basic/capability-util.c b/src/basic/capability-util.c index c3de20a0e8..fe10536a69 100644 --- a/src/basic/capability-util.c +++ b/src/basic/capability-util.c @@ -151,7 +151,7 @@ int capability_ambient_set_apply(uint64_t set, bool also_inherit) { } int capability_bounding_set_drop(uint64_t keep, bool right_now) { - _cleanup_cap_free_ cap_t after_cap = NULL; + _cleanup_cap_free_ cap_t before_cap = NULL, after_cap = NULL; cap_flag_value_t fv; unsigned long i; int r; @@ -161,71 +161,80 @@ int capability_bounding_set_drop(uint64_t keep, bool right_now) { * executing init!), so get it back temporarily so that we can * call PR_CAPBSET_DROP. */ - after_cap = cap_get_proc(); - if (!after_cap) + before_cap = cap_get_proc(); + if (!before_cap) return -errno; - if (cap_get_flag(after_cap, CAP_SETPCAP, CAP_EFFECTIVE, &fv) < 0) + if (cap_get_flag(before_cap, CAP_SETPCAP, CAP_EFFECTIVE, &fv) < 0) return -errno; if (fv != CAP_SET) { _cleanup_cap_free_ cap_t temp_cap = NULL; static const cap_value_t v = CAP_SETPCAP; - temp_cap = cap_dup(after_cap); - if (!temp_cap) { - r = -errno; - goto finish; - } + temp_cap = cap_dup(before_cap); + if (!temp_cap) + return -errno; - if (cap_set_flag(temp_cap, CAP_EFFECTIVE, 1, &v, CAP_SET) < 0) { - r = -errno; - goto finish; - } + if (cap_set_flag(temp_cap, CAP_EFFECTIVE, 1, &v, CAP_SET) < 0) + return -errno; - if (cap_set_proc(temp_cap) < 0) { - r = -errno; - goto finish; - } + if (cap_set_proc(temp_cap) < 0) + log_debug_errno(errno, "Can't acquire effective CAP_SETPCAP bit, ignoring: %m"); + + /* If we didn't manage to acquire the CAP_SETPCAP bit, we continue anyway, after all this just means + * we'll fail later, when we actually intend to drop some capabilities. */ } + after_cap = cap_dup(before_cap); + if (!after_cap) + return -errno; + for (i = 0; i <= cap_last_cap(); i++) { + cap_value_t v; - if (!(keep & (UINT64_C(1) << i))) { - cap_value_t v; + if ((keep & (UINT64_C(1) << i))) + continue; - /* Drop it from the bounding set */ - if (prctl(PR_CAPBSET_DROP, i) < 0) { + /* Drop it from the bounding set */ + if (prctl(PR_CAPBSET_DROP, i) < 0) { + r = -errno; + + /* If dropping the capability failed, let's see if we didn't have it in the first place. If so, + * continue anyway, as dropping a capability we didn't have in the first place doesn't really + * matter anyway. */ + if (prctl(PR_CAPBSET_READ, i) != 0) + goto finish; + } + v = (cap_value_t) i; + + /* Also drop it from the inheritable set, so + * that anything we exec() loses the + * capability for good. */ + if (cap_set_flag(after_cap, CAP_INHERITABLE, 1, &v, CAP_CLEAR) < 0) { + r = -errno; + goto finish; + } + + /* If we shall apply this right now drop it + * also from our own capability sets. */ + if (right_now) { + if (cap_set_flag(after_cap, CAP_PERMITTED, 1, &v, CAP_CLEAR) < 0 || + cap_set_flag(after_cap, CAP_EFFECTIVE, 1, &v, CAP_CLEAR) < 0) { r = -errno; goto finish; } - v = (cap_value_t) i; - - /* Also drop it from the inheritable set, so - * that anything we exec() loses the - * capability for good. */ - if (cap_set_flag(after_cap, CAP_INHERITABLE, 1, &v, CAP_CLEAR) < 0) { - r = -errno; - goto finish; - } - - /* If we shall apply this right now drop it - * also from our own capability sets. */ - if (right_now) { - if (cap_set_flag(after_cap, CAP_PERMITTED, 1, &v, CAP_CLEAR) < 0 || - cap_set_flag(after_cap, CAP_EFFECTIVE, 1, &v, CAP_CLEAR) < 0) { - r = -errno; - goto finish; - } - } } } r = 0; finish: - if (cap_set_proc(after_cap) < 0) - return -errno; + if (cap_set_proc(after_cap) < 0) { + /* If there are no actual changes anyway then let's ignore this error. */ + if (cap_compare(before_cap, after_cap) != 0) + r = -errno; + } return r; } From 39f608e4b0ec2eea0a1a97df14bbcbe511101e18 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Aug 2017 15:07:15 +0200 Subject: [PATCH 17/19] capability: add new ambient_capabilities_supported() helper This new function reports whether ambient caps are available, and should be quick because the result is cached. --- src/basic/capability-util.c | 15 +++++++++++++++ src/basic/capability-util.h | 2 ++ src/test/test-capability.c | 2 ++ 3 files changed, 19 insertions(+) diff --git a/src/basic/capability-util.c b/src/basic/capability-util.c index fe10536a69..96c2e992bd 100644 --- a/src/basic/capability-util.c +++ b/src/basic/capability-util.c @@ -370,3 +370,18 @@ int drop_capability(cap_value_t cv) { return 0; } + +bool ambient_capabilities_supported(void) { + static int cache = -1; + + if (cache >= 0) + return cache; + + /* If PR_CAP_AMBIENT returns something valid, or an unexpected error code we assume that ambient caps are + * available. */ + + cache = prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_KILL, 0, 0) >= 0 || + !IN_SET(errno, EINVAL, EOPNOTSUPP, ENOSYS); + + return cache; +} diff --git a/src/basic/capability-util.h b/src/basic/capability-util.h index 35a896e229..3dc9429153 100644 --- a/src/basic/capability-util.h +++ b/src/basic/capability-util.h @@ -55,3 +55,5 @@ static inline bool cap_test_all(uint64_t caps) { m = (UINT64_C(1) << (cap_last_cap() + 1)) - 1; return (caps & m) == m; } + +bool ambient_capabilities_supported(void); diff --git a/src/test/test-capability.c b/src/test/test-capability.c index 629bb63c81..8276c75987 100644 --- a/src/test/test-capability.c +++ b/src/test/test-capability.c @@ -205,6 +205,8 @@ int main(int argc, char *argv[]) { log_parse_environment(); log_open(); + log_info("have ambient caps: %s", yes_no(ambient_capabilities_supported())); + if (getuid() != 0) return EXIT_TEST_SKIP; From 165a31c0dbd658444b6f03c95365554200740ebb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Aug 2017 16:09:04 +0200 Subject: [PATCH 18/19] core: add two new special ExecStart= character prefixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch adds two new special character prefixes to ExecStart= and friends, in addition to the existing "-", "@" and "+": "!" → much like "+", except with a much reduced effect as it only disables the actual setresuid()/setresgid()/setgroups() calls, but leaves all other security features on, including namespace options. This is very useful in combination with RuntimeDirectory= or DynamicUser= and similar option, as a user is still allocated and used for the runtime directory, but the actual UID/GID dropping is left to the daemon process itself. This should make RuntimeDirectory= a lot more useful for daemons which insist on doing their own privilege dropping. "!!" → Similar to "!", but on systems supporting ambient caps this becomes a NOP. This makes it relatively straightforward to write unit files that make use of ambient capabilities to let systemd drop all privs while retaining compatibility with systems that lack ambient caps, where priv dropping is the left to the daemon codes themselves. This is an alternative approach to #6564 and related PRs. --- man/systemd.service.xml | 59 ++++++++++++++++++--- src/core/execute.c | 106 +++++++++++++++++++++++++------------- src/core/execute.h | 2 + src/core/load-fragment.c | 40 +++++++++----- src/shared/seccomp-util.c | 38 ++++++++++++++ src/shared/seccomp-util.h | 2 + 6 files changed, 190 insertions(+), 57 deletions(-) diff --git a/man/systemd.service.xml b/man/systemd.service.xml index da35a5205d..9863532692 100644 --- a/man/systemd.service.xml +++ b/man/systemd.service.xml @@ -290,13 +290,58 @@ ExecStop= are not valid.) For each of the specified commands, the first argument must be an absolute path to an - executable. Optionally, if this file name is prefixed with @, the second token will be - passed as argv[0] to the executed process, followed by the further arguments specified. If - the absolute filename is prefixed with -, an exit code of the command normally considered a - failure (i.e. non-zero exit status or abnormal exit due to signal) is ignored and considered success. If the - absolute path is prefixed with + then it is executed with full - privileges. @, -, and + may be used together and they - can appear in any order. + executable. Optionally, this file name may be prefixed with a number of special characters: + + + Special executable prefixes + + + + + + + + Prefix + Effect + + + + + @ + If the executable path is prefixed with @, the second specified token will be passed as argv[0] to the executed process (instead of the actual filename), followed by the further arguments specified. + + + + - + If the executable path is prefixed with -, an exit code of the command normally considered a failure (i.e. non-zero exit status or abnormal exit due to signal) is ignored and considered success. + + + + + + If the executable path is prefixed with + then the process is executed with full privileges. In this mode privilege restrictions configured with User=, Group=, CapabilityBoundingSet= or the various file system namespacing options (such as PrivateDevices=, PrivateTmp=) are not applied to the invoked command line (but still affect any other ExecStart=, ExecStop=, … lines). + + + + ! + + Similar to the + character discussed above this permits invoking command lines with elevated privileges. However, unlike + the ! character exclusively alters the effect of User=, Group= and SupplementaryGroups=, i.e. only the stanzas the affect user and group credentials. Note that this setting may be combined with DynamicUser=, in which case a dynamic user/group pair is allocated before the command is invoked, but credential changing is left to the executed process itself. + + + + !! + + This prefix is very similar to !!, however it only has an effect on systems lacking support for ambient process capabilities, i.e. without support for AmbientCapabilities=. It's intended to be used for unit files that take benefit of ambient capabilities to run processes with minimal privileges wherever possible while remaining compatible with systems that lack ambient capabilities support. Note that when !! is used, and a system lacking ambient capability support is detected any configured SystemCallFilter= and CapabilityBoundingSet= stanzas are implicitly modified, in order to permit spawned processes to drop credentials and capabilites themselves, even if this is configured to not be allowed. Moreover, if this prefix is used and a system lacking ambient capability support is detected AmbientCapabilities= will be skipped and not be applied. On systems supporting ambient capabilities, !! has no effect and is redundant. + + + +
+ + @, -, and one of + +/!/!! may be used together and they can appear in any + order. However, only one of +, !, !! may be used a at + time. Note that these prefixes are also supported for the other command line settings, + i.e. ExecStartPre=, ExecStartPost=, ExecReload, + ExecStop= and ExecStopPost=. If more than one command is specified, the commands are invoked sequentially in the order they appear in the unit diff --git a/src/core/execute.c b/src/core/execute.c index 6caf13baf6..46e0b184a4 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1312,8 +1312,9 @@ static bool skip_seccomp_unavailable(const Unit* u, const char* msg) { return true; } -static int apply_syscall_filter(const Unit* u, const ExecContext *c) { +static int apply_syscall_filter(const Unit* u, const ExecContext *c, bool needs_ambient_hack) { uint32_t negative_action, default_action, action; + int r; assert(u); assert(c); @@ -1334,6 +1335,12 @@ static int apply_syscall_filter(const Unit* u, const ExecContext *c) { action = negative_action; } + if (needs_ambient_hack) { + r = seccomp_filter_set_add(c->syscall_filter, c->syscall_whitelist, syscall_filter_sets + SYSCALL_FILTER_SET_SETUID); + if (r < 0) + return r; + } + return seccomp_load_syscall_filter_set_raw(default_action, c->syscall_filter, action); } @@ -2004,7 +2011,7 @@ static int apply_mount_namespace( .protect_kernel_modules = context->protect_kernel_modules, .mount_apivfs = context->mount_apivfs, }; - bool apply_restrictions; + bool needs_sandboxing; int r; assert(context); @@ -2039,18 +2046,18 @@ static int apply_mount_namespace( if (!context->dynamic_user && root_dir) ns_info.ignore_protect_paths = true; - apply_restrictions = (params->flags & EXEC_APPLY_SANDBOXING) && !(command->flags & EXEC_COMMAND_FULLY_PRIVILEGED); + needs_sandboxing = (params->flags & EXEC_APPLY_SANDBOXING) && !(command->flags & EXEC_COMMAND_FULLY_PRIVILEGED); r = setup_namespace(root_dir, root_image, &ns_info, rw, - apply_restrictions ? context->read_only_paths : NULL, - apply_restrictions ? context->inaccessible_paths : NULL, + needs_sandboxing ? context->read_only_paths : NULL, + needs_sandboxing ? context->inaccessible_paths : NULL, context->bind_mounts, context->n_bind_mounts, tmp, var, - apply_restrictions ? context->protect_home : PROTECT_HOME_NO, - apply_restrictions ? context->protect_system : PROTECT_SYSTEM_NO, + needs_sandboxing ? context->protect_home : PROTECT_HOME_NO, + needs_sandboxing ? context->protect_system : PROTECT_SYSTEM_NO, context->mount_flags, DISSECT_IMAGE_DISCARD_ON_LOOP); @@ -2302,7 +2309,10 @@ static int exec_child( const char *home = NULL, *shell = NULL; dev_t journal_stream_dev = 0; ino_t journal_stream_ino = 0; - bool needs_sandboxing, needs_mount_namespace; + bool needs_sandboxing, /* Do we need to set up full sandboxing? (i.e. all namespacing, all MAC stuff, caps, yadda yadda */ + needs_setuid, /* Do we need to do the actual setresuid()/setresgid() calls? */ + needs_mount_namespace, /* Do we need to set up a mount namespace for this kernel? */ + needs_ambient_hack; /* Do we need to apply the ambient capabilities hack? */ #ifdef HAVE_SELINUX bool use_selinux = false; #endif @@ -2317,6 +2327,7 @@ static int exec_child( int i, r, ngids = 0; unsigned n_fds; ExecDirectoryType dt; + int secure_bits; assert(unit); assert(command); @@ -2653,17 +2664,19 @@ static int exec_child( return r; } + /* We need sandboxing if the caller asked us to apply it and the command isn't explicitly excepted from it */ needs_sandboxing = (params->flags & EXEC_APPLY_SANDBOXING) && !(command->flags & EXEC_COMMAND_FULLY_PRIVILEGED); - if (needs_sandboxing) { - if (context->pam_name && username) { - r = setup_pam(context->pam_name, username, uid, gid, context->tty_path, &accum_env, fds, n_fds); - if (r < 0) { - *exit_status = EXIT_PAM; - return r; - } - } + /* We need the ambient capability hack, if the caller asked us to apply it and the command is marked for it, and the kernel doesn't actually support ambient caps */ + needs_ambient_hack = (params->flags & EXEC_APPLY_SANDBOXING) && (command->flags & EXEC_COMMAND_AMBIENT_MAGIC) && !ambient_capabilities_supported(); + /* We need setresuid() if the caller asked us to apply sandboxing and the command isn't explicitly excepted from either whole sandboxing or just setresuid() itself, and the ambient hack is not desired */ + if (needs_ambient_hack) + needs_setuid = false; + else + needs_setuid = (params->flags & EXEC_APPLY_SANDBOXING) && !(command->flags & (EXEC_COMMAND_FULLY_PRIVILEGED|EXEC_COMMAND_NO_SETUID)); + + if (needs_sandboxing) { /* MAC enablement checks need to be done before a new mount ns is created, as they rely on /sys being * present. The actual MAC context application will happen later, as late as possible, to avoid * impacting our own code paths. */ @@ -2671,15 +2684,22 @@ static int exec_child( #ifdef HAVE_SELINUX use_selinux = mac_selinux_use(); #endif - #ifdef HAVE_SMACK use_smack = mac_smack_use(); #endif - #ifdef HAVE_APPARMOR use_apparmor = mac_apparmor_use(); #endif + } + if (needs_setuid) { + if (context->pam_name && username) { + r = setup_pam(context->pam_name, username, uid, gid, context->tty_path, &accum_env, fds, n_fds); + if (r < 0) { + *exit_status = EXIT_PAM; + return r; + } + } } if (context->private_network && runtime && runtime->netns_storage_socket[0] >= 0) { @@ -2705,13 +2725,15 @@ static int exec_child( return r; /* Drop groups as early as possbile */ - if (needs_sandboxing) { + if (needs_setuid) { r = enforce_groups(context, gid, supplementary_gids, ngids); if (r < 0) { *exit_status = EXIT_GROUP; return r; } + } + if (needs_sandboxing) { #ifdef HAVE_SELINUX if (use_selinux && params->selinux_context_net && socket_fd >= 0) { r = mac_selinux_get_child_mls_label(socket_fd, command->path, context->selinux_context, &mac_selinux_context_net); @@ -2731,12 +2753,9 @@ static int exec_child( } } - /* We repeat the fd closing here, to make sure that - * nothing is leaked from the PAM modules. Note that - * we are more aggressive this time since socket_fd - * and the netns fds we don't need anymore. The custom - * endpoint fd was needed to upload the policy and can - * now be closed as well. */ + /* We repeat the fd closing here, to make sure that nothing is leaked from the PAM modules. Note that we are + * more aggressive this time since socket_fd and the netns fds we don't need anymore. The custom endpoint fd + * was needed to upload the policy and can now be closed as well. */ r = close_all_fds(fds, n_fds); if (r >= 0) r = shift_fds(fds, n_fds); @@ -2747,9 +2766,10 @@ static int exec_child( return r; } - if (needs_sandboxing) { + secure_bits = context->secure_bits; - int secure_bits = context->secure_bits; + if (needs_sandboxing) { + uint64_t bset; for (i = 0; i < _RLIMIT_MAX; i++) { @@ -2771,8 +2791,17 @@ static int exec_child( } } - if (!cap_test_all(context->capability_bounding_set)) { - r = capability_bounding_set_drop(context->capability_bounding_set, false); + bset = context->capability_bounding_set; + /* If the ambient caps hack is enabled (which means the kernel can't do them, and the user asked for + * our magic fallback), then let's add some extra caps, so that the service can drop privs of its own, + * instead of us doing that */ + if (needs_ambient_hack) + bset |= (UINT64_C(1) << CAP_SETPCAP) | + (UINT64_C(1) << CAP_SETUID) | + (UINT64_C(1) << CAP_SETGID); + + if (!cap_test_all(bset)) { + r = capability_bounding_set_drop(bset, false); if (r < 0) { *exit_status = EXIT_CAPABILITIES; *error_message = strdup("Failed to drop capabilities"); @@ -2782,7 +2811,8 @@ static int exec_child( /* This is done before enforce_user, but ambient set * does not survive over setresuid() if keep_caps is not set. */ - if (context->capability_ambient_set != 0) { + if (!needs_ambient_hack && + context->capability_ambient_set != 0) { r = capability_ambient_set_apply(context->capability_ambient_set, true); if (r < 0) { *exit_status = EXIT_CAPABILITIES; @@ -2790,7 +2820,9 @@ static int exec_child( return r; } } + } + if (needs_setuid) { if (context->user) { r = enforce_user(context, uid); if (r < 0) { @@ -2798,7 +2830,9 @@ static int exec_child( (void) asprintf(error_message, "Failed to change UID to "UID_FMT, uid); return r; } - if (context->capability_ambient_set != 0) { + + if (!needs_ambient_hack && + context->capability_ambient_set != 0) { /* Fix the ambient capabilities after user change. */ r = capability_ambient_set_apply(context->capability_ambient_set, false); @@ -2818,7 +2852,9 @@ static int exec_child( secure_bits |= 1<argv = n; nce->path = path; - nce->flags = - (ignore ? EXEC_COMMAND_IGNORE_FAILURE : 0) | - (privileged ? EXEC_COMMAND_FULLY_PRIVILEGED : 0); + nce->flags = flags; exec_command_append_list(e, nce); diff --git a/src/shared/seccomp-util.c b/src/shared/seccomp-util.c index e80d98ea0c..dd6d4fbdc7 100644 --- a/src/shared/seccomp-util.c +++ b/src/shared/seccomp-util.c @@ -1364,3 +1364,41 @@ int parse_syscall_archs(char **l, Set **archs) { return 0; } + +int seccomp_filter_set_add(Set *filter, bool add, const SyscallFilterSet *set) { + const char *i; + int r; + + assert(set); + + NULSTR_FOREACH(i, set->value) { + + if (i[0] == '@') { + const SyscallFilterSet *more; + + more = syscall_filter_set_find(i); + if (!more) + return -ENXIO; + + + r = seccomp_filter_set_add(filter, add, more); + if (r < 0) + return r; + } else { + int id; + + id = seccomp_syscall_resolve_name(i); + if (id == __NR_SCMP_ERROR) + return -ENXIO; + + if (add) { + r = set_put(filter, INT_TO_PTR(id + 1)); + if (r < 0) + return r; + } else + (void) set_remove(filter, INT_TO_PTR(id + 1)); + } + } + + return 0; +} diff --git a/src/shared/seccomp-util.h b/src/shared/seccomp-util.h index f6b6889460..0edffa116d 100644 --- a/src/shared/seccomp-util.h +++ b/src/shared/seccomp-util.h @@ -67,6 +67,8 @@ extern const SyscallFilterSet syscall_filter_sets[]; const SyscallFilterSet *syscall_filter_set_find(const char *name); +int seccomp_filter_set_add(Set *s, bool b, const SyscallFilterSet *set); + int seccomp_load_syscall_filter_set(uint32_t default_action, const SyscallFilterSet *set, uint32_t action); int seccomp_load_syscall_filter_set_raw(uint32_t default_action, Set* set, uint32_t action); From 635f3df5dc540892cca0b9d4ba42ab7215277f7c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 9 Aug 2017 16:15:07 +0200 Subject: [PATCH 19/19] units: make use of the new !! ExecStart= prefix in systemd-resolved.service Let's make use of !! to run resolved with ambient capabilities on systems supporting them. --- src/resolve/resolved.c | 19 ++++++++++++------- units/systemd-resolved.service.in | 9 ++++++--- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/resolve/resolved.c b/src/resolve/resolved.c index 74603f9311..8fd81956b8 100644 --- a/src/resolve/resolved.c +++ b/src/resolve/resolved.c @@ -67,13 +67,18 @@ int main(int argc, char *argv[]) { goto finish; } - /* Drop privileges, but keep three caps. Note that we drop those too, later on (see below) */ - r = drop_privileges(uid, gid, - (UINT64_C(1) << CAP_NET_RAW)| /* needed for SO_BINDTODEVICE */ - (UINT64_C(1) << CAP_NET_BIND_SERVICE)| /* needed to bind on port 53 */ - (UINT64_C(1) << CAP_SETPCAP) /* needed in order to drop the caps later */); - if (r < 0) - goto finish; + /* Drop privileges, but only if we have been started as root. If we are not running as root we assume all + * privileges are already dropped. */ + if (getuid() == 0) { + + /* Drop privileges, but keep three caps. Note that we drop those too, later on (see below) */ + r = drop_privileges(uid, gid, + (UINT64_C(1) << CAP_NET_RAW)| /* needed for SO_BINDTODEVICE */ + (UINT64_C(1) << CAP_NET_BIND_SERVICE)| /* needed to bind on port 53 */ + (UINT64_C(1) << CAP_SETPCAP) /* needed in order to drop the caps later */); + if (r < 0) + goto finish; + } assert_se(sigprocmask_many(SIG_BLOCK, NULL, SIGTERM, SIGINT, SIGUSR1, SIGUSR2, -1) >= 0); diff --git a/units/systemd-resolved.service.in b/units/systemd-resolved.service.in index 9821e1cfee..ba8d3f6bb1 100644 --- a/units/systemd-resolved.service.in +++ b/units/systemd-resolved.service.in @@ -19,9 +19,11 @@ Wants=nss-lookup.target Type=notify Restart=always RestartSec=0 -ExecStart=@rootlibexecdir@/systemd-resolved +ExecStart=!!@rootlibexecdir@/systemd-resolved WatchdogSec=3min -CapabilityBoundingSet=CAP_SETUID CAP_SETGID CAP_SETPCAP CAP_CHOWN CAP_DAC_OVERRIDE CAP_FOWNER CAP_NET_RAW CAP_NET_BIND_SERVICE +User=systemd-resolve +CapabilityBoundingSet=CAP_SETPCAP CAP_NET_RAW CAP_NET_BIND_SERVICE +AmbientCapabilities=CAP_SETPCAP CAP_NET_RAW CAP_NET_BIND_SERVICE PrivateTmp=yes PrivateDevices=yes ProtectSystem=strict @@ -34,7 +36,8 @@ RestrictRealtime=yes RestrictAddressFamilies=AF_UNIX AF_NETLINK AF_INET AF_INET6 SystemCallFilter=~@clock @cpu-emulation @debug @keyring @module @mount @obsolete @raw-io @reboot @swap SystemCallArchitectures=native -ReadWritePaths=/run/systemd +RuntimeDirectory=systemd/resolve +RuntimeDirectoryPreserve=yes [Install] WantedBy=multi-user.target