Allow pam stack to call ReleaseSession #32869
Conversation
github-actions
bot
added
login
documentation
please-review
|
Note We had successfully released a new major release. We are no longer in a development freeze phase. |
|
Hmm, according to #8598, the fix might be in the wrong direction? I.e. rather than allowing to call I think a bigger yet comprehensive rework should be the way to go (#31033 (comment)), but noone has yet reviewed the PR (including me, sorry, completely forgot about it). |
|
cc @yuwata |
| if (r < 0) | ||
| return r; | ||
|
|
||
| if (session != sender_session) |
There was a problem hiding this comment.
This means only the owner can call ReleaseSession now, doesn't it?
There was a problem hiding this comment.
Yes. But I think that's appropriate. ReleaseSession and friends are documented as: "These calls should never be invoked directly by clients. Creating/closing sessions is exclusively the job of PAM and its pam_systemd(8) module."
Yes, this PR doesn't implement that part of the request from #8598. It was targeted at #28514 and fixes that issue. I like that it's a simple solution. It also partially fixes the problems described in the other ticket, but doesn't solve all of them, because that'd require running the code at elevated privilege level. I know that there are reasons to do this, but it seems a bit iffy too. |
|
If #31033 is merged, then we won't need this PR. So let's put it on hold for now. |
keszybz
added
postponed
and removed
please-review
Actually that one looks a lot more complex, and it's still in draft too, so doesn't seem like a good candidate for v256. This one is much smaller, and seems like we could take it for v256, as it fixes an actual problem. Then the other one can be reworked on top of this for v257? |
|
I think this would be reasonable. I don't think we should merge #31033 at this point in the release. It is mostly likely a better solution, but it's also a very risky change. So yeah, I think we should merge this one here to solve the most visible issue. |
…ession Fixes systemd#28514. Quoting systemd#28514 (comment): > Whenever PAM is enabled for a service, we set up the PAM session and then > fork off a process whose only job is to eventually close the PAM session when > the service dies. That services we run with service privileges, both to > minimize attack surface and because we want to use PR_SET_DEATHSIG to be get > a notification via signal whenever the main process dies. But that only works > if we have the same credentials as that main process. > > Now, if pam_systemd runs inside the PAM stack (which it normally does) it's > session close hook will ask logind to synchronously end the session via a bus > call. Currently that call is not accessible to unprivileged clients. And > that's the part we need to relax: allow users to end their own sessions. The check is implemented in a way that allows the kill if the sender is in the target session. I found 'sudo systemctl --user -M "zbyszek@" is-system-running' to be a convenient reproducer. Before: May 16 16:25:26 x1c systemd[1]: run-u24754.service: Deactivated successfully. May 16 16:25:26 x1c dbus-broker[1489]: A security policy denied :1.24757 to send method call /org/freedesktop/login1:org.freedesktop.login1.Manager.ReleaseSession to org.freedesktop.login1. May 16 16:25:26 x1c (sd-pam)[3036470]: pam_systemd(login:session): Failed to release session: Access denied May 16 16:25:26 x1c systemd[1]: Stopping session-114.scope... May 16 16:25:26 x1c systemd[1]: session-114.scope: Deactivated successfully. May 16 16:25:26 x1c systemd[1]: Stopped session-114.scope. May 16 16:25:26 x1c systemd[1]: session-c151.scope: Deactivated successfully. May 16 16:25:26 x1c systemd-logind[1513]: Session c151 logged out. Waiting for processes to exit. May 16 16:25:26 x1c systemd-logind[1513]: Removed session c151. After: May 16 17:02:15 x1c systemd[1]: run-u24770.service: Deactivated successfully. May 16 17:02:15 x1c systemd[1]: Stopping session-115.scope... May 16 17:02:15 x1c systemd[1]: session-c153.scope: Deactivated successfully. May 16 17:02:15 x1c systemd[1]: session-115.scope: Deactivated successfully. May 16 17:02:15 x1c systemd[1]: Stopped session-115.scope. May 16 17:02:15 x1c systemd-logind[1513]: Session c153 logged out. Waiting for processes to exit. May 16 17:02:15 x1c systemd-logind[1513]: Removed session c153. Edit: this seems to also fix systemd#8598. It seems that with the call to ReleaseSession, we wait for the pam session close hooks to finish. I inserted a 'sleep(10)' after the call to ReleaseSession in pam_systemd, and things block on that, nothing is killed prematurely.
bluca
force-pushed
the
dbus-release-session
branch
from
ffd2ae3
to
fc0bb7c
Compare
github-actions
bot
added
the
please-review
bluca
added
good-to-merge/waiting-for-ci 👍
|
mkosi / ci (opensuse, tumbleweed) is busted because of git sha256. |
keszybz
added
ci-failure-appears-unrelated
and removed
good-to-merge/waiting-for-ci 👍
| return r; | ||
|
|
||
| if (session != sender_session) | ||
| return sd_bus_error_set(error, BUS_ERROR_NOT_IN_CONTROL, "You are not in control of this session"); |
There was a problem hiding this comment.
hmm, please define a new error for this. the NotInControl error is really about session controllers, but this here really is different. Please add a new error.
Or just return SD_BUS_ERROR_ACCESS_DENIED or so.
As requested in post-merge review systemd#32869 (review): > NotInControl error is really about session controllers, but this here really > is different.
As requested in post-merge review systemd#32869 (review): > NotInControl error is really about session controllers, but this here really > is different.
As requested in post-merge review systemd#32869 (review): > NotInControl error is really about session controllers, but this here really > is different.
As requested in post-merge review #32869 (review): > NotInControl error is really about session controllers, but this here really > is different.
No description provided.