Fix for USE_POSIX_THREADS in auth-pam.c

[Steven Michaud]

 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
>