Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Mar 27, 2018
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
117 changes: 59 additions & 58 deletions src/core/execute.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert back to the old uid/gid in the error path. This isn't too crucial, but I think we should try to be hygienic there. Maybe add a clean-up label to the bottom that resets the uid/gids before returning the error.

if (errno == ENOSYS)
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should reset r to 0 here for the cases where we ignore the error... otherwise its value will be undefined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, r was never set in all successful branches either, thus initialized it to zero at the very top of the function. Repushed.

}

/* 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;

Expand All @@ -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]) {
Expand Down