openssh unix dev - Sep 2004 - PATCH: Public key authentication defeats passwd age warning.

All,

I tried to sign up for this list a few weeks ago, but I don't think
it worked.  After I confirmed my intention to be on the list, I only
got one single message from someone on the list, and that was it.
So, either this is a particularly quiet list, or my subscription
was dropped somehow just after it was made.  So, if you could kindly
CC me directly on any responses to this, I sure would appreciate it.

In case y'all didn't see my request for help on comp.security.ssh
regarding this matter, here's a little review:

About a month ago I got the latest-n-greatest SSH available at the
time (v3.9p1), which may still be the latest-n-greatest, I dunno,
I haven't checked.  In order to get passwd aging to work properly,
I turned on ``UsePAM'' in the sshd_config file.  This seemed to
work fine in general except when using public key authentication.
Actually, even using public key authentication, most everything
worked fine WRT passwd aging.  When your passwd expired, it would
prompt you for a new one, when your account expired, you'd be
locked out, just like if you were using passwd authentication.
All that worked exactly like I expected, and exactly how I wanted
it to work with one minor exception.  The only thing that didn't
work WRT passwd aging + public key authentication was the ``passwd
expiration looming on the horizon'' warning.  If using public key
authentication, you'd get no warning.

Now, there was a philosophical response back to my initial query
regarding this.  ``Why do you want a warning that your passwd
is about to expire if you're not using your passwd anyway?''
Well, just because *YOU* aren't using your passwd doesn't mean you
shouldn't change it on a regular basis in a sensitive environment.
In many places, this is required, and it is also required that
the system enforce it.  The passwd change isn't to keep you out,
it's there to limit your exposure in case someone else gets your
passwd, and you don't notice.  Besides, as it turns out, if you're
enforcing passwd aging via ``UsePAM=yes'' SSH is going to prompt
you to change your passwd whether you're using passwd authentication
or using public key authentication, so if you're gonna be prompted
for a new one, and the local sysadmin has gone to all the trouble
to set up a warning period for you, shouldn't you get that warning?

Anyway, philosophy aside, I figgered out what happened to the
missing passwd age warning.  It turns out that even when you are
using public key authentication, the PAM function that checks up on
such things and is supposed to produce warnings (pam_acct_mgmt())
does actually get called.  However, it appears that since nobody
ever needed to interact with the lUser up until that point, no
conversation function was ever set up for the PAM library to use.
So, although pam_acct_mgmt() does actually figger out that the
lUser needs a warning, it can't communicate that with the poor,
unsuspecting lUser, so the message is just dropped, and all the bits
just dribble out all over the floor.  I'm surprised no one noticed
the puddle under the server.  Perhaps you've all got raised floors,
and all the bits are now down under there where the gremlins live.

Below is a patch for this, but here's the verbal:  To keep the basic
limited prototyping model this code seems to be following, I moved
do_pam_account() down below the definition of the function that I
wanted to use for the conversation function (sshpam_store_conv()).
Then, inside do_pam_account, I set PAM up with that conversation
function just before it calls pam_acct_mgmt().  However, this created
the side effect that any time do_pam_account() gets called, the
conversation function would always get reset to sshpam_store_conv().
Although I thought this would probably be OK, and although it
seemed to work fine that way under all the circumstances I could
think of to test, I just wasn't confident that it really was OK,
so I went back and made it save the original conversation function
off somewhere before setting it, then set it to sshpam_store_conv()
before the call to pam_acct_mgmt, and put back in place whatever
conversation function used to be there upon the function's
provocation before returning.  I doubt very seriously that all this
was really necessary, but like I said, I'm not all that familiar,
so to be safe, I did it.  Y'all can decide whether it was really
necessary or not.

Anyway, my patch is below.  Again, this is against v3.9p1.  Lemme
know what y'all think.  If this (or some reasonable approximation
thereof) manages to find its way into the distribution, I sure would
like to hear about it.  Like I said, I'm not on this mailing list
(although I tried to be), so I sure would appreciate being CCed on
any responses.

L8r,
tg.

--- openssh.original/BUILD/openssh-3.9p1/auth-pam.c	Mon Aug 16 09:12:06 2004
+++ openssh/BUILD/openssh-3.9p1/auth-pam.c	Mon Sep 13 08:35:36 2004
@@ -756,27 +756,6 @@
 	sshpam_cleanup();
 }
 
-u_int
-do_pam_account(void)
-{
-	if (sshpam_account_status != -1)
-		return (sshpam_account_status);
-
-	sshpam_err = pam_acct_mgmt(sshpam_handle, 0);
-	debug3("PAM: %s pam_acct_mgmt = %d", __func__, sshpam_err);
-	
-	if (sshpam_err != PAM_SUCCESS && sshpam_err != PAM_NEW_AUTHTOK_REQD) {
-		sshpam_account_status = 0;
-		return (sshpam_account_status);
-	}
-
-	if (sshpam_err == PAM_NEW_AUTHTOK_REQD)
-		sshpam_password_change_required(1);
-
-	sshpam_account_status = 1;
-	return (sshpam_account_status);
-}
-
 void
 do_pam_set_tty(const char *tty)
 {
@@ -939,6 +918,45 @@
 
 static struct pam_conv store_conv = { sshpam_store_conv, NULL };
 
+u_int
+do_pam_account(void)
+{
+	struct pam_conv *OldConv;
+	if (sshpam_account_status != -1)
+		return (sshpam_account_status);
+
+	sshpam_err = pam_get_item(sshpam_handle, PAM_CONV, (void *)&OldConv);
+	if (sshpam_err != PAM_SUCCESS)
+		fatal ("PAM: failed to get PAM_CONV: %s",
+		  pam_strerror (sshpam_handle, sshpam_err));
+
+	sshpam_err = pam_set_item(sshpam_handle, PAM_CONV, (void *)&store_conv);
+	if (sshpam_err != PAM_SUCCESS)
+		fatal("PAM: failed to set PAM_CONV: %s",
+		  pam_strerror(sshpam_handle, sshpam_err));
+
+	sshpam_err = pam_acct_mgmt(sshpam_handle, 0);
+	debug3("PAM: %s pam_acct_mgmt = %d", __func__, sshpam_err);
+	
+	if (sshpam_err != PAM_SUCCESS && sshpam_err != PAM_NEW_AUTHTOK_REQD) {
+		sshpam_account_status = 0;
+		goto Dun;
+	}
+
+	if (sshpam_err == PAM_NEW_AUTHTOK_REQD) sshpam_password_change_required(1);
+
+	sshpam_account_status = 1;
+
+Dun:
+
+	sshpam_err = pam_set_item(sshpam_handle, PAM_CONV, (void *)&OldConv);
+	if (sshpam_err != PAM_SUCCESS)
+		fatal ("PAM: failed to reset back to original PAM_CONV: %s",
+		  pam_strerror (sshpam_handle, sshpam_err));
+
+	return (sshpam_account_status);
+}
+
 void
 do_pam_session(void)
 {

Thomas Gardner wrote:
> Below is a patch for this, but here's the verbal:  To keep the basic
> limited prototyping model this code seems to be following, I moved
> do_pam_account() down below the definition of the function that I
> wanted to use for the conversation function (sshpam_store_conv()).
> Then, inside do_pam_account, I set PAM up with that conversation
> function just before it calls pam_acct_mgmt().
A similar change has already been made post-3.9p1 for similar reasons.

I'm wondering if we ought to set up a a catch-all conversation that 
keeps track of the best available methods of interacting with the user 
(eg start with store_conv, switch to interacting directly when PAM_TTY 
gets set) rather than trying to figure it out from combinations of what 
PAM functions we're calling and the use_privsep flag).

-- 
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4  37C9 C982 80C7 8FF4 FA69
     Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.