Fix for USE_POSIX_THREADS in auth-pam.c
Steven Michaud
smichaud at pobox.com
Mon Nov 3 04:49:48 EST 2003
I made a pretty bad mistake in the patch I submitted to this list on
10-29 -- in effect my mutex _didn't_ restrict access to the pam handle
(sshpam_handle) to one thread at a time. My new patch (below) fixes
this problem.
I suppose this is a good example of the difficulty of programming
threads :-(
It also seems that you can use the thread code in auth-pam.c _without_
any mutex -- that's in effect what I've been doing for the past couple
of weeks, without any ill effect (no crashes or other wierd behavior).
But my system is very quiet. People with busier systems might still
have trouble.
I know the OpenSSH folks aren't interested. I'm posting this
corrected patch for those adventurous types who might want to try to
use threads to solve the "PAM state" proble
This patch is against OpenSSH 3.7.1p2. Watch out for line breaks.
diff -u -r src.old/auth-pam.c src/auth-pam.c
--- src.old/auth-pam.c Sun Nov 2 10:41:47 2003
+++ src/auth-pam.c Sun Nov 2 10:41:46 2003
@@ -125,6 +125,90 @@
int pam_done;
};
+#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, only one other (sshpam_err) is ever changed from
+ * more than one thread. In any case, we can help by protecting whole
+ * blocks of PAM code at once.
+ */
+static void lock_pamh(void)
+{
+ pid_t pid_holder;
+ if (!process_id)
+ process_id = getpid();
+ /* 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).
+ */
+ else 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)
+ return;
+ ++sshpam_handle_lock_count;
+ pthread_mutex_lock(&sshpam_handle_lock);
+ --sshpam_handle_lock_count;
+ return;
+}
+
+static void unlock_pamh(void)
+{
+ pid_t pid_holder;
+ if (!process_id)
+ process_id = getpid();
+ else 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;
+ return;
+ }
+ if (!sshpam_handle_lock_ready)
+ return;
+ pthread_mutex_unlock(&sshpam_handle_lock);
+ return;
+}
+
+#else /* #ifdef USE_POSIX_THREADS */
+
+static void lock_pamh(void)
+{
+ return;
+}
+
+static void unlock_pamh(void)
+{
+ return;
+}
+
+#endif /* #ifdef USE_POSIX_THREADS */
+
static void sshpam_free_ctx(void *);
/*
@@ -223,6 +307,7 @@
sshpam_conv.appdata_ptr = ctxt;
buffer_init(&buffer);
+ lock_pamh();
sshpam_err = pam_set_item(sshpam_handle, PAM_CONV,
(const void *)&sshpam_conv);
if (sshpam_err != PAM_SUCCESS)
@@ -230,6 +315,7 @@
sshpam_err = pam_authenticate(sshpam_handle, 0);
if (sshpam_err != PAM_SUCCESS)
goto auth_fail;
+ unlock_pamh();
buffer_put_cstring(&buffer, "OK");
ssh_msg_send(ctxt->pam_csock, sshpam_err, &buffer);
buffer_free(&buffer);
@@ -238,6 +324,7 @@
auth_fail:
buffer_put_cstring(&buffer,
pam_strerror(sshpam_handle, sshpam_err));
+ unlock_pamh();
ssh_msg_send(ctxt->pam_csock, PAM_AUTH_ERR, &buffer);
buffer_free(&buffer);
pthread_exit(NULL);
@@ -270,20 +357,36 @@
{
(void)arg;
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 (sshpam_handle != NULL) {
+ lock_pamh();
+ 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;
+ unlock_pamh();
+ }
+#ifdef USE_POSIX_THREADS
+ else {
+ lock_pamh(); /* Update pthread variable state */
+ unlock_pamh(); /* and possibly bleed off traffic */
+ }
+ /* 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).
+ */
+ 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
@@ -293,12 +396,35 @@
extern char *__progname;
const char *pam_rhost, *pam_user;
+#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.
+ */
+ lock_pamh(); /* Update pthread variable state */
+ unlock_pamh(); /* and possibly bleed off traffic */
+ 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
+
+ lock_pamh();
if (sshpam_handle != NULL) {
/* We already have a PAM context; check if the user matches */
sshpam_err = pam_get_item(sshpam_handle,
PAM_USER, (const void **)&pam_user);
- if (sshpam_err == PAM_SUCCESS && strcmp(user, pam_user) == 0)
+ if (sshpam_err == PAM_SUCCESS && strcmp(user, pam_user) == 0) {
+ unlock_pamh();
return (0);
+ }
fatal_remove_cleanup(sshpam_cleanup, NULL);
pam_end(sshpam_handle, sshpam_err);
sshpam_handle = NULL;
@@ -309,6 +435,7 @@
if (sshpam_err != PAM_SUCCESS) {
pam_end(sshpam_handle, sshpam_err);
sshpam_handle = NULL;
+ unlock_pamh();
return (-1);
}
pam_rhost = get_remote_name_or_ip(utmp_len, options.use_dns);
@@ -317,6 +444,7 @@
if (sshpam_err != PAM_SUCCESS) {
pam_end(sshpam_handle, sshpam_err);
sshpam_handle = NULL;
+ unlock_pamh();
return (-1);
}
#ifdef PAM_TTY_KLUDGE
@@ -330,9 +458,11 @@
if (sshpam_err != PAM_SUCCESS) {
pam_end(sshpam_handle, sshpam_err);
sshpam_handle = NULL;
+ unlock_pamh();
return (-1);
}
#endif
+ unlock_pamh();
fatal_add_cleanup(sshpam_cleanup, NULL);
return (0);
}
@@ -531,7 +661,9 @@
u_int
do_pam_account(void)
{
+ lock_pamh();
sshpam_err = pam_acct_mgmt(sshpam_handle, 0);
+ unlock_pamh();
debug3("%s: pam_acct_mgmt = %d", __func__, sshpam_err);
if (sshpam_err != PAM_SUCCESS && sshpam_err != PAM_NEW_AUTHTOK_REQD)
@@ -552,38 +684,54 @@
void
do_pam_session(void)
{
+ const char *msg;
+ lock_pamh();
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));
+ if (sshpam_err != PAM_SUCCESS) {
+ msg = pam_strerror(sshpam_handle, sshpam_err);
+ unlock_pamh();
+ fatal("PAM: failed to set PAM_CONV: %s", msg);
+ }
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));
+ if (sshpam_err != PAM_SUCCESS) {
+ msg = pam_strerror(sshpam_handle, sshpam_err);
+ unlock_pamh();
+ fatal("PAM: pam_open_session(): %s", msg);
+ }
sshpam_session_open = 1;
+ unlock_pamh();
}
void
do_pam_set_tty(const char *tty)
{
+ const char *msg;
if (tty != NULL) {
+ lock_pamh();
debug("PAM: setting PAM_TTY to \"%s\"", tty);
sshpam_err = pam_set_item(sshpam_handle, PAM_TTY, tty);
- if (sshpam_err != PAM_SUCCESS)
- fatal("PAM: failed to set PAM_TTY: %s",
- pam_strerror(sshpam_handle, sshpam_err));
+ if (sshpam_err != PAM_SUCCESS) {
+ msg = pam_strerror(sshpam_handle, sshpam_err);
+ unlock_pamh();
+ fatal("PAM: failed to set PAM_TTY: %s", msg);
+ }
+ unlock_pamh();
}
}
void
do_pam_setcred(int init)
{
+ const char *msg;
+ lock_pamh();
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));
+ if (sshpam_err != PAM_SUCCESS) {
+ msg = pam_strerror(sshpam_handle, sshpam_err);
+ unlock_pamh();
+ fatal("PAM: failed to set PAM_CONV: %s", msg);
+ }
if (init) {
debug("PAM: establishing credentials");
sshpam_err = pam_setcred(sshpam_handle, PAM_ESTABLISH_CRED);
@@ -593,14 +741,15 @@
}
if (sshpam_err == PAM_SUCCESS) {
sshpam_cred_established = 1;
+ unlock_pamh();
return;
}
+ msg = pam_strerror(sshpam_handle, sshpam_err);
+ unlock_pamh();
if (sshpam_authenticated)
- fatal("PAM: pam_setcred(): %s",
- pam_strerror(sshpam_handle, sshpam_err));
+ fatal("PAM: pam_setcred(): %s", msg);
else
- debug("PAM: pam_setcred(): %s",
- pam_strerror(sshpam_handle, sshpam_err));
+ debug("PAM: pam_setcred(): %s", msg);
}
int
@@ -668,6 +817,7 @@
void
do_pam_chauthtok(void)
{
+ const char *msg;
struct pam_conv pam_conv;
pam_conv.conv = pam_chauthtok_conv;
@@ -675,16 +825,22 @@
if (use_privsep)
fatal("Password expired (unable to change with privsep)");
+ lock_pamh();
sshpam_err = pam_set_item(sshpam_handle, 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));
+ if (sshpam_err != PAM_SUCCESS) {
+ msg = pam_strerror(sshpam_handle, sshpam_err);
+ unlock_pamh();
+ fatal("PAM: failed to set PAM_CONV: %s", msg);
+ }
debug("PAM: changing password");
sshpam_err = pam_chauthtok(sshpam_handle, PAM_CHANGE_EXPIRED_AUTHTOK);
- if (sshpam_err != PAM_SUCCESS)
- fatal("PAM: pam_chauthtok(): %s",
- pam_strerror(sshpam_handle, sshpam_err));
+ if (sshpam_err != PAM_SUCCESS) {
+ msg = pam_strerror(sshpam_handle, sshpam_err);
+ unlock_pamh();
+ fatal("PAM: pam_chauthtok(): %s", msg);
+ }
+ unlock_pamh();
}
/*
@@ -705,7 +861,9 @@
compound = xmalloc(len);
snprintf(compound, len, "%s=%s", name, value);
+ lock_pamh();
ret = pam_putenv(sshpam_handle, compound);
+ unlock_pamh();
xfree(compound);
#endif
@@ -722,8 +880,12 @@
fetch_pam_environment(void)
{
#ifdef HAVE_PAM_GETENVLIST
+ char **retval;
+ lock_pamh();
debug("PAM: retrieving environment");
- return (pam_getenvlist(sshpam_handle));
+ retval = pam_getenvlist(sshpam_handle);
+ unlock_pamh();
+ return retval;
#else
return (NULL);
#endif
More information about the openssh-unix-dev
mailing list