From e89eaeb0274a51a59e00e7290ee682d46e324b73 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 24 Jan 2025 02:16:36 +0900 Subject: [PATCH 1/5] udev-rules: get_user_creds()/get_group_creds() return -ESRCH when user/group does not exist This drops -ENOENT error check for get_user_creds()/get_group_creds(), as nowadays they always return -ESRCH when the specified user/groups cannot be found. This also adds short comments for NULL arguments. --- src/udev/udev-rules.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 29c8c37c29..2fbf422aa4 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -489,9 +489,15 @@ static int rule_resolve_user(UdevRuleLine *rule_line, const char *name, uid_t *r return 0; } - r = get_user_creds(&name, &uid, NULL, NULL, NULL, USER_CREDS_ALLOW_MISSING); + r = get_user_creds( + &name, + &uid, + /* ret_gid = */ NULL, + /* ret_home = */ NULL, + /* ret_shell = */ NULL, + USER_CREDS_ALLOW_MISSING); if (r < 0) { - if (IN_SET(r, -ENOENT, -ESRCH)) + if (r == -ESRCH) log_line_error_errno(rule_line, r, "Unknown user '%s', ignoring.", name); else log_line_error_errno(rule_line, r, "Failed to resolve user '%s', ignoring: %m", name); @@ -531,7 +537,7 @@ static int rule_resolve_group(UdevRuleLine *rule_line, const char *name, gid_t * r = get_group_creds(&name, &gid, USER_CREDS_ALLOW_MISSING); if (r < 0) { - if (IN_SET(r, -ENOENT, -ESRCH)) + if (r == -ESRCH) log_line_error_errno(rule_line, r, "Unknown group '%s', ignoring.", name); else log_line_error_errno(rule_line, r, "Failed to resolve group '%s', ignoring: %m", name); @@ -2658,8 +2664,14 @@ static int udev_rule_apply_token_to_event( if (!apply_format_value(event, token, owner, sizeof(owner), "user name")) return true; - r = get_user_creds(&ow, &event->uid, NULL, NULL, NULL, USER_CREDS_ALLOW_MISSING); - if (IN_SET(r, -ENOENT, -ESRCH)) + r = get_user_creds( + &ow, + &event->uid, + /* ret_gid = */ NULL, + /* ret_home = */ NULL, + /* ret_shell = */ NULL, + USER_CREDS_ALLOW_MISSING); + if (r == -ESRCH) log_event_error_errno(event, token, r, "Unknown user \"%s\", ignoring.", owner); else if (r < 0) log_event_error_errno(event, token, r, "Failed to resolve user \"%s\", ignoring: %m", owner); @@ -2681,7 +2693,7 @@ static int udev_rule_apply_token_to_event( return true; r = get_group_creds(&gr, &event->gid, USER_CREDS_ALLOW_MISSING); - if (IN_SET(r, -ENOENT, -ESRCH)) + if (r == -ESRCH) log_event_error_errno(event, token, r, "Unknown group \"%s\", ignoring.", group); else if (r < 0) log_event_error_errno(event, token, r, "Failed to resolve group \"%s\", ignoring: %m", group); From a1ee55e3c9eaa02737cb74cf3fc93583b307c8cd Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 23 Jan 2025 05:59:04 +0900 Subject: [PATCH 2/5] udev-rules: ignore OWNER=/GROUP= with unknown user/group Previously, when an unknown or invalid user/group is specified, a token was installed with UID_INVALID/GID_INVALID. That's not only meaningless in most cases, but also clears previous assignment, if multiple OWNER=/GROUP= token exist for the same device, e.g. KERNEL=="sda", GROUP="disk" KERNEL=="sda", GROUP="nonexistentuser" This makes when an unknown user/group is specified, the line will be ignored. Hence, in the above example, the device will be owned by the group "disk". --- src/udev/udev-rules.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 2fbf422aa4..91a0d1f01a 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -496,23 +496,18 @@ static int rule_resolve_user(UdevRuleLine *rule_line, const char *name, uid_t *r /* ret_home = */ NULL, /* ret_shell = */ NULL, USER_CREDS_ALLOW_MISSING); - if (r < 0) { - if (r == -ESRCH) - log_line_error_errno(rule_line, r, "Unknown user '%s', ignoring.", name); - else - log_line_error_errno(rule_line, r, "Failed to resolve user '%s', ignoring: %m", name); - - *ret = UID_INVALID; - return 0; - } + if (r == -ESRCH) + return log_line_error_errno(rule_line, r, "Unknown user '%s', ignoring.", name); + if (r < 0) + return log_line_error_errno(rule_line, r, "Failed to resolve user '%s', ignoring: %m", name); n = strdup(name); if (!n) - return -ENOMEM; + return log_oom(); r = hashmap_ensure_put(known_users, &string_hash_ops_free, n, UID_TO_PTR(uid)); if (r < 0) - return r; + return log_oom(); TAKE_PTR(n); *ret = uid; @@ -536,23 +531,18 @@ static int rule_resolve_group(UdevRuleLine *rule_line, const char *name, gid_t * } r = get_group_creds(&name, &gid, USER_CREDS_ALLOW_MISSING); - if (r < 0) { - if (r == -ESRCH) - log_line_error_errno(rule_line, r, "Unknown group '%s', ignoring.", name); - else - log_line_error_errno(rule_line, r, "Failed to resolve group '%s', ignoring: %m", name); - - *ret = GID_INVALID; - return 0; - } + if (r == -ESRCH) + return log_line_error_errno(rule_line, r, "Unknown group '%s', ignoring.", name); + if (r < 0) + return log_line_error_errno(rule_line, r, "Failed to resolve group '%s', ignoring: %m", name); n = strdup(name); if (!n) - return -ENOMEM; + return log_oom(); r = hashmap_ensure_put(known_groups, &string_hash_ops_free, n, GID_TO_PTR(gid)); if (r < 0) - return r; + return log_oom(); TAKE_PTR(n); *ret = gid; @@ -1052,9 +1042,10 @@ static int parse_token( r = rule_line_add_token(rule_line, TK_A_OWNER_ID, op, NULL, UID_TO_PTR(uid), /* is_case_insensitive = */ false, token_str); else if (resolve_name_timing == RESOLVE_NAME_EARLY && rule_get_substitution_type(value) == SUBST_TYPE_PLAIN) { + r = rule_resolve_user(rule_line, value, &uid); if (r < 0) - return log_line_error_errno(rule_line, r, "Failed to resolve user name '%s': %m", value); + return r; r = rule_line_add_token(rule_line, TK_A_OWNER_ID, op, NULL, UID_TO_PTR(uid), /* is_case_insensitive = */ false, token_str); } else if (resolve_name_timing != RESOLVE_NAME_NEVER) { @@ -1080,9 +1071,10 @@ static int parse_token( r = rule_line_add_token(rule_line, TK_A_GROUP_ID, op, NULL, GID_TO_PTR(gid), /* is_case_insensitive = */ false, token_str); else if (resolve_name_timing == RESOLVE_NAME_EARLY && rule_get_substitution_type(value) == SUBST_TYPE_PLAIN) { + r = rule_resolve_group(rule_line, value, &gid); if (r < 0) - return log_line_error_errno(rule_line, r, "Failed to resolve group name '%s': %m", value); + return r; r = rule_line_add_token(rule_line, TK_A_GROUP_ID, op, NULL, GID_TO_PTR(gid), /* is_case_insensitive = */ false, token_str); } else if (resolve_name_timing != RESOLVE_NAME_NEVER) { From f5cdf9515aceca2e91f9a33b74267e0cf5a5b7e8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 23 Jan 2025 06:08:23 +0900 Subject: [PATCH 3/5] udev-rules: ignore non-system user/group in OWNER=/GROUP= Recently, we introduce 'clock' system group, and set it for rtc/ptp devices. See af96ccfc24bc4803078a46b4ef2cdeb5decdfbcd. However, if non-system group with the same name is already exist, previously the devices were owned by the non-system group. That may possibly happen on updating systemd. Let's avoid accidentally devices being owned by non-system user/group. --- src/udev/udev-rules.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 91a0d1f01a..f8afe923d8 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -42,6 +42,7 @@ #include "udev-trace.h" #include "udev-util.h" #include "udev-worker.h" +#include "uid-classification.h" #include "user-util.h" #include "virt.h" @@ -500,6 +501,9 @@ static int rule_resolve_user(UdevRuleLine *rule_line, const char *name, uid_t *r return log_line_error_errno(rule_line, r, "Unknown user '%s', ignoring.", name); if (r < 0) return log_line_error_errno(rule_line, r, "Failed to resolve user '%s', ignoring: %m", name); + if (!uid_is_system(uid)) + return log_line_error_errno(rule_line, SYNTHETIC_ERRNO(EINVAL), + "User '%s' is not a system user (UID="UID_FMT"), ignoring.", name, uid); n = strdup(name); if (!n) @@ -535,6 +539,9 @@ static int rule_resolve_group(UdevRuleLine *rule_line, const char *name, gid_t * return log_line_error_errno(rule_line, r, "Unknown group '%s', ignoring.", name); if (r < 0) return log_line_error_errno(rule_line, r, "Failed to resolve group '%s', ignoring: %m", name); + if (!gid_is_system(gid)) + return log_line_error_errno(rule_line, SYNTHETIC_ERRNO(EINVAL), + "Group '%s' is not a system group (GID="GID_FMT"), ignoring.", name, gid); n = strdup(name); if (!n) @@ -1038,9 +1045,13 @@ static int parse_token( op = OP_ASSIGN; } - if (parse_uid(value, &uid) >= 0) + if (parse_uid(value, &uid) >= 0) { + if (!uid_is_system(uid)) + return log_line_error_errno(rule_line, SYNTHETIC_ERRNO(EINVAL), + "UID="UID_FMT" is not in the system user range, ignoring.", uid); + r = rule_line_add_token(rule_line, TK_A_OWNER_ID, op, NULL, UID_TO_PTR(uid), /* is_case_insensitive = */ false, token_str); - else if (resolve_name_timing == RESOLVE_NAME_EARLY && + } else if (resolve_name_timing == RESOLVE_NAME_EARLY && rule_get_substitution_type(value) == SUBST_TYPE_PLAIN) { r = rule_resolve_user(rule_line, value, &uid); @@ -1067,9 +1078,13 @@ static int parse_token( op = OP_ASSIGN; } - if (parse_gid(value, &gid) >= 0) + if (parse_gid(value, &gid) >= 0) { + if (!gid_is_system(gid)) + return log_line_error_errno(rule_line, SYNTHETIC_ERRNO(EINVAL), + "GID="GID_FMT" is not in the system group range, ignoring.", gid); + r = rule_line_add_token(rule_line, TK_A_GROUP_ID, op, NULL, GID_TO_PTR(gid), /* is_case_insensitive = */ false, token_str); - else if (resolve_name_timing == RESOLVE_NAME_EARLY && + } else if (resolve_name_timing == RESOLVE_NAME_EARLY && rule_get_substitution_type(value) == SUBST_TYPE_PLAIN) { r = rule_resolve_group(rule_line, value, &gid); @@ -2656,9 +2671,10 @@ static int udev_rule_apply_token_to_event( if (!apply_format_value(event, token, owner, sizeof(owner), "user name")) return true; + uid_t uid; r = get_user_creds( &ow, - &event->uid, + &uid, /* ret_gid = */ NULL, /* ret_home = */ NULL, /* ret_shell = */ NULL, @@ -2667,8 +2683,12 @@ static int udev_rule_apply_token_to_event( log_event_error_errno(event, token, r, "Unknown user \"%s\", ignoring.", owner); else if (r < 0) log_event_error_errno(event, token, r, "Failed to resolve user \"%s\", ignoring: %m", owner); - else + else if (!uid_is_system(uid)) + log_event_error(event, token, "User \"%s\" is not a system user (UID="UID_FMT"), ignoring.", owner, uid); + else { + event->uid = uid; log_event_debug(event, token, "Set owner: %s(%u)", owner, event->uid); + } return true; } case TK_A_GROUP: { @@ -2684,13 +2704,18 @@ static int udev_rule_apply_token_to_event( if (!apply_format_value(event, token, group, sizeof(group), "group name")) return true; - r = get_group_creds(&gr, &event->gid, USER_CREDS_ALLOW_MISSING); + gid_t gid; + r = get_group_creds(&gr, &gid, USER_CREDS_ALLOW_MISSING); if (r == -ESRCH) log_event_error_errno(event, token, r, "Unknown group \"%s\", ignoring.", group); else if (r < 0) log_event_error_errno(event, token, r, "Failed to resolve group \"%s\", ignoring: %m", group); - else + else if (!gid_is_system(gid)) + log_event_error(event, token, "Group \"%s\" is not a system group (GID="GID_FMT"), ignoring.", group, gid); + else { + event->gid = gid; log_event_debug(event, token, "Set group: %s(%u)", group, event->gid); + } return true; } case TK_A_MODE: { From 02ec3dd4ef01d0882a735ded9c6395bf656a893f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 23 Jan 2025 09:13:38 +0900 Subject: [PATCH 4/5] test: add test cases for OWNER=/GROUP= with non-system user/group --- test/units/TEST-17-UDEV.11.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/units/TEST-17-UDEV.11.sh b/test/units/TEST-17-UDEV.11.sh index ac99a80b0f..d781db8d1e 100755 --- a/test/units/TEST-17-UDEV.11.sh +++ b/test/units/TEST-17-UDEV.11.sh @@ -292,11 +292,15 @@ test_syntax_error 'OWNER-="b"' 'Invalid operator for OWNER.' test_syntax_error 'OWNER!="b"' 'Invalid operator for OWNER.' test_syntax_error 'OWNER+="0"' "OWNER key takes '=' or ':=' operator, assuming '='." test_syntax_error 'OWNER=":nosuchuser:"' "Unknown user ':nosuchuser:', ignoring." +test_syntax_error 'OWNER="testuser"' "User 'testuser' is not a system user (UID=$(id -u testuser 2>/dev/null)), ignoring." +test_syntax_error 'OWNER="12345"' "UID=12345 is not in the system user range, ignoring." test_syntax_error 'GROUP{a}="b"' 'Invalid attribute for GROUP.' test_syntax_error 'GROUP-="b"' 'Invalid operator for GROUP.' test_syntax_error 'GROUP!="b"' 'Invalid operator for GROUP.' test_syntax_error 'GROUP+="0"' "GROUP key takes '=' or ':=' operator, assuming '='." test_syntax_error 'GROUP=":nosuchgroup:"' "Unknown group ':nosuchgroup:', ignoring." +test_syntax_error 'GROUP="testuser"' "Group 'testuser' is not a system group (GID=$(id -u testuser 2>/dev/null)), ignoring." +test_syntax_error 'GROUP="12345"' "GID=12345 is not in the system group range, ignoring." test_syntax_error 'MODE{a}="b"' 'Invalid attribute for MODE.' test_syntax_error 'MODE-="b"' 'Invalid operator for MODE.' test_syntax_error 'MODE!="b"' 'Invalid operator for MODE.' From 7e6786b7fbbabaca15ea34c725dbd087ff5eb6c8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 24 Jan 2025 02:33:05 +0900 Subject: [PATCH 5/5] NEWS: mention OWNER=/GROUP= in udev rules now refuses non-system user/group --- NEWS | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS b/NEWS index b6b1c80907..b763358513 100644 --- a/NEWS +++ b/NEWS @@ -33,6 +33,12 @@ CHANGES WITH 258 in spe: an incompatible change of sorts, since per-user services will typically not be available for such PAM sessions of system users. + * systemd-udevd ignores OWNER=/GROUP= settings with a non-system + user/group specified in udev rules files, to avoid device nodes being + owned by a non-system user/group. It is recommended to check udev + rules files with 'udevadm verify' and/or 'udevadm test' commands if + the specified user/group in OWNER=/GROUP= are valid. + Announcements of Future Feature Removals: * The D-Bus method org.freedesktop.systemd1.StartAuxiliaryScope() is