udev-rules: downgrade error about non-system user/group in OWNER=/GROUP=

This reverts (in sprit) commit f5cdf9515aceca2e91f9a33b74267e0cf5a5b7e8,
"udev-rules: ignore non-system user/group in OWNER=/GROUP=".

The original change was done to clean up a situation where we added a
new group, but the group could already have been used for some other
purposes, and now the some unexpected entity would own the device.
Unfortunately, this check doesn't really address the issue, since the
existing account might as well be a system account, which might be
equally bad. In addition, this change is a big compatiblity break,
causing existing rules to stop working. Since quite a lot of systems
have local configuration to assign devices to users for various
purposes, this is very noticable to users. In a way, the original change
to add a new group was the compat break, and follow-up patch to cahnge
the rule parsing evolved a small compat break into a much bigger one.
There is merit to the change though, since device nodes shouldn't be
owned by users and groups and different mechanisms should be used
instead. To avoid breaking users systems, and since the original goal
cannot be achieved by this patch, let's downgrade this to a warning
to guide users towards different solutions.

diff --git a/NEWS b/NEWS
index 06c01d1c9cc3534cc7bacf9dc0879609a7fce413..6ce07c82cb5c8c9e858ab7ab1276294b21c53413 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -669,8 +669,8 @@ CHANGES WITH 258:
           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
+        * systemd-udevd warns about OWNER=/GROUP= settings with a non-system
+          user/group specified in udev rules files. Device nodes should not be
           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.

diff --git a/catalog/systemd.catalog.in b/catalog/systemd.catalog.in
index 2d926d8caee76928bf8a20ff45bdecdb56078626..911254ceb92f265cb878951e3a51e928276f3a15 100644 (file)
--- a/catalog/systemd.catalog.in
+++ b/catalog/systemd.catalog.in
@@ -984,3 +984,21 @@ Documentation: man:systemd(1)
 An unsuccessful attempt has been made to break an ordering cycle between units
 for which jobs have been enqueued as part of a transaction. The transaction
 will fail.
+
+-- 3405205d368e49feb5ab3925fee13874
+Subject: Non-system user or group used for device ownership
+Defined-By: systemd
+Support: %SUPPORT_URL%
+Documentation: man:systemd(1) systemd-udevd(8)
+
+The ownership of a device managed by systemd-udevd is assigned to a "regular"
+(non-system) user or group. This is currently allowed for compatibility, but is
+deprecated and discouraged. Ownership of a device node grants the privileges to
+change ACLs, the group, access mode, or set labels or extended attributes,
+which creates a conflict of management, because both udev and the user are in
+power to change these attributes. In addition, device nodes appear early in
+boot, while regular users may appear only later.
+
+For devices managed by systemd-udevd, it is instead recommended to use the
+"uaccess"/"xaccess" mechanisms to grant limited and temporary access to device
+nodes, see sd-login(8).

diff --git a/src/systemd/sd-messages.h b/src/systemd/sd-messages.h
index 6dc7dd527a09a1b144620bc135c70a98fd6ea01b..5465db8b1311f828f85aa7622c3c8476fd55e443 100644 (file)
--- a/src/systemd/sd-messages.h
+++ b/src/systemd/sd-messages.h
@@ -303,6 +303,9 @@ _SD_BEGIN_DECLARATIONS;
 #define SD_MESSAGE_CANT_BREAK_ORDERING_CYCLE          SD_ID128_MAKE(b3,11,2d,da,d1,90,45,53,8c,76,68,5b,a5,91,8a,80)
 #define SD_MESSAGE_CANT_BREAK_ORDERING_CYCLE_STR      SD_ID128_MAKE_STR(b3,11,2d,da,d1,90,45,53,8c,76,68,5b,a5,91,8a,80)
 
+#define SD_MESSAGE_SYSTEM_ACCOUNT_REQUIRED            SD_ID128_MAKE(34,05,20,5d,36,8e,49,fe,b5,ab,39,25,fe,e1,38,74)
+#define SD_MESSAGE_SYSTEM_ACCOUNT_REQUIRED_STR        SD_ID128_MAKE_STR(34,05,20,5d,36,8e,49,fe,b5,ab,39,25,fe,e1,38,74)
+
 _SD_END_DECLARATIONS;
 
 #endif

diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c
index 6213a7e58f7f3f06fbbb831df22c4147917e91a3..06686e3715e83df3904c35298a2ade2820680a53 100644 (file)
--- a/src/udev/udev-rules.c
+++ b/src/udev/udev-rules.c
@@ -5,6 +5,7 @@
 #include <unistd.h>
 
 #include "sd-json.h"
+#include "sd-messages.h"
 
 #include "alloc-util.h"
 #include "architecture.h"
@@ -50,6 +51,7 @@
 #include "udev-trace.h"
 #include "udev-util.h"
 #include "udev-worker.h"
+#include "uid-classification.h"
 #include "user-record.h"
 #include "user-util.h"
 #include "userdb.h"
@@ -226,6 +228,26 @@ static bool token_is_for_parents(UdevRuleToken *token) {
 
 /*** Logging helpers ***/
 
+#define log_udev_event_syntax(event, token, level, message_id, fmt, ...) \
+        ({                                                              \
+                UdevEvent *_event = (event);                            \
+                UdevRuleToken *_token = ASSERT_PTR(token);              \
+                int _level = (level);                                   \
+                sd_device *_d = token_is_for_parents(_token) ? _event->dev_parent : _event->dev; \
+                const char *_sysname = NULL;                            \
+                                                                        \
+                if (_d && log_get_max_level() >= LOG_PRI(_level))       \
+                        (void) sd_device_get_sysname(_d, &_sysname);    \
+                log_struct(_level,                                      \
+                           LOG_MESSAGE("%s:%u %s:" fmt,                 \
+                                       strna(_token->rule_line->rule_file ? _token->rule_line->rule_file->filename : NULL), \
+                                       _token->rule_line->line_number,  \
+                                       _token->token_str,               \
+                                       __VA_ARGS__),                    \
+                           LOG_MESSAGE_ID(message_id),                  \
+                           LOG_ITEM("DEVICE=%s", strempty(_sysname)));  \
+        })
+
 #define _log_udev_rule_file_full(device, device_u, file, file_u, line_nr, level, level_u, error, fmt, ...) \
         ({                                                              \
                 int level_u = (level);                                  \
@@ -509,13 +531,19 @@ static int rule_resolve_user(UdevRuleLine *rule_line, const char *name, uid_t *r
         }
 
         _cleanup_(user_record_unrefp) UserRecord *ur = NULL;
-        r = userdb_by_name(name, &USERDB_MATCH_ROOT_AND_SYSTEM,
+        r = userdb_by_name(name, /* match= */ NULL,
                            USERDB_SUPPRESS_SHADOW | USERDB_PARSE_NUMERIC | USERDB_SYNTHESIZE_NUMERIC,
                            &ur);
         if (r < 0)
                 return log_line_error_errno(rule_line, r,
                                             "Failed to resolve user '%s', ignoring: %s",
                                             name, STRERROR_USER(r));
+        if (!uid_is_system(ur->uid))
+                log_struct(LOG_WARNING,
+                           LOG_MESSAGE("%s:%u User %s configured to own a device node is not a system user. "
+                                       "Support for device node ownership by non-system accounts is deprecated and will be removed in the future.",
+                                       rule_line->rule_file->filename, rule_line->line_number, name),
+                           LOG_MESSAGE_ID(SD_MESSAGE_SYSTEM_ACCOUNT_REQUIRED_STR));
 
         _cleanup_free_ char *n = strdup(name);
         if (!n)
@@ -544,13 +572,19 @@ static int rule_resolve_group(UdevRuleLine *rule_line, const char *name, gid_t *
         }
 
         _cleanup_(group_record_unrefp) GroupRecord *gr = NULL;
-        r = groupdb_by_name(name, &USERDB_MATCH_ROOT_AND_SYSTEM,
+        r = groupdb_by_name(name, /* match= */ NULL,
                             USERDB_SUPPRESS_SHADOW | USERDB_PARSE_NUMERIC | USERDB_SYNTHESIZE_NUMERIC,
                             &gr);
         if (r < 0)
                 return log_line_error_errno(rule_line, r,
                                             "Failed to resolve group '%s', ignoring: %s",
                                             name, STRERROR_GROUP(r));
+        if (!gid_is_system(gr->gid))
+                log_struct(LOG_WARNING,
+                           LOG_MESSAGE("%s:%u Group %s configured to own a device node is not a system group. "
+                                       "Support for device node ownership by non-system accounts is deprecated and will be removed in the future.",
+                                       rule_line->rule_file->filename, rule_line->line_number, name),
+                           LOG_MESSAGE_ID(SD_MESSAGE_SYSTEM_ACCOUNT_REQUIRED_STR));
 
         _cleanup_free_ char *n = strdup(name);
         if (!n)
@@ -2674,7 +2708,7 @@ static int udev_rule_apply_token_to_event(
                         return true;
 
                 _cleanup_(user_record_unrefp) UserRecord *ur = NULL;
-                r = userdb_by_name(owner, &USERDB_MATCH_ROOT_AND_SYSTEM,
+                r = userdb_by_name(owner, /* match= */ NULL,
                                    USERDB_SUPPRESS_SHADOW | USERDB_PARSE_NUMERIC | USERDB_SYNTHESIZE_NUMERIC,
                                    &ur);
                 if (r < 0)
@@ -2682,6 +2716,13 @@ static int udev_rule_apply_token_to_event(
                                               "Failed to resolve user \"%s\", ignoring: %s",
                                               owner, STRERROR_USER(r));
                 else {
+                        if (!uid_is_system(ur->uid))
+                                log_udev_event_syntax(event, token, LOG_WARNING,
+                                                      SD_MESSAGE_SYSTEM_ACCOUNT_REQUIRED_STR,
+                                                      "User %s configured to own a device node is not a system user. "
+                                                      "Support for device node ownership by non-system accounts is deprecated and will be removed in the future.",
+                                                      owner);
+
                         event->uid = ur->uid;
                         log_event_debug(event, token, "Set owner: %s("UID_FMT")", owner, event->uid);
                 }
@@ -2700,7 +2741,7 @@ static int udev_rule_apply_token_to_event(
                         return true;
 
                 _cleanup_(group_record_unrefp) GroupRecord *gr = NULL;
-                r = groupdb_by_name(group, &USERDB_MATCH_ROOT_AND_SYSTEM,
+                r = groupdb_by_name(group, /* match= */ NULL,
                                     USERDB_SUPPRESS_SHADOW | USERDB_PARSE_NUMERIC | USERDB_SYNTHESIZE_NUMERIC,
                                     &gr);
                 if (r < 0)
@@ -2708,6 +2749,13 @@ static int udev_rule_apply_token_to_event(
                                               "Failed to resolve group \"%s\", ignoring: %s",
                                               group, STRERROR_GROUP(r));
                 else {
+                        if (!gid_is_system(gr->gid))
+                                log_udev_event_syntax(event, token, LOG_WARNING,
+                                                      SD_MESSAGE_SYSTEM_ACCOUNT_REQUIRED_STR,
+                                                      "Group %s configured to own a device node is not a system group. "
+                                                      "Support for device node ownership by non-system accounts is deprecated and will be removed in the future.",
+                                                      group);
+
                         event->gid = gr->gid;
                         log_event_debug(event, token, "Set group: %s("GID_FMT")", group, event->gid);
                 }

diff --git a/test/units/TEST-17-UDEV.verify.sh b/test/units/TEST-17-UDEV.verify.sh
index 66f34a3e36f94f524b3410a2867198b16eaec4cb..66ab257be8a59067674b96027efbae3960b2c71c 100755 (executable)
--- a/test/units/TEST-17-UDEV.verify.sh
+++ b/test/units/TEST-17-UDEV.verify.sh
@@ -315,8 +315,10 @@ if ! getent passwd 12345 >/dev/null; then
 fi
 # regular user
 if getent passwd testuser >/dev/null; then
-    test_syntax_error 'OWNER="testuser"' "Failed to resolve user 'testuser', ignoring: Not a system user"
-    test_syntax_error "OWNER=\"$(id -u testuser)\"" "Failed to resolve user '$(id -u testuser)', ignoring: Not a system user"
+    echo 'OWNER="testuser"' >"${rules}"
+    udevadm verify "${rules}"
+    echo "OWNER=\"$(id -u testuser)\"" >"${rules}"
+    udevadm verify "${rules}"
 fi
 test_syntax_error 'GROUP{a}="b"' 'Invalid attribute for GROUP.'
 test_syntax_error 'GROUP-="b"' 'Invalid operator for GROUP.'
@@ -347,8 +349,11 @@ if ! getent group 12345 >/dev/null; then
 fi
 # regular group
 if getent group testuser >/dev/null; then
-    test_syntax_error 'GROUP="testuser"' "Failed to resolve group 'testuser', ignoring: Not a system group"
-    test_syntax_error "GROUP=\"$(id -g testuser)\"" "Failed to resolve group '$(id -g testuser)', ignoring: Not a system group"
+    echo 'GROUP="testuser"' >"${rules}"
+    udevadm verify "${rules}"
+
+    echo "GROUP=\"$(id -g testuser)\"" >"${rules}"
+    udevadm verify "${rules}"
 fi
 test_syntax_error 'MODE{a}="b"' 'Invalid attribute for MODE.'
 test_syntax_error 'MODE-="b"' 'Invalid operator for MODE.'