Shut down forked PAM handle #520
Conversation
Xyene
force-pushed
the
pam-cleanup
branch
2 times, most recently
from
e79ed86
to
2975c2a
Compare
|
|
||
| #ifdef PAM_DATA_SILENT | ||
| /* macOS PAM doesn't support PAM_DATA_SILENT. */ | ||
| pam_end(sshpam_handle, PAM_SUCCESS | PAM_DATA_SILENT); |
There was a problem hiding this comment.
| pam_end(sshpam_handle, PAM_SUCCESS | PAM_DATA_SILENT); | |
| pam_end(sshpam_handle, sshpam_err | PAM_DATA_SILENT); |
Shouldn't be still be ended even outside pam-linux?
There was a problem hiding this comment.
I'm not sure it's safe to do so without PAM_DATA_SILENT, otherwise pam_end may result in modules in the forked child trying to perform cleanup work meant for the parent (deleting directories, logging, etc.)
A real example, from the pam_krb5 module: shadow-maint/shadow#408 (comment) / rra/pam-krb5#21
There was a problem hiding this comment.
If the forked process is going to be run, I would expect sshpam_err to always be PAM_SUCCESS. When one of the pam_authenticate() etc calls fail to return PAM_SUCCESS, I would have expected this fork to not have happened.
+1 to @Xyene - this does not change the need for the parent to call pam_end(sshpam_err) and that call should not have PAM_DATA_SILENT or'd in.
Xyene
force-pushed
the
pam-cleanup
branch
3 times, most recently
from
0825cd0
to
07bd8b8
Compare
Shutting down the forked PAM handle must be done as per the Linux-PAM documentation, specifically after setuid. This is not a new requirement, but until recently (~2021) there was no consequence to not doing so. `pam_cap` now requires the handle to be shut down correctly in order to configure ambient capabilities for the session. Importantly, these must be configured after setuid, as setuid clears the ambient capability vector. Signed-off-by: Tudor Brindus <[email protected]>
|
I moved the cleanup to after |
Shutting down the forked PAM handle must be done as per the Linux-PAM documentation, specifically after setuid.
https://fossies.org/linux/Linux-PAM-docs/doc/adg/html/adg-interface-by-app-expected.html#adg-pam_end
This is not a new requirement, but until recently (~2021) there was no consequence to not doing so.
pam_capnow requires the handle to be shut down correctly in order to configure ambient capabilities for the session. Importantly, these must be configured after setuid, as setuid clears the ambient capability vector.See this comment (and thread) for more details: shadow-maint/shadow#408 (comment)