Bug 1322 - pam_end() is not called if authentication fails, which breaks pam-abl
Summary: pam_end() is not called if authentication fails, which breaks pam-abl
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: PAM support (show other bugs)
Version: 4.6p1
Hardware: Other Linux
: P2 normal
Assignee: Assigned to nobody
URL: http://bugs.debian.org/cgi-bin/bugrep...
Keywords:
: 1308 (view as bug list)
Depends on:
Blocks: V_4_7 V_4_6_P2
  Show dependency treegraph
 
Reported: 2007-06-18 05:40 AEST by Christoffer Hammarström
Modified: 2008-04-04 09:59 AEDT (History)
4 users (show)

See Also:


Attachments
Changeset that introduced the change in question. (3.11 KB, patch)
2007-06-18 09:57 AEST, Darren Tucker
no flags Details | Diff
Patch by Sandro Wefel from Debian bug #405041 (don't use as per djm's comments) (725 bytes, patch)
2007-06-18 10:16 AEST, Darren Tucker
djm: ok-
Details | Diff
Patch for do_cleanup with respect to the signal handler vulnerability fixed in openssh-4.4 (570 bytes, patch)
2007-07-12 00:47 AEST, Sandro Wefel
no flags Details | Diff
Patch #1325 with dead code removed. (961 bytes, patch)
2007-08-13 23:21 AEST, Darren Tucker
dtucker: ok-
Details | Diff
Allow PAM cleanup for unathenticated connections based on previous (953 bytes, patch)
2007-08-15 23:51 AEST, Darren Tucker
djm: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christoffer Hammarström 2007-06-18 05:40:49 AEST
Pam-abl (http://www.hexten.net/wiki/index.php/Pam_abl) is a PAM module that automatically blacklists hosts or users after a given number of failed authentication attempts.

It relies on pam_end() being called by the pam application, and this is not done by sshd for failed authentication attempts.

This is debian bug 405041, and i have confirmed that applying the patch found at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=405041 makes pam-abl work again.
Comment 1 Darren Tucker 2007-06-18 09:57:50 AEST
Created attachment 1307 [details]
Changeset that introduced the change in question.

This is the changeset that introduced the change for reference.
Comment 2 Damien Miller 2007-06-18 10:07:54 AEST
DO NOT apply the patch in the Debian bug. It will expose your system to the signal handler vulnerability fixed in openssh-4.4

This is the "difficult to fix" SIGALRM handler. We could make sshpam_cleanup() fire if do_cleanup was not called in signal context, but that would just open a different workaround for password guessers: make max_auth_tries-1 guesses and keep the connection open until it times out. 
Comment 3 Darren Tucker 2007-06-18 10:16:43 AEST
Created attachment 1308 [details]
Patch by Sandro Wefel from Debian bug #405041 (don't use as per djm's comments)

Proposed patch from the Debian bug.
Comment 4 Darren Tucker 2007-06-18 10:17:36 AEST
Target next release
Comment 5 Damien Miller 2007-06-18 10:20:43 AEST
Comment on attachment 1308 [details]
Patch by Sandro Wefel from Debian bug #405041 (don't use as per djm's comments)

as per comment #2
Comment 6 Sandro Wefel 2007-07-12 00:47:03 AEST
Created attachment 1325 [details]
Patch for do_cleanup with respect to the signal handler vulnerability fixed in openssh-4.4
Comment 7 Sandro Wefel 2007-07-12 01:17:41 AEST
Please have a look at the attached patch (id=1325).

The idea is to call sshpam_cleanup() if authctxt->authenticated is not set before the KRB5 and GSSAPI blocks. After the pam-call we just return from the function do_cleanup(). This means that krb5_cleanup_proc(authctxt) is not called with an invalid parameter but the sshpam_cleanup() is done which leads to the pam_end call.

IMHO this should avoid the signal handler race condition CVE-2006-5051 in krb5_cleanup_proc but calls pam_end() if the user authentication fails.

Comment 8 Darren Tucker 2007-08-13 23:21:31 AEST
Created attachment 1339 [details]
Patch #1325 with dead code removed.

Damien points out that this makes the existing PAM cleanup code unnecessary.
Comment 9 Darren Tucker 2007-08-15 23:29:08 AEST
*** Bug 1308 has been marked as a duplicate of this bug. ***
Comment 10 Darren Tucker 2007-08-15 23:48:19 AEST
Comment on attachment 1339 [details]
Patch #1325 with dead code removed.

Oops, the patch is wrong; it won't clean up after authenticated connections.
Comment 11 Darren Tucker 2007-08-15 23:51:33 AEST
Created attachment 1342 [details]
Allow PAM cleanup for unathenticated connections based on previous

I think this is the simplest patch that does what is required.
Comment 12 Sandro Wefel 2007-08-16 02:22:19 AEST
The last patch works for me like my patch on all tested architectures and machines in combination with pam_abl. Good work.
Comment 13 Damien Miller 2007-08-16 13:52:47 AEST
Comment on attachment 1342 [details]
Allow PAM cleanup for unathenticated connections based on previous

ok for 4.7
Comment 14 Darren Tucker 2007-08-16 23:29:59 AEST
Applied, thanks to all.
Comment 15 Damien Miller 2008-04-04 09:59:54 AEDT
Close resolved bugs after release.