Fix for USE_POSIX_THREADS in auth-pam.c
Steven Michaud
smichaud* at pobox.com
Thu Oct 30 08:32:14 EST 2003
As many of you know, OpenSSH 3.7.X, unlike previous versions, makes
PAM authentication take place in a separate process or thread
(launched from sshpam_init_ctx() in auth-pam.c). By default (if you
don't define USE_POSIX_THREADS) the code "fork"s a separate process.
Or if you define USE_POSIX_THREADS it will create a new thread (a
second one, in addition to the primary thread).
The default option (authenticating in a child process) has a serious
problem -- authentication changes PAM's state, but the new state won't
get transferred from the child to the parent unless you add new hacks
to explicitly transfer each bit and piece that you want.
For example, Christian Pfaffel posted a patch to this list on 9-17
with hacks to force Kerberos credentials to disk and to use
ssh_msg_send() to send the PAM environment from the child process to
the parent. (His patch was in an attachment and got dropped. But
fortunately he re-posted his message to the MIT Kerberos newsgroup a
few days later, and this time the attachment came through --
http://diswww.mit.edu:8008/menelaus.mit.edu/kerberos/19973.)
Christian's patch works. But it's difficult to know exactly what
state needs to be transferred. A simpler and more elegant solution is
to create a new thread -- as long as both the "parent" and the "child"
thread use the same PAM handle, state-changes in each thread
"automatically" become visible in the other.
In fact this solution works just fine (as long as your OS has support
for POSIX threads). But a small change was required to the "thread"
code in auth-pam.c: The man pages for Linux PAM (also used on
Darwin/OS X) and Solaris PAM say that PAM isn't thread safe unless
each thread uses a different PAM handle. But that's useless for us --
we need both threads to share a single PAM handle. Instead we should
use a mutex to prevent the single handle from being used by more than
a single thread at a time.
That's what the following patch does. It also re-initializes the
mutex when (after the authentication has succeeded) a user-owned child
process gets forked (in do_exec_no_pty() and do_exec_pty() in
session.c) that still needs access to the PAM handle.
Since I know that the OpenSSH folks prefer to get patches against the
most recent versions, this patch is meant to be applied to
openssh-SNAP-20031028. It's been tested (separately) on Solaris 8 and
(together with other patches) on Darwin/OS X.
On some OSs (notably Solaris) you will need to add -lpthread to
LDFLAGS (besides defining USE_POSIX_THREADS in CPPFLAGS).
Please let me know if you have problems with it.
Beware of broken lines (I don't dare include it as a separate
attachment).
diff -u -r src.old/auth-pam.c src/auth-pam.c
--- src.old/auth-pam.c Wed Oct 29 12:37:08 2003
+++ src/auth-pam.c Wed Oct 29 12:37:07 2003
@@ -128,6 +128,69 @@
static void sshpam_free_ctx(void *);
static struct pam_ctxt *cleanup_ctxt;
+#ifdef USE_POSIX_THREADS
+
+static pthread_mutexattr_t lock_attr;
+static pthread_mutex_t sshpam_handle_lock;
+static int sshpam_handle_lock_ready = 0;
+static int sshpam_handle_lock_count = 0;
+static pid_t process_id = 0;
+
+/* On Solaris, Linux and Darwin, PAM routines are said to only be
+ * thread-safe if each thread has a different PAM handle (which really
+ * means they're NOT thread-safe, but anyway). For our purposes, all
+ * threads must use the same handle (otherwise it will be difficult for
+ * them to share PAM state), so we need to protect access to
+ * sshpam_handle with a mutex.
+ *
+ * Auth-pam.c has many other global variables, which might in principle
+ * also need to be protected by mutexes. But none of the others is a
+ * handle into an external black box (like PAM). And in the current state
+ * of the code, none of these global variables (even sshpam_handle) is
+ * ever changed from more than one thread.
+ */
+static pam_handle_t *grab_pamh(int set, pam_handle_t *value)
+{
+ pid_t pid_holder;
+ /* It's not safe to use pthread structures created for our parent
+ * (if we've been forked our pid will have changed). Reinitialize
+ * everything if this has happened (we know beforehand that these
+ * structures can't yet be in use in our process).
+ */
+ if (process_id != (pid_holder = getpid())) {
+ sshpam_handle_lock_ready = 0;
+ process_id = pid_holder;
+ sshpam_handle_lock_count = 0;
+ pthread_mutexattr_init(&lock_attr);
+ pthread_mutexattr_settype(&lock_attr, PTHREAD_MUTEX_RECURSIVE);
+ pthread_mutex_init(&sshpam_handle_lock, &lock_attr);
+ sshpam_handle_lock_ready = 1;
+ }
+ if (!sshpam_handle_lock_ready) {
+ if (set)
+ sshpam_handle = value;
+ return sshpam_handle;
+ }
+ ++sshpam_handle_lock_count;
+ pthread_mutex_lock(&sshpam_handle_lock);
+ if (set)
+ sshpam_handle = value;
+ pthread_mutex_unlock(&sshpam_handle_lock);
+ --sshpam_handle_lock_count;
+ return sshpam_handle;
+}
+
+#else /* #ifdef USE_POSIX_THREADS */
+
+static pam_handle_t *grab_pamh(int set, pam_handle_t *value)
+{
+ if (set)
+ sshpam_handle = value;
+ return sshpam_handle;
+}
+
+#endif /* #ifdef USE_POSIX_THREADS */
+
/*
* Conversation function for authentication thread.
*/
@@ -216,7 +279,7 @@
#ifndef USE_POSIX_THREADS
const char *pam_user;
- pam_get_item(sshpam_handle, PAM_USER, (const void **)&pam_user);
+ pam_get_item(grab_pamh(0, NULL), PAM_USER, (const void **)&pam_user);
setproctitle("%s [pam]", pam_user);
#endif
@@ -224,11 +287,11 @@
sshpam_conv.appdata_ptr = ctxt;
buffer_init(&buffer);
- sshpam_err = pam_set_item(sshpam_handle, PAM_CONV,
+ sshpam_err = pam_set_item(grab_pamh(0, NULL), PAM_CONV,
(const void *)&sshpam_conv);
if (sshpam_err != PAM_SUCCESS)
goto auth_fail;
- sshpam_err = pam_authenticate(sshpam_handle, 0);
+ sshpam_err = pam_authenticate(grab_pamh(0, NULL), 0);
if (sshpam_err != PAM_SUCCESS)
goto auth_fail;
buffer_put_cstring(&buffer, "OK");
@@ -238,7 +301,7 @@
auth_fail:
buffer_put_cstring(&buffer,
- pam_strerror(sshpam_handle, sshpam_err));
+ pam_strerror(grab_pamh(0, NULL), sshpam_err));
ssh_msg_send(ctxt->pam_csock, PAM_AUTH_ERR, &buffer);
buffer_free(&buffer);
pthread_exit(NULL);
@@ -274,20 +337,31 @@
sshpam_cleanup(void)
{
debug("PAM: cleanup");
- if (sshpam_handle == NULL)
- return;
- pam_set_item(sshpam_handle, PAM_CONV, (const void *)&null_conv);
- if (sshpam_cred_established) {
- pam_setcred(sshpam_handle, PAM_DELETE_CRED);
- sshpam_cred_established = 0;
- }
- if (sshpam_session_open) {
- pam_close_session(sshpam_handle, PAM_SILENT);
- sshpam_session_open = 0;
- }
- sshpam_authenticated = sshpam_new_authtok_reqd = 0;
- pam_end(sshpam_handle, sshpam_err);
- sshpam_handle = NULL;
+ if (grab_pamh(0, NULL) != NULL) {
+ pam_set_item(grab_pamh(0, NULL), PAM_CONV, (const void *)&null_conv);
+ if (sshpam_cred_established) {
+ pam_setcred(grab_pamh(0, NULL), PAM_DELETE_CRED);
+ sshpam_cred_established = 0;
+ }
+ if (sshpam_session_open) {
+ pam_close_session(grab_pamh(0, NULL), PAM_SILENT);
+ sshpam_session_open = 0;
+ }
+ sshpam_authenticated = sshpam_new_authtok_reqd = 0;
+ pam_end(grab_pamh(0, NULL), sshpam_err);
+ grab_pamh(1, NULL);
+ }
+#ifdef USE_POSIX_THREADS
+ /* Free our pthread structures if it's safe to do so (if they were
+ * previously initialized and aren't currently in use in our process).
+ */
+ grab_pamh(0, NULL); /* Bleed off traffic (possibly) and update state */
+ if (!sshpam_handle_lock_count && sshpam_handle_lock_ready) {
+ sshpam_handle_lock_ready = 0;
+ pthread_mutexattr_destroy(&lock_attr);
+ pthread_mutex_destroy(&sshpam_handle_lock);
+ }
+#endif
}
static int
@@ -296,30 +370,53 @@
extern u_int utmp_len;
extern char *__progname;
const char *pam_rhost, *pam_user;
+ pam_handle_t *sshpam_handle_holder;
+
+#ifdef USE_POSIX_THREADS
+ /* (Re)initialize our pthread structures if it's safe to do so. Only
+ * free them if they were previously initialized and they aren't
+ * currently in use.
+ */
+ if (!process_id)
+ process_id = getpid();
+ grab_pamh(0, NULL); /* Bleed off traffic (possibly) and update state */
+ if (!sshpam_handle_lock_count) {
+ if (sshpam_handle_lock_ready) {
+ sshpam_handle_lock_ready = 0;
+ pthread_mutexattr_destroy(&lock_attr);
+ pthread_mutex_destroy(&sshpam_handle_lock);
+ }
+ pthread_mutexattr_init(&lock_attr);
+ pthread_mutexattr_settype(&lock_attr, PTHREAD_MUTEX_RECURSIVE);
+ pthread_mutex_init(&sshpam_handle_lock, &lock_attr);
+ sshpam_handle_lock_ready = 1;
+ }
+#endif
- if (sshpam_handle != NULL) {
+ if (grab_pamh(0, NULL) != NULL) {
/* We already have a PAM context; check if the user matches */
- sshpam_err = pam_get_item(sshpam_handle,
+ sshpam_err = pam_get_item(grab_pamh(0, NULL),
PAM_USER, (const void **)&pam_user);
if (sshpam_err == PAM_SUCCESS && strcmp(user, pam_user) == 0)
return (0);
- pam_end(sshpam_handle, sshpam_err);
- sshpam_handle = NULL;
+ pam_end(grab_pamh(0, NULL), sshpam_err);
+ grab_pamh(1, NULL);
}
debug("PAM: initializing for \"%s\"", user);
sshpam_err =
- pam_start(SSHD_PAM_SERVICE, user, &null_conv, &sshpam_handle);
+ pam_start(SSHD_PAM_SERVICE, user, &null_conv, &sshpam_handle_holder);
+ grab_pamh(1, sshpam_handle_holder);
if (sshpam_err != PAM_SUCCESS) {
- pam_end(sshpam_handle, sshpam_err);
- sshpam_handle = NULL;
+ pam_end(grab_pamh(0, NULL), sshpam_err);
+ grab_pamh(1, NULL);
return (-1);
}
pam_rhost = get_remote_name_or_ip(utmp_len, options.use_dns);
debug("PAM: setting PAM_RHOST to \"%s\"", pam_rhost);
- sshpam_err = pam_set_item(sshpam_handle, PAM_RHOST, pam_rhost);
+ sshpam_err = pam_set_item(grab_pamh(0, NULL), PAM_RHOST, pam_rhost);
if (sshpam_err != PAM_SUCCESS) {
- pam_end(sshpam_handle, sshpam_err);
- sshpam_handle = NULL;
+ pam_end(grab_pamh(0, NULL), sshpam_err);
+ grab_pamh(1, NULL);
return (-1);
}
#ifdef PAM_TTY_KLUDGE
@@ -329,10 +426,10 @@
* may not even set one (for tty-less connections)
*/
debug("PAM: setting PAM_TTY to \"ssh\"");
- sshpam_err = pam_set_item(sshpam_handle, PAM_TTY, "ssh");
+ sshpam_err = pam_set_item(grab_pamh(0, NULL), PAM_TTY, "ssh");
if (sshpam_err != PAM_SUCCESS) {
- pam_end(sshpam_handle, sshpam_err);
- sshpam_handle = NULL;
+ pam_end(grab_pamh(0, NULL), sshpam_err);
+ grab_pamh(1, NULL);
return (-1);
}
#endif
@@ -532,7 +629,7 @@
u_int
do_pam_account(void)
{
- sshpam_err = pam_acct_mgmt(sshpam_handle, 0);
+ sshpam_err = pam_acct_mgmt(grab_pamh(0, NULL), 0);
debug3("%s: pam_acct_mgmt = %d", __func__, sshpam_err);
if (sshpam_err != PAM_SUCCESS && sshpam_err != PAM_NEW_AUTHTOK_REQD)
@@ -553,15 +650,15 @@
void
do_pam_session(void)
{
- sshpam_err = pam_set_item(sshpam_handle, PAM_CONV,
+ sshpam_err = pam_set_item(grab_pamh(0, NULL), 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);
+ pam_strerror(grab_pamh(0, NULL), sshpam_err));
+ sshpam_err = pam_open_session(grab_pamh(0, NULL), 0);
if (sshpam_err != PAM_SUCCESS)
fatal("PAM: pam_open_session(): %s",
- pam_strerror(sshpam_handle, sshpam_err));
+ pam_strerror(grab_pamh(0, NULL), sshpam_err));
sshpam_session_open = 1;
}
@@ -570,27 +667,27 @@
{
if (tty != NULL) {
debug("PAM: setting PAM_TTY to \"%s\"", tty);
- sshpam_err = pam_set_item(sshpam_handle, PAM_TTY, tty);
+ sshpam_err = pam_set_item(grab_pamh(0, NULL), PAM_TTY, tty);
if (sshpam_err != PAM_SUCCESS)
fatal("PAM: failed to set PAM_TTY: %s",
- pam_strerror(sshpam_handle, sshpam_err));
+ pam_strerror(grab_pamh(0, NULL), sshpam_err));
}
}
void
do_pam_setcred(int init)
{
- sshpam_err = pam_set_item(sshpam_handle, PAM_CONV,
+ sshpam_err = pam_set_item(grab_pamh(0, NULL), 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));
+ pam_strerror(grab_pamh(0, NULL), sshpam_err));
if (init) {
debug("PAM: establishing credentials");
- sshpam_err = pam_setcred(sshpam_handle, PAM_ESTABLISH_CRED);
+ sshpam_err = pam_setcred(grab_pamh(0, NULL), PAM_ESTABLISH_CRED);
} else {
debug("PAM: reinitializing credentials");
- sshpam_err = pam_setcred(sshpam_handle, PAM_REINITIALIZE_CRED);
+ sshpam_err = pam_setcred(grab_pamh(0, NULL), PAM_REINITIALIZE_CRED);
}
if (sshpam_err == PAM_SUCCESS) {
sshpam_cred_established = 1;
@@ -598,10 +695,10 @@
}
if (sshpam_authenticated)
fatal("PAM: pam_setcred(): %s",
- pam_strerror(sshpam_handle, sshpam_err));
+ pam_strerror(grab_pamh(0, NULL), sshpam_err));
else
debug("PAM: pam_setcred(): %s",
- pam_strerror(sshpam_handle, sshpam_err));
+ pam_strerror(grab_pamh(0, NULL), sshpam_err));
}
int
@@ -676,16 +773,16 @@
if (use_privsep)
fatal("Password expired (unable to change with privsep)");
- sshpam_err = pam_set_item(sshpam_handle, PAM_CONV,
+ sshpam_err = pam_set_item(grab_pamh(0, NULL), PAM_CONV,
(const void *)&pam_conv);
if (sshpam_err != PAM_SUCCESS)
fatal("PAM: failed to set PAM_CONV: %s",
- pam_strerror(sshpam_handle, sshpam_err));
+ pam_strerror(grab_pamh(0, NULL), sshpam_err));
debug("PAM: changing password");
- sshpam_err = pam_chauthtok(sshpam_handle, PAM_CHANGE_EXPIRED_AUTHTOK);
+ sshpam_err = pam_chauthtok(grab_pamh(0, NULL), PAM_CHANGE_EXPIRED_AUTHTOK);
if (sshpam_err != PAM_SUCCESS)
fatal("PAM: pam_chauthtok(): %s",
- pam_strerror(sshpam_handle, sshpam_err));
+ pam_strerror(grab_pamh(0, NULL), sshpam_err));
}
/*
@@ -706,7 +803,7 @@
compound = xmalloc(len);
snprintf(compound, len, "%s=%s", name, value);
- ret = pam_putenv(sshpam_handle, compound);
+ ret = pam_putenv(grab_pamh(0, NULL), compound);
xfree(compound);
#endif
@@ -724,7 +821,7 @@
{
#ifdef HAVE_PAM_GETENVLIST
debug("PAM: retrieving environment");
- return (pam_getenvlist(sshpam_handle));
+ return (pam_getenvlist(grab_pamh(0, NULL)));
#else
return (NULL);
#endif
More information about the openssh-unix-dev
mailing list