Created attachment 3445 [details] pam: Correctly handle errors from pam_acct_mgmt The fix for a bug #1188 introduced an unconditional override of return value from pam_acct_mgmt(), setting PAM_ACCT_EXPIRED on any error from account step. It could have been 15 years ago, when there were not any other reasons why this function could fail, but these days, there are at least PAM_USER_UNKNOWN and PAM_PERM_DENIED (from Fedora 32 man pages). In these cases, openssh goes into unexpected code paths giving confusing error messages, such as: pam_sss(sshd:auth): authentication success; logname= uid=0 euid=0 tty=ssh ruser= rhost=client user=useruser debug1: do_pam_account: called pam_sss(sshd:account): Access denied for user useruser: 6 (Permission denied) debug3: PAM: do_pam_account pam_acct_mgmt = 6 (Permission denied) debug3: ssh_msg_send: type 13 debug3: PAM: User account has expired error: PAM: User account has expired for useruser from client As far as I read that, I do not see any recovery from either of these errors worth retrying so I believe we should be fine handling them the same as expired account though. The attached is proposed patch, which should handle this use case as well as the original issue in #1188. Tested in Fedora 32 with pam_debug.so with respective return values as well as with expired user.
FYI, it looks like the countermeasures against retrying the pam again are now implemented in the privilege separation and my tests end with the following errors: debug3: monitor_read: checking request 104 fatal: monitor_read: unpermitted request 104 so the patch might be simplified as it is no longer possible to call sshpam_init_ctx() at all if I follow the code and logs correctly.
Created attachment 3545 [details] Alternate proposal to preserve pam_acct_mgmt() return value I'm adding a slightly different proposed patch, that I believe is both more generic, in that all error values from pam_acct_mgmt() can be passed through. It is also slightly smaller a change. Unlike the prior patch it intentionally overrides a PAM_SUCCESS sshpam_err value with the one from pam_acct_mgmt(). This is so that an account that has successfully authenticate but for some other reason is not allowed access "just now" has an appropriate error returned (likely PAM_PERM_DENIED).
Comment on attachment 3545 [details] Alternate proposal to preserve pam_acct_mgmt() return value I think I prefer this approach, however > sshpam_err = do_pam_account(); [...] >- return (sshpam_account_status); >+ return (sshpam_err); do_pam_account returns u_int but sshpam_err is and pam_account_mgmt returns int. (sshpam_account_status is also an int but it's only used if it's set to 0 or 1)