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
Steven Michaud wrote:> 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.)Please try the one at http://bugzilla.mindrot.org/show_bug.cgi?id=717 It has been sitting there with little feedback for a while.> 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.We won't be supporting threads, they add way more complexity then they solve. The code is still ethere because some people may want to use it, at their own risk. I'd prefer to explicitly export state from the PAM child back to the parent (hidden state is a bad idea, especially in a security API). Getting the above patch reviewed would be a start in this direction. -d
Please try the one at http://bugzilla.mindrot.org/show_bug.cgi?id=717 I've just done so. Like others who posted comments to the same URI, I found that it "doesn't work". (I'll explain how below, and suggest a fix.) Your patch is functionally equivalent to part of Christian Pfaffel's patch to auth-pam.c -- the part that copies the PAM environment from the child process to the parent process. But you don't do anything to force the export of user credentials while still in the child ... which explains why your patch "doesn't work" for users of pam_krb5: Pam_krb5's pam_sm_setcred() function recovers and exports to disk the Kerberos credentials that were created in pam_sm_authenticate() (and stored to PAM's internal state by a call to pam_set_data()), then sets the KRB5CCNAME variable (in the PAM environment) to point to the cache file. You can trigger this by a call to do_pam_setcred() in OpenSSH. But it must be done while still in the child process where PAM authentication took place -- the internal state where the Kerberos credentials were stored (by pam_set_data()) is only present in the child, not in the parent. Your patch can be fixed (for users of pam_krb5) by adding a call to do_pam_setcred(0) at line 288 of auth-pam.c (as patched by you). By the way, I performed my tests of your patch on Solaris 8. I kept the tests as simple as possible -- my "configure" options were "--with-ssl-dir=/usr/local/ssl --with-pam" (so I didn't compile in GSSAPI support). The three lines in my pam.conf that governed OpenSSH were as follows: sshd auth requisite pam_authtok_get.so.1 sshd auth sufficient pam_krb5.so.1 use_first_pass sshd auth sufficient pam_unix_auth.so.1 When using your patch without the call to do_pam_setcred(0), I could connect using my "Kerberos password", but no credentials file was created and the KRB5CCNAME variable wasn't set. Adding the call to do_pam_setcred(0) fixed both of these problems.> We won't be supporting threads, they add way more complexity then > they solve.I actually think the PAM-state problem is a textbook example of what threads are good for -- allowing multiple lines of execution to easily share the same process state. It's true that multi-threaded programming is tricky, and has a steep learning curve. But I don't believe it's any harder than programming multiple processes. It's just a question of what you're used to (in my case I got into multiple threads before I got into multiple processes). That said, OpenSSH would need quite a few changes before multiple threads could be used everywhere -- in particular, you'd need to get rid of a lot of your global variables. And using multiple threads in just one small part of OpenSSH increases the possibility that someone will inadvertently tie them in knots :-) So I can understand your reluctance to start using threads.> The code is still ethere because some people may want to use it, at > their own risk.Like I said, I understand why this code isn't turned on by default. But in its current state anyone who tries to use it will shoot themselves in the foot. Couldn't you include my patch for the sake of those who are willing to experiment?> I'd prefer to explicitly export state from the PAM child back to the > parent (hidden state is a bad idea, especially in a security API).A PAM implementation is (in effect) a security program. Like OpenSSH, you can use it without fully understanding what it does ... as long as you trust the programmers sufficiently. Calling it a black box isn't a valid criticism -- so is everything you're not currently paying attention to. It's true that PAM is insufficiently specified, and that it's configuration "language" is both too complex and not powerful enough. But there are some things that are very difficult to do without it -- like using several different authentication methods simultaneously, or translating credentials from one type to another. Damien Miller wrote:> Steven Michaud wrote: > > > 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.) > > Please try the one at http://bugzilla.mindrot.org/show_bug.cgi?id=717 > > It has been sitting there with little feedback for a while. > > > 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. > > We won't be supporting threads, they add way more complexity then > they solve. The code is still ethere because some people may want to > use it, at their own risk. > > I'd prefer to explicitly export state from the PAM child back to the > parent (hidden state is a bad idea, especially in a security API). > Getting the above patch reviewed would be a start in this direction. > > -d >
> It doesn't sound like this patch is supporting threads in PAM > persay. I mean literally from the sounds of the patch. It is not > threaded. Thus doesn't break..You've completely missed the boat :-) But I'm not sure why (you don't know threads?, you haven't read the patch carefully?, you tried it and it "didn't work"?), so I don't know what to say next. Let's assume that my patch "didn't work": One thing to remember is that you need to define USE_POSIX_THREADS, and that you may also need to add libpthread.so to the list of libraries that sshd links to. The way I did this was to add the following to my "configure" parameters: "CPPFLAGS=-DUSE_POSIX_THREADS LDFLAGS=-lpthread" Sean O'Malley wrote:> On Thu, 30 Oct 2003, Damien Miller wrote: > > > Steven Michaud wrote: > > > > > 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.) > > > > Please try the one at http://bugzilla.mindrot.org/show_bug.cgi?id=717 > > > > It has been sitting there with little feedback for a while. > > I tried this a while ago and it still didn't work. > > > > 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. > > > > We won't be supporting threads, they add way more complexity then > > they solve. The code is still ethere because some people may want > > to use it, at their own risk. > > It doesn't sound like this patch is supporting threads in PAM > persay. I mean literally from the sounds of the patch. It is not > threaded. Thus doesn't break.. > > > I'd prefer to explicitly export state from the PAM child back to > > the parent (hidden state is a bad idea, especially in a security > > API). Getting the above patch reviewed would be a start in this > > direction. > > Is this even going to work things like AFS and Kerberos? I am not > seeing how this is going to work at all since copying the state goes > against what AFS does with pags (I assume kerberos works extremely > similarly). Kudos to you if you do, but in the meantime we need > something that works. We can ill afford to have release versions of > OpenSSH just not work especially when the newer versions cover > vulnerabilities. > > I really am also not seeing the vulnerability as to why you _need_ > to copy the child to the parent. I mean literally if a pam module is > allowing certain illegal behavior. It isn't the fault of SSH. If > someone gains root access to change the pam modules you are fried > anyway. What am I missing besides an out of the box release of > OpenSSH that actually works with PAM the way it is supposed to? >
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