Bug 1188

Summary: keyboard-interactive should not allow retry after pam_acct_mgmt fails
Product: Portable OpenSSH Reporter: Darren Tucker <dtucker>
Component: PAM supportAssignee: Assigned to nobody <unassigned-bugs>
Status: CLOSED FIXED    
Severity: normal    
Priority: P2    
Version: -current   
Hardware: Other   
OS: All   
Bug Depends on:    
Bug Blocks: 1155    
Attachments:
Description Flags
prevent retry of keyboard-interactive if PAM account check fails. djm: ok+

Description Darren Tucker 2006-05-03 12:44:08 AEST 
Because each keyboard-interactive attempt is effectively self-contained, when the PAM account check fails, the user is reprompted, even though they can never possible succeed (since do_pam_account() caches the result).  Eg:

$ ssh localhost
Password:
Your account has expired; please contact your system administrator

Password:

sshd should prevent further keyboard-interactive attempts if the PAM account check fails.
Comment 1 Darren Tucker 2006-05-03 12:45:09 AEST 
Created attachment 1130 [details]
prevent retry of keyboard-interactive if PAM account check fails.
Comment 2 Damien Miller 2006-05-03 12:51:26 AEST 
Comment on attachment 1130 [details]
prevent retry of keyboard-interactive if PAM account check fails.

Looks OK to me as far as PAM goes, but you should update the copyright year while you are in there.

Would it make sense to use a global flag? so a PAM authorization failure blocks all other methods...
Comment 3 Darren Tucker 2006-05-03 13:06:37 AEST 
(In reply to comment #2)
> Would it make sense to use a global flag? so a PAM authorization
> failure blocks all other methods...

I don't think that's necessary.  do_pam_account() caches its result and it gets called in auth1.c or auth2.c.
Comment 4 Frank Cusack 2006-05-03 15:35:18 AEST 
PAM acct mgmt can fail for reasons other than password expiry.  The
patch looks like you assume this is the reason.  Also, if the account
IS expired, the user should be given a chance to update their password.
Comment 5 Darren Tucker 2006-05-03 15:57:51 AEST 
(In reply to comment #4)
> PAM acct mgmt can fail for reasons other than password expiry. The
> patch looks like you assume this is the reason.

The patch is about *account* expiry not *password* expiry.  Actually, it's about any failures of pam_acct_mgmt that aren't password expiry.

do_pam_account() sets force_pwchange and returns success if pam_account_mgmt returns PAM_NEW_AUTHTOK_REQD (but the code already checks for that) or returns a failure for any other non-success code.

> Also, if the account IS expired, the user should be given a chance
> to update their password.

If pam_acct_mgmt failed for any reason other than PAM_NEW_AUTHTOK_REQD then no, they shouldn't.
Comment 6 Frank Cusack 2006-05-04 12:49:44 AEST 
> do_pam_account() sets force_pwchange and returns success if
> pam_account_mgmt returns PAM_NEW_AUTHTOK_REQD (but the code already
> checks for that) or returns a failure for any other non-success code.

I hadn't looked at do_pam_acct(), I only looked at the patch.  So without
enough context I mistook the effects of the patch.  I did at least say
"looks like".

Thanks for the additional info, it sounds like the patch DTRT.

>> Also, if the account IS expired, the user should be given a chance
>> to update their password.
>
> If pam_acct_mgmt failed for any reason other than PAM_NEW_AUTHTOK_REQD
> then no, they shouldn't.

That's what I just said.  Since the patch doesn't have the effect I thought it
did, you can obviously ignore this comment.
Comment 7 Darren Tucker 2006-05-04 12:57:21 AEST 
(In reply to comment #6)
[...]
> it sounds like the patch DTRT.

Thanks for reviewing it.

> >> Also, if the account IS expired, the user should be given a chance
> >> to update their password.
> >
> > If pam_acct_mgmt failed for any reason other than
> > PAM_NEW_AUTHTOK_REQD then no, they shouldn't.
> 
> That's what I just said.  Since the patch doesn't have the effect I
> thought it did, you can obviously ignore this comment.

Actually you said "if the *account* is expired.  Since you apparently meant "if the password is expired" then we're in agreement and you can ignore my comment too :-)
Comment 8 Darren Tucker 2006-05-15 17:23:55 AEST 
Patch applied, thanks.
Comment 9 Darren Tucker 2006-09-28 19:26:15 AEST 
With the release of 4.4, we believe that this bug is now closed.  For information about the release please see http://www.openssh.com/txt/release-4.4 .