From f28a7e87f1a16fb7ac8a28d9d7d06a3baa96c12c Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 26 May 2024 01:16:17 +0800 Subject: [PATCH 1/7] core/load-fragment: also clear missing_ok when WorkingDirectory="" --- src/core/load-fragment.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 8215a66e31..5ae68886af 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -2608,6 +2608,7 @@ int config_parse_working_directory( assert(rvalue); if (isempty(rvalue)) { + c->working_directory_missing_ok = false; c->working_directory_home = false; c->working_directory = mfree(c->working_directory); return 0; From af87bdc6bc0d5b50af87ffd3b5cbd3e7c472dd42 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 26 May 2024 00:49:09 +0800 Subject: [PATCH 2/7] core/dbus-execute: use correct char for representing WorkingDirectory=home --- src/core/dbus-execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index e907aa67af..e55fb6ee16 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -2755,7 +2755,7 @@ int bus_exec_context_set_transient_property( c->working_directory_home = is_home; c->working_directory_missing_ok = missing_ok; - unit_write_settingf(u, flags|UNIT_ESCAPE_SPECIFIERS, name, "WorkingDirectory=%s%s", missing_ok ? "-" : "", c->working_directory_home ? "+" : ASSERT_PTR(c->working_directory)); + unit_write_settingf(u, flags|UNIT_ESCAPE_SPECIFIERS, name, "WorkingDirectory=%s%s", missing_ok ? "-" : "", c->working_directory_home ? "~" : ASSERT_PTR(c->working_directory)); } return 1; From 6f8ef80bb3ba5d244a428aee200c168e809a0079 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 26 May 2024 00:53:46 +0800 Subject: [PATCH 3/7] core/dbus-execute: don't trigger assertion if WorkingDirectory="" or "-" Follow-up for 14631951cea807de2d482a430841c604c2040718 Before this commit, if WorkingDirectory= is empty or literally "-", 'simplified' is not populated, resulting in the ASSERT_PTR in unit_write_settingf() below getting triggered. Also, do not accept "-", so that the parser is consistent with load-fragment.c Fixes #33015 --- src/core/dbus-execute.c | 49 ++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index e55fb6ee16..21c260b26b 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -2716,38 +2716,38 @@ int bus_exec_context_set_transient_property( } else if (streq(name, "WorkingDirectory")) { _cleanup_free_ char *simplified = NULL; - bool missing_ok, is_home; + bool missing_ok = false, is_home = false; const char *s; r = sd_bus_message_read(message, "s", &s); if (r < 0) return r; - if (s[0] == '-') { - missing_ok = true; - s++; - } else - missing_ok = false; + if (!isempty(s)) { + if (s[0] == '-') { + missing_ok = true; + s++; + } - if (isempty(s)) - is_home = false; - else if (streq(s, "~")) - is_home = true; - else { - if (!path_is_absolute(s)) - return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "WorkingDirectory= expects an absolute path or '~'"); + if (streq(s, "~")) + is_home = true; + else { + if (!path_is_absolute(s)) + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, + "WorkingDirectory= expects an absolute path or '~'"); - r = path_simplify_alloc(s, &simplified); - if (r < 0) - return r; + r = path_simplify_alloc(s, &simplified); + if (r < 0) + return r; - if (!path_is_normalized(simplified)) - return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "WorkingDirectory= expects a normalized path or '~'"); + if (!path_is_normalized(simplified)) + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, + "WorkingDirectory= expects a normalized path or '~'"); - if (path_below_api_vfs(simplified)) - return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "WorkingDirectory= may not be below /proc/, /sys/ or /dev/."); - - is_home = false; + if (path_below_api_vfs(simplified)) + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, + "WorkingDirectory= may not be below /proc/, /sys/ or /dev/"); + } } if (!UNIT_WRITE_FLAGS_NOOP(flags)) { @@ -2755,7 +2755,10 @@ int bus_exec_context_set_transient_property( c->working_directory_home = is_home; c->working_directory_missing_ok = missing_ok; - unit_write_settingf(u, flags|UNIT_ESCAPE_SPECIFIERS, name, "WorkingDirectory=%s%s", missing_ok ? "-" : "", c->working_directory_home ? "~" : ASSERT_PTR(c->working_directory)); + unit_write_settingf(u, flags|UNIT_ESCAPE_SPECIFIERS, name, + "WorkingDirectory=%s%s", + c->working_directory_missing_ok ? "-" : "", + c->working_directory_home ? "~" : strempty(c->working_directory)); } return 1; From c0afdec5cff5765cbf3922a7328ca186b0512c67 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 26 May 2024 01:36:45 +0800 Subject: [PATCH 4/7] core/exec-invoke: drop unused param for acquire_home, prefix out param with ret_ --- src/core/exec-invoke.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 86bd797896..e719976daa 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -3585,12 +3585,12 @@ static int send_user_lookup( return 0; } -static int acquire_home(const ExecContext *c, uid_t uid, const char** home, char **buf) { +static int acquire_home(const ExecContext *c, const char **home, char **ret_buf) { int r; assert(c); assert(home); - assert(buf); + assert(ret_buf); /* If WorkingDirectory=~ is set, try to acquire a usable home directory. */ @@ -3600,11 +3600,11 @@ static int acquire_home(const ExecContext *c, uid_t uid, const char** home, char if (!c->working_directory_home) return 0; - r = get_home_dir(buf); + r = get_home_dir(ret_buf); if (r < 0) return r; - *home = *buf; + *home = *ret_buf; return 1; } @@ -4291,7 +4291,7 @@ int exec_invoke( params->user_lookup_fd = safe_close(params->user_lookup_fd); - r = acquire_home(context, uid, &home, &home_buffer); + r = acquire_home(context, &home, &home_buffer); if (r < 0) { *exit_status = EXIT_CHDIR; return log_exec_error_errno(context, params, r, "Failed to determine $HOME for user: %m"); From 3a14167102ac6a38d97208e7e95706ba1d68d0f4 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 26 May 2024 04:16:15 +0800 Subject: [PATCH 5/7] core/exec-invoke: add a comment that acquire_home uses result from get_fixed_user Prompted by https://github.com/systemd/systemd/pull/33016#discussion_r1614848067 --- src/core/exec-invoke.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index e719976daa..ee62ff3dd4 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -3594,7 +3594,7 @@ static int acquire_home(const ExecContext *c, const char **home, char **ret_buf) /* If WorkingDirectory=~ is set, try to acquire a usable home directory. */ - if (*home) + if (*home) /* Already acquired from get_fixed_user()? */ return 0; if (!c->working_directory_home) From 4dd884af1ba74d837d230490480396b3ceaca935 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 26 May 2024 01:43:02 +0800 Subject: [PATCH 6/7] core/unit: don't set missing_ok if WorkingDirectory=~ is explicitly requested --- src/core/unit.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 324f82c3a4..7698f601c7 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4238,16 +4238,14 @@ int unit_patch_contexts(Unit *u) { return -ENOMEM; } - if (MANAGER_IS_USER(u->manager) && - !ec->working_directory) { - + if (MANAGER_IS_USER(u->manager) && !ec->working_directory) { r = get_home_dir(&ec->working_directory); if (r < 0) return r; - /* Allow user services to run, even if the - * home directory is missing */ - ec->working_directory_missing_ok = true; + if (!ec->working_directory_home) + /* If home directory is implied by us, allow it to be missing. */ + ec->working_directory_missing_ok = true; } if (ec->private_devices) From 52d8ba71b6c1e215e1e99bd1b35c87603cbebc6b Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 26 May 2024 04:43:53 +0800 Subject: [PATCH 7/7] core: introduce unit_verify_contexts Refuse WorkingDirectory=~ both in that and exec_invoke() when dynamic user is used. --- src/core/exec-invoke.c | 3 +++ src/core/unit.c | 17 ++++++++++++++++- src/test/test-execute.c | 7 ++++++- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index ee62ff3dd4..e88f524893 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -3600,6 +3600,9 @@ static int acquire_home(const ExecContext *c, const char **home, char **ret_buf) if (!c->working_directory_home) return 0; + if (c->dynamic_user) + return -EADDRNOTAVAIL; + r = get_home_dir(ret_buf); if (r < 0) return r; diff --git a/src/core/unit.c b/src/core/unit.c index 7698f601c7..1dea0db3fe 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4217,6 +4217,21 @@ static int user_from_unit_name(Unit *u, char **ret) { return 0; } +static int unit_verify_contexts(const Unit *u, const ExecContext *ec) { + assert(u); + + if (!ec) + return 0; + + if (MANAGER_IS_USER(u->manager) && ec->dynamic_user) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(ENOEXEC), "DynamicUser= enabled for user unit, which is not supported. Refusing."); + + if (ec->dynamic_user && ec->working_directory_home) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(ENOEXEC), "WorkingDirectory=~ is not allowed under DynamicUser=yes. Refusing."); + + return 0; +} + int unit_patch_contexts(Unit *u) { CGroupContext *cc; ExecContext *ec; @@ -4340,7 +4355,7 @@ int unit_patch_contexts(Unit *u) { } } - return 0; + return unit_verify_contexts(u, ec); } ExecContext *unit_get_exec_context(const Unit *u) { diff --git a/src/test/test-execute.c b/src/test/test-execute.c index 23eefcdf4b..191741d97c 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -987,6 +987,11 @@ static char* private_directory_bad(Manager *m) { } static void test_exec_dynamicuser(Manager *m) { + if (MANAGER_IS_USER(m)) { + log_notice("Skipping %s for user manager", __func__); + return; + } + _cleanup_free_ char *bad = private_directory_bad(m); if (bad) { log_warning("%s: %s has bad permissions, skipping test.", __func__, bad); @@ -998,7 +1003,7 @@ static void test_exec_dynamicuser(Manager *m) { return; } - int status = can_unshare ? 0 : MANAGER_IS_SYSTEM(m) ? EXIT_NAMESPACE : EXIT_GROUP; + int status = can_unshare ? 0 : EXIT_NAMESPACE; test(m, "exec-dynamicuser-fixeduser.service", status, CLD_EXITED); if (check_user_has_group_with_same_name("adm"))