PAM sessions and conversation functions

[Darren Tucker]

Colin Watson wrote:
> 
> In OpenSSH 3.6.1p2, pam_open_session() ran with a conversation function,
> do_pam_conversation(), that fed text to the client. In OpenSSH 3.7.1p2,
> this is no longer the case: session modules run with a conversation
> function that just returns PAM_CONV_ERR. This means that simple session
> modules whose job involves printing text on the user's terminal no
> longer work: pam_lastlog, pam_mail, and pam_motd.
> 
> Can somebody explain to me why this change was made (as part of the
> FreeBSD PAM merge, apparently), or if it was a mistake? I realize that
> session modules are now run as root, but I'd have thought that modules
> should be trusted code and don't need to have their output sanitized.

It appears to be an oversight.

Attached are 2 PAM-related patches.  The first appends newlines to the PAM
messages from pam_chauthtok_conv(), and the second uses
pam_chauthtok_conv() as the session conversation function.

The reason for the first is trivial: the conversation function is supposed
to supply newlines if necessary, but currently doesn't.

The second has two purposes: first is that pam_chauthtok_conv will spit
the messages from the session modules onto stdout.  The second is that it
appears sometimes (only some patchlevels?) Solaris tries to do a password
change in the session module if the password is expired.

Comments on either?

-- 
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.
-------------- next part --------------
Index: auth-pam.c
===================================================================
RCS file: /usr/local/src/security/openssh/cvs/openssh_cvs/auth-pam.c,v
retrieving revision 1.76
diff -u -p -r1.76 auth-pam.c
--- auth-pam.c	9 Oct 2003 04:20:15 -0000	1.76
+++ auth-pam.c	11 Oct 2003 07:34:58 -0000
@@ -636,14 +636,14 @@ pam_chauthtok_conv(int n, const struct p
 			reply[i].resp_retcode = PAM_SUCCESS;
 			break;
 		case PAM_PROMPT_ECHO_ON:
-			fputs(PAM_MSG_MEMBER(msg, i, msg), stderr);
+			fprintf(stderr, "%s\n", PAM_MSG_MEMBER(msg, i, msg));
 			fgets(input, sizeof input, stdin);
 			reply[i].resp = xstrdup(input);
 			reply[i].resp_retcode = PAM_SUCCESS;
 			break;
 		case PAM_ERROR_MSG:
 		case PAM_TEXT_INFO:
-			fputs(PAM_MSG_MEMBER(msg, i, msg), stderr);
+			fprintf(stderr, "%s\n", PAM_MSG_MEMBER(msg, i, msg));
 			reply[i].resp_retcode = PAM_SUCCESS;
 			break;
 		default:
-------------- next part --------------
Index: auth-pam.c
===================================================================
RCS file: /usr/local/src/security/openssh/cvs/openssh_cvs/auth-pam.c,v
retrieving revision 1.76
diff -u -p -r1.76 auth-pam.c
--- auth-pam.c	9 Oct 2003 04:20:15 -0000	1.76
+++ auth-pam.c	11 Oct 2003 07:36:04 -0000
@@ -551,21 +551,6 @@ do_pam_account(void)
 }
 
 void
-do_pam_session(void)
-{
-	sshpam_err = pam_set_item(sshpam_handle, PAM_CONV, 
-	    (const void *)&null_conv);
-	if (sshpam_err != PAM_SUCCESS)
-		fatal("PAM: failed to set PAM_CONV: %s",
-		    pam_strerror(sshpam_handle, sshpam_err));
-	sshpam_err = pam_open_session(sshpam_handle, 0);
-	if (sshpam_err != PAM_SUCCESS)
-		fatal("PAM: pam_open_session(): %s",
-		    pam_strerror(sshpam_handle, sshpam_err));
-	sshpam_session_open = 1;
-}
-
-void
 do_pam_set_tty(const char *tty)
 {
 	if (tty != NULL) {
@@ -662,6 +647,8 @@ pam_chauthtok_conv(int n, const struct p
 	return (PAM_CONV_ERR);
 }
 
+static struct pam_conv chauthtok_conv = { pam_chauthtok_conv, NULL };
+
 /*
  * XXX this should be done in the authentication phase, but ssh1 doesn't
  * support that
@@ -669,15 +656,10 @@ pam_chauthtok_conv(int n, const struct p
 void
 do_pam_chauthtok(void)
 {
-	struct pam_conv pam_conv;
-
-	pam_conv.conv = pam_chauthtok_conv;
-	pam_conv.appdata_ptr = NULL;
-
 	if (use_privsep)
 		fatal("Password expired (unable to change with privsep)");
 	sshpam_err = pam_set_item(sshpam_handle, PAM_CONV,
-	    (const void *)&pam_conv);
+	    (const void *)&chauthtok_conv);
 	if (sshpam_err != PAM_SUCCESS)
 		fatal("PAM: failed to set PAM_CONV: %s",
 		    pam_strerror(sshpam_handle, sshpam_err));
@@ -686,6 +668,21 @@ do_pam_chauthtok(void)
 	if (sshpam_err != PAM_SUCCESS)
 		fatal("PAM: pam_chauthtok(): %s",
 		    pam_strerror(sshpam_handle, sshpam_err));
+}
+
+void
+do_pam_session(void)
+{
+	sshpam_err = pam_set_item(sshpam_handle, PAM_CONV, 
+	    (const void *)&chauthtok_conv);
+	if (sshpam_err != PAM_SUCCESS)
+		fatal("PAM: failed to set PAM_CONV: %s",
+		    pam_strerror(sshpam_handle, sshpam_err));
+	sshpam_err = pam_open_session(sshpam_handle, 0);
+	if (sshpam_err != PAM_SUCCESS)
+		fatal("PAM: pam_open_session(): %s",
+		    pam_strerror(sshpam_handle, sshpam_err));
+	sshpam_session_open = 1;
 }
 
 /*