core: use setreuid/setregid trick to create session keyring with right ownership...

Re-use the hacks used to link user keyring, when creating the session
keyring. This way changing ownership of the keyring is not required, and thus
incovation_id can be correctly created in restricted environments.

Creating invocation_id with root permissions works and linking it into session
keyring works, as at that point session keyring is possessed.

Simple way to validate this is with following commands:

$ journalctl -f &
$ sudo systemd-run --uid 1000 /bin/sh -c 'keyctl describe @s; keyctl list @s; keyctl read `keyctl search @s user invocation_id`'

which now works in LXD containers as well as on the host.

Fixes: https://github.com/systemd/systemd/issues/7655

diff --git a/src/core/execute.c b/src/core/execute.c
index 56e0eec..664a3b9 100644 (file)
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -2434,7 +2434,9 @@ static int setup_keyring(
                 uid_t uid, gid_t gid) {
 
         key_serial_t keyring;
-        int r;
+        int r = 0;
+        uid_t saved_uid;
+        gid_t saved_gid;
 
         assert(u);
         assert(context);
@@ -2453,6 +2455,26 @@ static int setup_keyring(
         if (context->keyring_mode == EXEC_KEYRING_INHERIT)
                 return 0;
 
+        /* Acquiring a reference to the user keyring is nasty. We briefly change identity in order to get things set up
+         * properly by the kernel. If we don't do that then we can't create it atomically, and that sucks for parallel
+         * execution. This mimics what pam_keyinit does, too. Setting up session keyring, to be owned by the right user
+         * & group is just as nasty as acquiring a reference to the user keyring. */
+
+        saved_uid = getuid();
+        saved_gid = getgid();
+
+        if (gid_is_valid(gid) && gid != saved_gid) {
+                if (setregid(gid, -1) < 0)
+                        return log_unit_error_errno(u, errno, "Failed to change GID for user keyring: %m");
+        }
+
+        if (uid_is_valid(uid) && uid != saved_uid) {
+                if (setreuid(uid, -1) < 0) {
+                        r = log_unit_error_errno(u, errno, "Failed to change UID for user keyring: %m");
+                        goto out;
+                }
+        }
+
         keyring = keyctl(KEYCTL_JOIN_SESSION_KEYRING, 0, 0, 0, 0);
         if (keyring == -1) {
                 if (errno == ENOSYS)
@@ -2462,12 +2484,36 @@ static int setup_keyring(
                 else if (errno == EDQUOT)
                         log_unit_debug_errno(u, errno, "Out of kernel keyrings to allocate, ignoring.");
                 else
-                        return log_unit_error_errno(u, errno, "Setting up kernel keyring failed: %m");
+                        r = log_unit_error_errno(u, errno, "Setting up kernel keyring failed: %m");
 
-                return 0;
+                goto out;
         }
 
-        /* Populate they keyring with the invocation ID by default. */
+        /* When requested link the user keyring into the session keyring. */
+        if (context->keyring_mode == EXEC_KEYRING_SHARED) {
+
+                if (keyctl(KEYCTL_LINK,
+                           KEY_SPEC_USER_KEYRING,
+                           KEY_SPEC_SESSION_KEYRING, 0, 0) < 0) {
+                        r = log_unit_error_errno(u, errno, "Failed to link user keyring into session keyring: %m");
+                        goto out;
+                }
+        }
+
+        /* Restore uid/gid back */
+        if (uid_is_valid(uid) && uid != saved_uid) {
+                if (setreuid(saved_uid, -1) < 0) {
+                        r = log_unit_error_errno(u, errno, "Failed to change UID back for user keyring: %m");
+                        goto out;
+                }
+        }
+
+        if (gid_is_valid(gid) && gid != saved_gid) {
+                if (setregid(saved_gid, -1) < 0)
+                        return log_unit_error_errno(u, errno, "Failed to change GID back for user keyring: %m");
+        }
+
+        /* Populate they keyring with the invocation ID by default, as original saved_uid. */
         if (!sd_id128_is_null(u->invocation_id)) {
                 key_serial_t key;
 
@@ -2478,65 +2524,20 @@ static int setup_keyring(
                         if (keyctl(KEYCTL_SETPERM, key,
                                    KEY_POS_VIEW|KEY_POS_READ|KEY_POS_SEARCH|
                                    KEY_USR_VIEW|KEY_USR_READ|KEY_USR_SEARCH, 0, 0) < 0)
-                                return log_unit_error_errno(u, errno, "Failed to restrict invocation ID permission: %m");
+                                r = log_unit_error_errno(u, errno, "Failed to restrict invocation ID permission: %m");
                 }
         }
 
-        /* And now, make the keyring owned by the service's user */
-        if (uid_is_valid(uid) || gid_is_valid(gid))
-                if (keyctl(KEYCTL_CHOWN, keyring, uid, gid, 0) < 0)
-                        return log_unit_error_errno(u, errno, "Failed to change ownership of session keyring: %m");
-
-        /* When requested link the user keyring into the session keyring. */
-        if (context->keyring_mode == EXEC_KEYRING_SHARED) {
-                uid_t saved_uid;
-                gid_t saved_gid;
+out:
+        /* Revert back uid & gid for the the last time, and exit */
+        /* no extra logging, as only the first already reported error matters */
+        if (getuid() != saved_uid)
+                (void) setreuid(saved_uid, -1);
 
-                /* Acquiring a reference to the user keyring is nasty. We briefly change identity in order to get things
-                 * set up properly by the kernel. If we don't do that then we can't create it atomically, and that
-                 * sucks for parallel execution. This mimics what pam_keyinit does, too.*/
+        if (getgid() != saved_gid)
+                (void) setregid(saved_gid, -1);
 
-                saved_uid = getuid();
-                saved_gid = getgid();
-
-                if (gid_is_valid(gid) && gid != saved_gid) {
-                        if (setregid(gid, -1) < 0)
-                                return log_unit_error_errno(u, errno, "Failed to change GID for user keyring: %m");
-                }
-
-                if (uid_is_valid(uid) && uid != saved_uid) {
-                        if (setreuid(uid, -1) < 0) {
-                                (void) setregid(saved_gid, -1);
-                                return log_unit_error_errno(u, errno, "Failed to change UID for user keyring: %m");
-                        }
-                }
-
-                if (keyctl(KEYCTL_LINK,
-                           KEY_SPEC_USER_KEYRING,
-                           KEY_SPEC_SESSION_KEYRING, 0, 0) < 0) {
-
-                        r = -errno;
-
-                        (void) setreuid(saved_uid, -1);
-                        (void) setregid(saved_gid, -1);
-
-                        return log_unit_error_errno(u, r, "Failed to link user keyring into session keyring: %m");
-                }
-
-                if (uid_is_valid(uid) && uid != saved_uid) {
-                        if (setreuid(saved_uid, -1) < 0) {
-                                (void) setregid(saved_gid, -1);
-                                return log_unit_error_errno(u, errno, "Failed to change UID back for user keyring: %m");
-                        }
-                }
-
-                if (gid_is_valid(gid) && gid != saved_gid) {
-                        if (setregid(saved_gid, -1) < 0)
-                                return log_unit_error_errno(u, errno, "Failed to change GID back for user keyring: %m");
-                }
-        }
-
-        return 0;
+        return r;
 }
 
 static void append_socket_pair(int *array, unsigned *n, const int pair[2]) {