ioctl socket fixes, sd-bus error updates, resolved error addition, PAM stub process priv fix#4299
Conversation
poettering
changed the title
src/core/execute.c
Outdated
| * If this fails, ignore the error - but expect sd-pam threads | ||
| * to fail to exit normally */ | ||
|
|
||
| if (setgroups(0, NULL) < 0) |
There was a problem hiding this comment.
indeed! (my PR predates the merging of the PR that introduced that)
| if (setgroups(0, NULL) < 0) | ||
| log_warning_errno(errno, "Failed to setgroups() in sd-pam: %m"); | ||
| if (setresgid(gid, gid, gid) < 0) | ||
| log_warning_errno(errno, "Failed to setresgid() in sd-pam: %m"); |
There was a problem hiding this comment.
setresgid doesn't respect the SupplementaryGroups (enforce_groups respects)
There was a problem hiding this comment.
what do you mean? I think it's totally fine if the auxiliary group list is empty in this case. We don't really care for the full cred initialiuation here. We only drop privs because we can here, because there's no need to stay root in the PAM stub.
Note that we don't even check whether the dropping of privs really worked, because it doesn't matter too much: if we remain privileged here, it's not a complete loss...
There was a problem hiding this comment.
oh, sorry, I mean there is a little difference. This doesn't really matter. Thanks!
|
Will push an updated version shortly that, fixes the maybe_setgroups() thing. Thanks for the reviews! |
|
OK, i force pushed a new version, that calls maye_setgroups() instead of setgroups() now, as suggested by @evverx. I also added another new commit that rewokrs maybe_setgroups() a bit, and drops its caching. It also normalizes the error handling of it, to match the way we usually do it in our tree. |
|
@poettering LGTM, and I've learned a few more tricks about systemd hacking looking at it. |
…tls on As suggested here: systemd#4296 (comment) Let's try AF_INET first as socket, but let's fall back to AF_NETLINK, so that we can use a protocol-independent socket here if possible. This has the benefit that our code will still work even if AF_INET/AF_INET6 is made unavailable (for exmple via seccomp), at least on current kernels.
These were forgotten, let's add some useful mappings for all errors we define.
Add this new error code (documented in RFC7873) to our list of known errors.
We generate these, hence we should also add errno translations for them.
In the process execution code of PID 1, before 096424d the GID settings where changed before invoking PAM, and the UID settings after. After the change both changes are made after the PAM session hooks are run. When invoking PAM we fork once, and leave a stub process around which will invoke the PAM session end hooks when the session goes away. This code previously was dropping the remaining privs (which were precisely the UID). Fix this code to do this correctly again, by really dropping them else (i.e. the GID as well). While we are at it, also fix error logging of this code. Fixes: systemd#4238
gcc at some optimization levels thinks thes variables were used without initialization. it's wrong, but let's make the message go anyway.
Let's drop the caching of the setgroups /proc field for now. While there's a strict regime in place when it changes states, let's better not cache it since we cannot really be sure we follow that regime correctly. More importantly however, this is not in performance sensitive code, and there's no indication the cache is really beneficial, hence let's drop the caching and make things a bit simpler. Also, while we are at it, rework the error handling a bit, and always return negative errno-style error codes, following our usual coding style. This has the benefit that we can sensible hanld read_one_line_file() errors, without having to updat errno explicitly.
|
force pushed new version with the kernel version comment fix as suggested by @evverx applied. please have a look |
4 participants
Some minor fixes. @yuwata, @evverx please have a look!