Bug 3210

Summary:

Confusing errors when pam_acct_mgmt() fails

Product:

Portable OpenSSH

Reporter:

Jakub Jelen <jjelen>

Component:

PAM support

Assignee:

Assigned to nobody <unassigned-bugs>

Status:

NEW ---

Severity:

enhancement

CC:

darren.moffat, dtucker

Priority:

Version:

8.3p1

Hardware:

Other

OS:

Linux

Bug Depends on:

Bug Blocks:

3549

Attachments:

Description	Flags
pam: Correctly handle errors from pam_acct_mgmt	none
Alternate proposal to preserve pam_acct_mgmt() return value	none

Description Jakub Jelen 2020-09-08 21:18:20 AEST

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.

Comment 1 Jakub Jelen 2020-09-08 21:43:50 AEST

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.

Comment 2 Darren Moffat 2021-08-25 22:03:46 AEST

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 3 Darren Tucker 2021-09-13 16:28:22 AEST

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)