Thread: pq_eof() broken with SSL

I came across another bug in the SSL code.  backend/libpq/pqcomm.c:pq_eof()
calls recv() to read a single byte of data to check for EOF.  The
character is then stuffed into the read buffer.

This will not work with SSL.  Besides the data being encrypted, you
could end up reading a byte from an SSL control message instead of a
data message, or messing up counts.  Fortunately this procedure only
seems to be called in some password code - if you use 'trust' or 'ident'
then the SSL should work fine.

The quick fix is to add another USE_SSL block, a better fix is to
explicitly create a new abstraction layer.

Bear

Bear Giles writes:

> I came across another bug in the SSL code.  backend/libpq/pqcomm.c:pq_eof()
> calls recv() to read a single byte of data to check for EOF.  The
> character is then stuffed into the read buffer.

> The quick fix is to add another USE_SSL block,

So it seems.  Do you volunteer to do that?

> a better fix is to explicitly create a new abstraction layer.

Well, this is supposed to be an abstraction already. ;-)

-- 
Peter Eisentraut   peter_e@gmx.net

> > a better fix is to explicitly create a new abstraction layer.
> 
> Well, this is supposed to be an abstraction already. ;-)
The new abstraction layer would localize SSL vs. plain sockets, and
possibly SASL as well.

The SSL issues I've identified to date are:

critical
- no check for SSL_get_error() after reads and writes. (*)
- code assumes zero bytes read or write indicates an error.  This isn't necessarily the case with SSL because of
control messages.
 

severe
- pq_eof() fails on SSL.  Since this is localized to the password  handling code, I don't consider this error critical
sincethe  system can reliably work provided known problematic conditions  are avoided.
 
- both front- and back-end should call SSL_shutdown() immediately  prior to closing connection. (1/2 *)
- private keys should be regular files with mode 0600 or 0400. (*)  they should be owned by the running process.
- backend should use empheral DH keys.
- encrypted private keys should be supported.

important
- client cert handling. (*)
- makecert(?), a tool to generate PostgreSQL server certs.  It is comparable in function to Apache mod-ssl script of
thesame name, and should be run when installing database  if SSL is enabled.
 
- pgkeygen(?), a tool to generate client certificates.  It is  comparable to sshkeygen for SSH.
- client and server should migrate to TLS.
- connections should expire after a period of inactivity.
- clients should provide identification of remote system to  user. (*)
- clients should verify that the server cert identifies the  server.  (server "common name" should resolve to IP
address of server.)
 
- DSA keys should work.

ongoing
- change protocol to use 'STARTTLS' type negotiation, instead  of current approach.
- SASL?
- using client certs for authentication

unknown
- I'm not sure select() is guaranteed to work with SSL.

(*) have had patches submitted, but may be superceded by subsequent
patches.


Unfortunately, I'm not sure that this list is complete - I'm still
doing research.  The patches I already submitted are fairly straight-
forward - OpenSSL contains sample clients and servers that demonstrate
good techniques.  Right now I'm cross-checking the code with my
_SSL and TLS_ book to make sure there aren't other holes, and that
takes time.

I hadn't planned on doing any of this, but I got caught up in it while
setting up snort to log to PostgreSQL via an encrypted channel.  As 
an aside, this is a good example of a case where an SSH tunnel is 
inadequate!

So to answer the question I clipped, I'm looking at it but I don't
want to do a half-assed solution.  But as the scope of the solution
expands, it becomes more important to have consensus that something
needs to be done and this is the right solution.  So right now I'm
not ready to make any commitments.

Bear