From 4e6e3b8707c84018051ae1885af20e06b2a5209e Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 26 Feb 2026 11:07:39 +0100 Subject: [PATCH 1/4] sd-json: fix off-by-one issue when updating parent for array elements Follow-up for 8525bb369a09f488ec77f94e1557ecc2343eb4ab --- src/libsystemd/sd-json/sd-json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsystemd/sd-json/sd-json.c b/src/libsystemd/sd-json/sd-json.c index 247e9790fe..647b56555a 100644 --- a/src/libsystemd/sd-json/sd-json.c +++ b/src/libsystemd/sd-json/sd-json.c @@ -2235,7 +2235,7 @@ _public_ int sd_json_variant_append_array(sd_json_variant **v, sd_json_variant * if (old != *v) /* Readjust the parent pointers to the new address */ - for (size_t i = 1; i < size; i++) + for (size_t i = 0; i < size; i++) (*v)[1 + i].parent = *v; return json_variant_array_put_element(*v, element); From 679e738bf47fe971131d149bf11346c84eb45371 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 21 Feb 2026 19:53:03 +0100 Subject: [PATCH 2/4] core/dbus-unit: query sender uid only once, validate unit uid early Follow-up for 05f5156ad1a3b84b54c104ee375b9ce7b746e0cd --- src/core/dbus-unit.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 525ec76da2..1f9030f3e2 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -1559,9 +1559,9 @@ static int property_get_effective_limit( } int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd_bus_error *reterr_error) { + Unit *u = ASSERT_PTR(userdata); _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; _cleanup_set_free_ Set *pids = NULL; - Unit *u = userdata; const char *path; int r; @@ -1599,12 +1599,22 @@ int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd if (r < 0) return r; + /* Let's query the sender's UID, so that we can make our security decisions */ + uid_t sender_uid; + r = sd_bus_creds_get_euid(creds, &sender_uid); + if (r < 0) + return r; + bool validate_ownership = sender_uid != 0 && sender_uid != getuid(); + + if (validate_ownership && !uid_is_valid(u->ref_uid)) /* process_is_owned_by_uid() requires a valid uid */ + return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_ACCESS_DENIED, + "Refusing to attach processes to unit with unknown user credentials."); + r = sd_bus_message_enter_container(message, 'a', "u"); if (r < 0) return r; for (;;) { _cleanup_(pidref_freep) PidRef *pidref = NULL; - uid_t sender_uid; uint32_t upid; r = sd_bus_message_read(message, "u", &upid); @@ -1634,25 +1644,17 @@ int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd if (r < 0) return r; - /* Let's query the sender's UID, so that we can make our security decisions */ - r = sd_bus_creds_get_euid(creds, &sender_uid); - if (r < 0) - return r; - /* Let's validate security: if the sender is root or the owner of the service manager, then * all is OK. If the sender is any other user, then the process in question must be owned by * both the sender and the target unit's UID. Note that ownership here means either direct * ownership, or indirect via a userns that is owned by the right UID. */ - if (sender_uid != 0 && sender_uid != getuid()) { + if (validate_ownership) { r = process_is_owned_by_uid(pidref, sender_uid); if (r < 0) return sd_bus_error_set_errnof(reterr_error, r, "Failed to check if process " PID_FMT " is owned by client's UID: %m", pidref->pid); if (r == 0) return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_ACCESS_DENIED, "Process " PID_FMT " not owned by client's UID. Refusing.", pidref->pid); - if (!uid_is_valid(u->ref_uid)) /* process_is_owned_by_uid() requires a valid uid */ - return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_ACCESS_DENIED, "Unit does not have a valid ref_uid, refusing to attach process " PID_FMT ".", pidref->pid); - r = process_is_owned_by_uid(pidref, u->ref_uid); if (r < 0) return sd_bus_error_set_errnof(reterr_error, r, "Failed to check if process " PID_FMT " is owned by target unit's UID: %m", pidref->pid); From 42aee39107fbdd7db1ccd402a2151822b2805e9f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 26 Feb 2026 11:06:00 +0100 Subject: [PATCH 3/4] core/cgroup: avoid one unnecessary strjoina() --- src/core/cgroup.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index ad889db3b9..514dabf371 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -2115,12 +2115,13 @@ static int unit_update_cgroup( return 0; } -static int unit_attach_pid_to_cgroup_via_bus(Unit *u, pid_t pid, const char *suffix_path) { +static int unit_attach_pid_to_cgroup_via_bus(Unit *u, const char *cgroup_path, pid_t pid) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - char *pp; int r; assert(u); + assert(cgroup_path); + assert(pid_is_valid(pid)); if (MANAGER_IS_SYSTEM(u->manager)) return -EINVAL; @@ -2128,18 +2129,13 @@ static int unit_attach_pid_to_cgroup_via_bus(Unit *u, pid_t pid, const char *suf if (!u->manager->system_bus) return -EIO; - CGroupRuntime *crt = unit_get_cgroup_runtime(u); - if (!crt || !crt->cgroup_path) - return -EOWNERDEAD; - /* Determine this unit's cgroup path relative to our cgroup root */ - pp = path_startswith(crt->cgroup_path, u->manager->cgroup_root); + const char *pp = path_startswith_full(cgroup_path, + u->manager->cgroup_root, + PATH_STARTSWITH_RETURN_LEADING_SLASH|PATH_STARTSWITH_REFUSE_DOT_DOT); if (!pp) return -EINVAL; - pp = strjoina("/", pp, suffix_path); - path_simplify(pp); - r = bus_call_method(u->manager->system_bus, bus_systemd_mgr, "AttachProcessesToUnit", @@ -2172,8 +2168,10 @@ int unit_attach_pids_to_cgroup(Unit *u, Set *pids, const char *suffix_path) { CGroupRuntime *crt = ASSERT_PTR(unit_get_cgroup_runtime(u)); if (isempty(suffix_path)) - p = crt->cgroup_path; + p = empty_to_root(crt->cgroup_path); else { + assert(path_is_absolute(suffix_path)); + joined = path_join(crt->cgroup_path, suffix_path); if (!joined) return -ENOMEM; @@ -2189,7 +2187,7 @@ int unit_attach_pids_to_cgroup(Unit *u, Set *pids, const char *suffix_path) { * before we use it */ r = pidref_verify(pid); if (r < 0) { - log_unit_info_errno(u, r, "PID " PID_FMT " vanished before we could move it to target cgroup '%s', skipping: %m", pid->pid, empty_to_root(p)); + log_unit_info_errno(u, r, "PID " PID_FMT " vanished before we could move it to target cgroup '%s', skipping: %m", pid->pid, p); continue; } @@ -2199,7 +2197,7 @@ int unit_attach_pids_to_cgroup(Unit *u, Set *pids, const char *suffix_path) { log_unit_full_errno(u, again ? LOG_DEBUG : LOG_INFO, r, "Couldn't move process "PID_FMT" to%s requested cgroup '%s': %m", - pid->pid, again ? " directly" : "", empty_to_root(p)); + pid->pid, again ? " directly" : "", p); if (again) { int z; @@ -2209,11 +2207,11 @@ int unit_attach_pids_to_cgroup(Unit *u, Set *pids, const char *suffix_path) { * Since it's more privileged it might be able to move the process across the * leaves of a subtree whose top node is not owned by us. */ - z = unit_attach_pid_to_cgroup_via_bus(u, pid->pid, suffix_path); + z = unit_attach_pid_to_cgroup_via_bus(u, p, pid->pid); if (z >= 0) goto success; - log_unit_info_errno(u, z, "Couldn't move process "PID_FMT" to requested cgroup '%s' (directly or via the system bus): %m", pid->pid, empty_to_root(p)); + log_unit_info_errno(u, z, "Couldn't move process "PID_FMT" to requested cgroup '%s' (directly or via the system bus): %m", pid->pid, p); } RET_GATHER(ret, r); From efa6ba2ab625aaa160ac435a09e6482fc63bdbe8 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 26 Feb 2026 11:06:34 +0100 Subject: [PATCH 4/4] core: validate input cgroup path more prudently --- src/core/dbus-manager.c | 6 ++++++ src/core/varlink-unit.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index ad79c6e96a..5e02d18907 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -652,6 +652,12 @@ static int method_get_unit_by_control_group(sd_bus_message *message, void *userd if (r < 0) return r; + if (!path_is_absolute(cgroup)) + return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_INVALID_ARGS, "Control group path is not absolute: %s", cgroup); + + if (!path_is_normalized(cgroup)) + return sd_bus_error_setf(reterr_error, SD_BUS_ERROR_INVALID_ARGS, "Control group path is not normalized: %s", cgroup); + u = manager_get_unit_by_cgroup(m, cgroup); if (!u) return sd_bus_error_setf(reterr_error, BUS_ERROR_NO_SUCH_UNIT, diff --git a/src/core/varlink-unit.c b/src/core/varlink-unit.c index c554a11f5e..3babe02732 100644 --- a/src/core/varlink-unit.c +++ b/src/core/varlink-unit.c @@ -457,7 +457,7 @@ static int lookup_unit_by_parameters( } if (p->cgroup) { - if (!path_is_safe(p->cgroup)) + if (!path_is_absolute(p->cgroup) || !path_is_normalized(p->cgroup)) return sd_varlink_error_invalid_parameter_name(link, "cgroup"); Unit *cgroup_unit = manager_get_unit_by_cgroup(manager, p->cgroup);