openssh vs pam (was: Possible security flaw in OpenSSH and/or pam_krb5)

[Frank Cusack]

Darren (et al.),

Thanks for taking so much time here!  I wish I had more time to devote to it.

On June 22, 2005 10:19:21 AM +1000 Darren Tucker <dtucker at zip.com.au> wrote:
> Let me preface this with some things I think we can agree on:
>
> 1) sshd's PAM interface isn't perfect.
>
> 2) PAM itself isn't perfect either.
>
> I'd like to think sshd's PAM interface is improving.  We've certainly been working to that end.
> There is still room for improvement, no question.  (If you look at the open sshd PAM bugs you
> will see that some of them have been raised by me.)

Agreed up to here.

> It's never going to be perfect.  Given the SSHv1 and v2 protocol specs and PAM as it stands I
> don't think it's possible.

Disagree.  It is quite possible for ssh v2 to work properly with PAM.  I have it working.
Sun has it working.

> Now some of my opinions on PAM from working with it in the context of sshd.  (I'm not going to
> justify them although I will if people wish):
>
> 3) PAM's blocking callback conversation interface makes using it from sshd harder than it has to
> be.  Similarly functional authentication systems with reentrant interfaces can be used with much
> less code (like an order of magnitude less).

Agreed.

> 4) Several other of its characteristics make PAM as a whole harder for sshd to use.

OK.  (Don't know if I agree, but I'll give you this one.)

> 5) The differences between PAM implementations (and in some cases bugs in them) make it harder
> for sshd to work with all of them.

Sigh.  Yes.

> 6) The multiple-messages part of the PAM conversation protocol (ie allowing more than one message
> per call to the conversation function) is more complex than necessary and this has been a source
> of bugs.

A big no here.

This is a great example of both not understanding PAM and not wanting to.  One, you're wrong,
two, you can't imagine that the PAM interface might be the way it is for a reason, rather you
insist that it's broken and refuse to accommodate it.

Multiple messages are required because a) authentication exchanges are not limited to single
request/reply type of messages, and b) clients cannot derive any semantic meaning from
server-generated messages.

Gobbledygook, I know, but we need only look at password expiry, something you are quite
familiar, dare I say, expert, in for a concrete example.

pam_authenticate():
  info_request:  0: [enter password], type prompt
  info_response: 0: 1234
pam_acct_mgmt(): PAM_NEW_AUTHTOK_REQD
pam_chauthtok():
  info_request:  0: [your password has expired], type message
                 1: [old password], type prompt
                 2: [new password], type prompt
                 3: [verify], type prompt
  info_response: ...

1) Look at how difficult it is to do this (in ssh) without PAM, then get back to me
   about how PAM is broken.
2) Openssh tends to assume a CLI client, and even then handles this exchange incorrectly.
3) With a GUI client, openssh is especially broken here (multiple dialogs instead of
   one dialog).
4) This is a simple example.

I'm picking on password expiry because it is probably the most common issue, also since
that is/was one of your focuses (foci?) I figure you can appreciate the problem.  But
the same problem exists purely in the pam_authenticate() phase.

I threw in the comment about semantic meaning to head off any arguments that the client
might possibly understand that this is a password expiry and somehow coalesce messages.
The server can communicate in any language, not just ASCII english.  The client cannot
be expected to understand what is going on, nor should it.  Nor are multiple message
exchanges limited to password expiry.  So, PAM clients need to handle the general case.

Now, previously, in one sense I did overstate the case when I said "it's easy".  The
issue just described is a subtle one, and is probably not even understood by many of the
current PAM stack implementers and maintainers.  Even the authors of PAM don't describe
this in the PAM specification, and in fact they get a few things wrong!  (viz, they
claim kerberos and smartcard support, which isn't possible strictly within the PAM
framework as specified.)  OTOH, I give an explicit example of the need for multiple
messages in a single exchange in the kbdint draft, so this shouldn't be some new
or novel requirement for ssh implementers.  (I will say, though, that I took heat from
some WG folks for including such examples, but I insist on keeping them for what may now
be obvious reasons.)

So, sure, understanding the PAM API isn't exactly "easy", but it's certainly not "hard".
>From an implementation perspective, it IS easy though.  Perhaps not palatable, but still
easy.

You can either use a nested event loop, as Nico shows, or read incoming data from an
internal queue rather than the kernel packet buffer, as I have done (and I'm sure
I've submitted patches for).  Getting new ssh messages from a managed queue instead
of from the kernel read() interface allows you to "putback" messages, thus you can
escape the conversation function without nesting the event loop.  I didn't actually
implement a queue, I just buffered one message and added a hack to go back one
message, because the pam conversation is the only place it was necessary, so a general
purpose interface seemed unnecessarily complex.

The Sun patches are readily available.  Mine are not, and I'm sorry but I don't have
time to generate them, but it's easy enough to implement from scratch.

> Frank Cusack wrote:
>> On June 17, 2005 3:49:11 PM +1000 Damien Miller <djm at mindrot.org> wrote:
> [...]
>> But it does not follow the API correctly.  Isn't correctness a priority
>> for the openssh folks?  It's not just the conversation function handling
>> that's broken in openssh.
>
> Would you please describe this (or provide a pointer to what you're referring to)?

TEXT_INFO and ERROR_MSG buffering is broken (see password expiry example above for
one specific case).

pam_start() with "NOUSER" is broken (described verbosely on the list in the past).

pam_getenvlist() is broken (you already know this).

I think there's something specifically wrong with privsep and PAM but I'll be damned
if I can think of it now.  (I've never used privsep because it was historically
broken for PAM, nowadays some issues are fixed but there's no privsep value for me.)

>> Anyway, I withdraw my comment as I see (now that I examine recent
>> sources) that PAM support is steadily improving. Documenting threads
>> as an ugly hack is one of those nice improvements, IMHO. Why you
>> added it in the first place is beyond my understanding.
>
> It was in the FreeBSD code on which the current PAM code is based, and since then it's been the
> only general solution to modules that rely on pam_set_data/pam_get_data (in the main
> distribution, anyway).

OK, add that pam_set_data() and pam_get_data() is broken.  Although, I don't get it since
these should work.  Maybe the problem is that you fork a child (in addition to the privsep
fork) to do PAM; that's broken unless you start PAM in the forked child.  Just guessing;
I'm not familiar with the current implementation.  I remember when it was added though,
and I felt that the previous implementation was superior (for reasons that I've now
forgotten).

Frank