Re: SSL renegotiation and other related woes

Hi,

On 2015-02-05 23:03:02 +0200, Heikki Linnakangas wrote:
> 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.

There's at least two implementations that seem to have workaround to
achieve something like that. Apache's mod_ssl and the ssl layer for
libevent.

> >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.

Some blaming seems to show that that's been the case for a long time.

> >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:

Yep, I got to that as well.

> > 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().

That'd be nice, but I think there were multiple reports to them about
this and there wasn't much of a response. Maybe it's time to try again.

> 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.

I think we should do it on the server side - got some server side errors
when I cranked up the pg_receivexlog's status interval and set the wal
sender timeout very low.

Greetings,

Andres Freund