Re: SSL renegotiation

> If the renegotiation fails

AH! Now I remember. SSL clients can optionally renegotiate, it's not
required to renegotiate the session if the other side chooses not to
(almost certainly due to a bug or limitation in the client's connecting
library). By monkeying with the state, you can explicitly force a client
to renegotiate.

I don't think in code from yesteryear it was portable or possible to see
if the server successfully renegotiated a connection before 0.9.6, so
you just forced the client to renegotiate after the server and ignored
the result. A client pausing for a few extra round trips was probably
never noticed. I'm not saying this is correct, but I think that was the
thinking back in the day.

> , I suppose two things can be done:
>
> 1. Quit the connection

With my Infosec hat on, this is the correct option - force the client
back in to compliance with whatever the stated crypto policy is through
a reconnection.

> 2. Carry on pretending nothing happened.

This is almost never correct in a security context (all errors or
abnormalities must boil up).

> I think 2 is correct in the vast majority of cases (as it looks like
> is being done now).

That is a correct statement in that most code disregards renegotiation,
but that is because there is a pragmatic assumption that HTTPS
connections will be short lived. In the case of PostgreSQL, there is a
good chance that a connection will be established for weeks or months.
In the case of Apache, allowing a client to renegotiate every byte would
be a possible CPU DoS, but I digress....

> And in that case: not resetting port->count will make for a very slow
> and awkward connection as new handshakes will be attempted again and
> again,
> even if the other party persistently refuse to shake hands.

Which could lead to a depletion of random bits. This sounds like a
plausible explanation to me.

Too bad we're stuck using an ill-concieved SSL implementation and can't
use botan[1].

> I think this block is better written as:
>
> if (ssl_renegotiation_limit && port->count >
> ssl_renegotiation_limit * 1024L)

I don't think the " * 1024L" is right.

> {
> SSL_set_session_id_context(port->ssl, (void *)
> &SSL_context,
> sizeof(SSL_context));
> if (SSL_renegotiate(port->ssl) <= 0)
> ereport(COMMERROR,
> (errcode(ERRCODE_PROTOCOL_VIOLATION),
> errmsg("SSL renegotiation failure in
> renegotiate")));
> else
> {
> int handshake;
>
> do {
> handshake = SSL_do_handshake(port->ssl);
> if (handshake <= 0)
> ereport(COMMERROR,
> (errcode(ERRCODE_PROTOCOL_VIOLATION),
> errmsg("SSL renegotiation failure
> in handshake, retrying")));
> } while (handshake <= 0);

It's worth noting that for broken SSL implementations or SSL
implementations that refuse to renegotiate, this will be yield a stalled
connection, though at least it will be obvious in the logs. I think this
is the correct approach.

It is probably prudent to set an upper bound on this loop in order to
free up the resource and unblock the client who will appear to be
mysteriously hung for no reason until they look at the PostgreSQL server
logs.

> if (port->ssl->state != SSL_ST_OK)
> ereport(COMMERROR,
> (errcode(ERRCODE_PROTOCOL_VIOLATION),
> errmsg("SSL failed to send
> renegotiation request")));
> else
> port->count = 0;
> }
> }
>
> In other words, retry the SSL_do_handshake() until it reports OK, but
> not more than that; this seems to give better results in my
> (admittedly
> crude) experiments. I am unsure about checking port->ssl->state after
> the handshake; it's probably pointless, really.

Correct. In async IO, this would be important, but since the server is
synchronous in its handling of communication, we can remove the if/else
(state != SSL_ST_OK) block.

> In any case, the old code was calling SSL_do_handshake() even if
> SSL_renegotiate() failed; and it was resetting the port->count even if
> the handshake failed. Both of these smell like bugs to me.

I don't know how SSL_renegotiate() could fail in the past.
SSL_renegotiate(3) should never fail on a well formed implementation
(e.g. ssl/t1_reneg.c @ssl_add_serverhello_renegotiate_ext()).

While we're on the subject of crypto and OpenSSL, we force server
ciphers to be preferred instead of client ciphers:

SSL_CTX_set_options(SSL_context, SSL_OP_CIPHER_SERVER_PREFERENCE);

http://www.openssl.org/docs/ssl/SSL_CTX_set_options.html#NOTES

SSL_get_secure_renegotiation_support() would be a good call to add to
autoconf to see if it is supported (OpenSSL >= 0.9.8m), which would make
this code a good bit easier. Add a GUC,
ssl_renegotiation_require=(true,false,disconnect), which would force
renegotiations, ignore renegotiation failures, and disconnect a client
if it fails. As mentioned above re: breaking out of the loop, there
should probably be a ssl_renegotiation_min tunable so that the client
can't renegotiate too fast.

The default ssl_ciphers in the examples should also be updated as well
(e.g. we still allow SSLv2):

ssl_ciphers =
'ECDHE-RSA-AES128-SHA256:AES128-GCM-SHA256:RC4:HIGH:!MD5:!aNULL:!EDH:@STRENGTH'

Longer, but current with today's security standards.

-sc

[1] http://botan.randombit.net/tls.html?highlight=renegotiate

--
Sean Chittenden