Re: SSL renegotiation and other related woes

On 01/26/2015 12:14 PM, Andres Freund wrote:
> Hi,
>
> When working on getting rid of ImmediateInterruptOK I wanted to verify
> that ssl still works correctly. Turned out it didn't. But neither did it
> in master.
>
> Turns out there's two major things we do wrong:
>
> 1) We ignore the rule that once called and returning
> SSL_ERROR_WANTS_(READ|WRITE) SSL_read()/write() have to be called
> again with the same parameters. Unfortunately that rule doesn't mean
> just that the same parameters have to be passed in, but also that we
> can't just constantly switch between _read()/write(). Especially
> nonblocking backend code (i.e. walsender) and the whole frontend code
> violate this rule.

I don't buy that. Googling around, and looking at the man pages, I just
don't see anything to support that claim. I believe it is supported to
switch between reads and writes.

> 2) We start renegotiations in be_tls_write() while in nonblocking mode,
> but don't properly retry to handle socket readyness. There's a loop
> that retries handshakes twenty times (???), but what actually is
> needed is to call SSL_get_error() and ensure that there's actually
> data available.

Yeah, that's just crazy.

Actually, I don't think we need to call SSL_do_handshake() at all. At
least on modern OpenSSL versions. OpenSSL internally uses a state
machine, and SSL_read() and SSL_write() calls will complete the
handshake just as well.

> 2) is easy enough to fix, but 1) is pretty hard. Before anybody says
> that 1) isn't an important rule: It reliably causes connection aborts
> within a couple renegotiations. The higher the latency the higher the
> likelihood of aborts. Even locally it doesn't take very long to
> abort. Errors usually are something like "SSL connection has been closed
> unexpectedly" or "SSL Error: sslv3 alert unexpected message" and a host
> of other similar messages. There's a couple reports of those in the
> archives and I've seen many more in client logs.

Yeah, I can reproduce that. There's clearly something wrong.

I believe this is an OpenSSL bug. I traced the "unexpected message"
error to this code in OpenSSL's s3_read_bytes() function:

> case SSL3_RT_APPLICATION_DATA:
> /*
> * At this point, we were expecting handshake data, but have
> * application data. If the library was running inside ssl3_read()
> * (i.e. in_read_app_data is set) and it makes sense to read
> * application data at this point (session renegotiation not yet
> * started), we will indulge it.
> */
> if (s->s3->in_read_app_data &&
> (s->s3->total_renegotiations != 0) &&
> (((s->state & SSL_ST_CONNECT) &&
> (s->state >= SSL3_ST_CW_CLNT_HELLO_A) &&
> (s->state <= SSL3_ST_CR_SRVR_HELLO_A)
> ) || ((s->state & SSL_ST_ACCEPT) &&
> (s->state <= SSL3_ST_SW_HELLO_REQ_A) &&
> (s->state >= SSL3_ST_SR_CLNT_HELLO_A)
> )
> )) {
> s->s3->in_read_app_data = 2;
> return (-1);
> } else {
> al = SSL_AD_UNEXPECTED_MESSAGE;
> SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_UNEXPECTED_RECORD);
> goto f_err;
> }

So that quite clearly throws an error, *unless* the original application
call was SSL_read(). As you also concluded, OpenSSL doesn't like it when
SSL_write() is called when it's in renegotiation. I think that's
OpenSSL's fault and it should "indulge" the application data message
even in SSL_write().

Can we work-around that easily? I believe we can. The crucial part is
that we mustn't let SSL_write() to process any incoming application
data. We can achieve that if we always call SSL_read() to drain such
data, before calling SSL_write(). But that's not quite enough; if we're
in renegotiation, SSL_write() might still try to read more data from the
socket that has arrived after the SSL_read() call. Fortunately, we can
easily prevent that by hacking pqsecure_raw_read() to always return
EWOULDBLOCK, when it's called through SSL_write().

The attached patch does that. I've been running your pg_receivexlog test
case with this for about 15 minutes without any errors now, with
ssl_renegotiation_limit=50kB, while before it errored out within seconds.

In theory, I guess we should do similar hacks in the server, and always
call SSL_read() before SSL_write(), but it seems to work without it. Or
maybe not; OpenSSL server and client code is not symmetric, so perhaps
it works in server mode without that.

PS. The server code started renegotiation when it's 1kB from reaching
ssl_renegotiation_limit. I increased that to 32kB, because in testing, I
got some "SSL failed to renegotiate connection before limit expired"
errors in testing before I did that.

PPS. I did all testing of this patch with my sleeping logic
simplification patch applied
(http://www.postgresql.org/message-id/54D3821E.9060004@vmware.com). It
applies without it too, and I don't think it matters, but I thought I'd
mention it.

- Heikki