Re: [HACKERS] SSL renegotiation
On 02/23/2015 04:01 PM, Albe Laurenz wrote: I think you could remove renegotiation from PostgreSQL as long as you offer something better than RC4 in the TLS handshake. I'd say it is best to wait if and how OpenSSL change their API when they implement TLS 1.3. I'd vote against removing renegotiation. I'm just suggesting that the effort required to fix bugs in this part of PostgreSQL could be spent better elsewhere. If changing the encryption is so useless, whe did the TLS workgroup decide to introduce rekeying as a substitute for renegotiation? Theoretical considerations, mostly. If rekeying is strictly required after processing just a few petabytes, the cipher is severely broken and should no longer be used. -- Florian Weimer / Red Hat Product Security -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On 02/22/2015 02:05 PM, Andres Freund wrote: On 2015-02-22 01:27:54 +0100, Emil Lenngren wrote: I honestly wonder why postgres uses renegotiation at all. The motivation that cryptoanalysis is easier as more data is sent seems quite far-fetched. I don't think so. There's a fair number of algorithms that can/could be much easier be attached with lots of data available. Especially if you can guess/know/control some of the data. Additionally renegotiating regularly helps to constrain a possible key leagage to a certain amount of time. With backend connections often being alive for weeks at a time that's not a bad thing. Renegotiation will be removed from future TLS versions because it is considered unnecessary with modern ciphers: https://github.com/tlswg/tls13-spec/issues/38 If ciphers require rekeying, that mechanism will be provided at the TLS layer in the future. I think you could remove renegotiation from PostgreSQL as long as you offer something better than RC4 in the TLS handshake. -- Florian Weimer / Red Hat Product Security -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Florian Weimer wrote: On 02/22/2015 02:05 PM, Andres Freund wrote: On 2015-02-22 01:27:54 +0100, Emil Lenngren wrote: I honestly wonder why postgres uses renegotiation at all. The motivation that cryptoanalysis is easier as more data is sent seems quite far-fetched. I don't think so. There's a fair number of algorithms that can/could be much easier be attached with lots of data available. Especially if you can guess/know/control some of the data. Additionally renegotiating regularly helps to constrain a possible key leagage to a certain amount of time. With backend connections often being alive for weeks at a time that's not a bad thing. Renegotiation will be removed from future TLS versions because it is considered unnecessary with modern ciphers: https://github.com/tlswg/tls13-spec/issues/38 If ciphers require rekeying, that mechanism will be provided at the TLS layer in the future. I think you could remove renegotiation from PostgreSQL as long as you offer something better than RC4 in the TLS handshake. I'd say it is best to wait if and how OpenSSL change their API when they implement TLS 1.3. I'd vote against removing renegotiation. At the very least, if the feature should provide unnecessary and cumbersome with future versions of OpenSSL, we should retain ssl_renegotiation_limit and change the default to 0. It might still be of value with older versions. If changing the encryption is so useless, whe did the TLS workgroup decide to introduce rekeying as a substitute for renegotiation? Yours, Laurenz Albe -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On 2015-02-23 15:15:31 +0100, Florian Weimer wrote: On 02/22/2015 02:05 PM, Andres Freund wrote: On 2015-02-22 01:27:54 +0100, Emil Lenngren wrote: I honestly wonder why postgres uses renegotiation at all. The motivation that cryptoanalysis is easier as more data is sent seems quite far-fetched. I don't think so. There's a fair number of algorithms that can/could be much easier be attached with lots of data available. Especially if you can guess/know/control some of the data. Additionally renegotiating regularly helps to constrain a possible key leagage to a certain amount of time. With backend connections often being alive for weeks at a time that's not a bad thing. Renegotiation will be removed from future TLS versions because it is considered unnecessary with modern ciphers: https://github.com/tlswg/tls13-spec/issues/38 If ciphers require rekeying, that mechanism will be provided at the TLS layer in the future. I think you could remove renegotiation from PostgreSQL as long as you offer something better than RC4 in the TLS handshake. As far as I understand it, this argument misses an important point. Without protocol level rekeying, handled by the library in the background, never changing the session key pretty much breaks PFS. In http the sessions almost always are short enough that such a consideration doesn't play a role, but it's far from uncommon to have database connections that live weeks and transport hundreds of gigabytes in their lifetime. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Renegotiation should be a best practice. Trouble is it's been broken (at the protocol level) three times in the last few years so it's a massive hole in practice. Ideally we should leave the renegotiate in, and only remove it if configure detects a broken version of TLS. Personal email. hbh...@oxy.edu On Feb 23, 2015, at 7:01 AM, Albe Laurenz laurenz.a...@wien.gv.at wrote: I'd say it is best to wait if and how OpenSSL change their API when they implement TLS 1.3. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On 2015-02-22 01:27:54 +0100, Emil Lenngren wrote: I honestly wonder why postgres uses renegotiation at all. The motivation that cryptoanalysis is easier as more data is sent seems quite far-fetched. I don't think so. There's a fair number of algorithms that can/could be much easier be attached with lots of data available. Especially if you can guess/know/control some of the data. Additionally renegotiating regularly helps to constrain a possible key leagage to a certain amount of time. With backend connections often being alive for weeks at a time that's not a bad thing. And it's not just us. E.g. openssh also triggers renegotiations based on the amount of data sent. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation and other related woes
On 02/12/2015 01:41 AM, Andres Freund wrote:
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.
I've not been able to reproduce any errors on the server side, with my
latest client-only patch. Not sure why. I would assume that the server
has the same problem, but perhaps there are some mitigating factors that
make it impossible or at least less likely there.
I'm planning to commit and backpatch the first two of the attached
patches. The first is essentially the same as what I've posted before,
with some minor cleanup. I realized that the hack can be isolated
completely in fe-secure-openssl.c by putting the kill-switch in the
custom BIO_read routine, instead of
Re: [HACKERS] SSL renegotiation and other related woes
On 02/12/2015 01:33 AM, Andres Freund wrote: On 2015-02-11 14:54:03 +0200, Heikki Linnakangas wrote: Thoughts? Can you reproduce any errors with this? I'm on battery right now, so I can't really test much. Did you test having an actual standby instead of pg_receivexlog? I saw some slightly different errors there. Does this patch alone work for you or did you test it together with the earlier one to fix the renegotiation handling server side when nonblocking? Because that frequently seemed to error out for me, at least over actual network. A latch loop + checking for the states seemed to fix that for me. This patch alone worked for me. +* NB: This relies on the calling code to call pqsecure_read(), completing +* the renegotiation handshake, if pqsecure_write() returns 0. Otherwise +* we'll never make progress. +*/ I think this should a) refer to the fact that pqSendSome does that in blocking scenarios b) expand the comment around the pqReadData to reference that fact. How are we going to deal with callers using libpq in nonblocking mode. A client doing a large COPY FROM STDIN to the server using a nonblocking connection (not a stupid thing to do) could hit this IIUC. Hmm, that's problematic even without SSL. If you do a large COPY FROM STDIN, and the server sends a lot of NOTICEs, the NOTICEs will be accumulated in the TCP send and receive buffers until they fill up. After that, the server will block on the send, and will stop reading the tuple data the client sends, and you get a deadlock. In blocking mode, pqSendSome calls pqReadData to avoid that. I think pqSendSome should call pqReadData() after getting EWOULDBLOCK, even in non-blocking mode. It won't completely prevent the problem: it's possible that the receive buffer fills up, but the application doesn't call pqSendSome() until the socket becomes writeable, which won't happen until the client drains the incoming data is drained and unblocks the server. But it greatly reduces the risk. In practice that will solve the problem for SSL renegotiations. We should also document that the application should always wait for the socket readable condition, in addition to writeable, and call pqConsumeInput(). That fixes the issue for good. Could we perhaps call SSL_peek() when SSL_write() hits WANTS_READ? In combination with your fix I think that should fix the possibility of such a deadlock? Especially on the serverside where the socket most of the time is is in, although emulated, blocking mode that seems easier - we can just retry afterwards. Hmm. Not sure what the semantics of SSL_peek() are. At least we'd need to call it with a large enough buffer that it would read all the incoming data from the OS buffer. I think it would be safer to just call SSL_read(). Both the server and libpq have an input buffer that you can easily read into at any time. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation and other related woes
On 2015-02-11 14:54:03 +0200, Heikki Linnakangas wrote: Thoughts? Can you reproduce any errors with this? I'm on battery right now, so I can't really test much. Did you test having an actual standby instead of pg_receivexlog? I saw some slightly different errors there. Does this patch alone work for you or did you test it together with the earlier one to fix the renegotiation handling server side when nonblocking? Because that frequently seemed to error out for me, at least over actual network. A latch loop + checking for the states seemed to fix that for me. I'm pretty darn happy that you seem to have found a much less ugly solution than mine. Although it's only pretty by comparison ;) + /* + * To work-around an issue with OpenSSL and renegotiation, we don't + * let SSL_write() to read any incoming data. superflous to. + * NB: This relies on the calling code to call pqsecure_read(), completing + * the renegotiation handshake, if pqsecure_write() returns 0. Otherwise + * we'll never make progress. + */ I think this should a) refer to the fact that pqSendSome does that in blocking scenarios b) expand the comment around the pqReadData to reference that fact. How are we going to deal with callers using libpq in nonblocking mode. A client doing a large COPY FROM STDIN to the server using a nonblocking connection (not a stupid thing to do) could hit this IIUC. Could we perhaps call SSL_peek() when SSL_write() hits WANTS_READ? In combination with your fix I think that should fix the possibility of such a deadlock? Especially on the serverside where the socket most of the time is is in, although emulated, blocking mode that seems easier - we can just retry afterwards. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation and other related woes
On 02/05/2015 11:03 PM, Heikki Linnakangas wrote:
On 01/26/2015 12:14 PM, Andres Freund wrote:
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.
Here is a simplified version of this patch. It seems to be enough to not
let SSL_write() to read any data from the socket. No need to call
SSL_read() first, and the server-side changes I made were only needed
because of the other patch I had applied.
Thoughts? Can you reproduce any errors with this?
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.
Not included in this patch, but I believe we apply a similar patch to
the server-side too.
- Heikki
From b78bfbb70127cbe3f360458eb2a97dcc28194fbc Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas heikki.linnakangas@iki.fi
Date: Thu, 5 Feb 2015 22:44:58 +0200
Subject: [PATCH 1/1] Fix sslv3 alert unexpected message errors in SSL
renegotiation.
There seems to be a bug in OpenSSL renegotiation, so that when SSL_write()
needs to read data to complete the handshake, but it receives application
data instead, it gets confused and throws an unexpected message error. To
work around that, don't let SSL_write() to read data from the socket, even
if some is available. Arrange so that SSL_read() processes all incoming
messages instead.
---
src/interfaces/libpq/fe-misc.c | 14 +-
src/interfaces/libpq/fe-secure-openssl.c | 20
src/interfaces/libpq/fe-secure.c | 9 +
src/interfaces/libpq/libpq-int.h | 2 ++
4 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 945e283..9dd02d5 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -920,11 +920,15 @@ pqSendSome(PGconn *conn, int len)
* to clear the channel eventually because it's blocked trying to
* send data to us. (This can happen when we are sending a large
* amount of COPY data, and the server has generated lots of
- * NOTICE responses.) To avoid a deadlock situation, we must be
- * prepared to accept and buffer incoming data before we try
- * again. Furthermore, it is possible that such incoming data
- * might not arrive until after we've gone to sleep. Therefore,
- * we wait for either read ready or write ready.
+ * NOTICE responses.) Another scenario is that SSL renegotiation
+ * is in progress, and the SSL library needs to read a message
+ * to complete the handshake, before it can send more data.
+ *
+ * To avoid a deadlock situation, we must be prepared to accept
+ * and buffer incoming data before we try again. Furthermore, it
+ * is possible that such incoming data might not arrive until
+ * after we've gone to sleep. Therefore, we wait for either read
+ * ready or write ready.
*/
if (pqReadData(conn) 0)
{
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index a32af34..2e75f3c 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -319,8 +319,28 @@ pgtls_write(PGconn *conn, const void *ptr, size_t len)
char sebuf[256];
int err;
+ /*
+ * To work-around an issue with OpenSSL and renegotiation, we don't
+ * let SSL_write() to read any incoming data.
+ *
+ * If SSL_write() is called, and renegotiation has just been inititated,
+ * SSL_write() might try to read from the socket to complete the
+ * handshake. If there was some application data in-flight, it might
+ * receive the application data instead. That confuses it, and it throws
+ * an sslv3 alert unexpected message error.
+ *
+ * We avoid that setting a kill-switch, pqsecure_block_raw_read, which
+ * tells pqsecure_raw_read() to not return any data to the caller, even
+ * if some is available.
+ *
+ * NB: This relies on the calling code to call pqsecure_read(), completing
+ * the renegotiation handshake, if pqsecure_write() returns 0. Otherwise
+ * we'll never make progress.
+ */
SOCK_ERRNO_SET(0);
+ pqsecure_block_raw_read = true;
n =
Re: [HACKERS] 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.9060...@vmware.com). It
applies without it too, and I don't think it matters, but I thought
Re: [HACKERS] SSL renegotiation and other related woes
On 2015-01-26 11:14:05 +0100, Andres Freund wrote: My testcase for this is just to setup a server with a low ssl_renegotiation_limit, generate lots of WAL (wal.sql attached) and receive data via pg_receivexlog -n. Usually it'll error out quickly. ... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services DROP TABLE IF EXISTS foo_:client_id; CREATE TABLE foo_:client_id AS SELECT * FROM generate_series(1, 100) a(a), generate_series(1, 1) b(b); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-11-15 10:43:23 -0500, Tom Lane wrote: Another reason I'm not in a hurry is that the problem we're trying to solve doesn't seem to be causing real-world trouble. So by awhile, I'm thinking let's let it get through 9.4 beta testing. Well, there have been a bunch of customer complaints about it, afair that's what made Alvaro look into it in the first place. So it's not a victimless bug. OK, then maybe end-of-beta is too long. But how much testing will it get during development? I know I never use SSL on development installs. How many hackers do? Just a reminder that I intend to backpatch this (and subsequent fixes). We've gone over two 9.4 betas now. Maybe it'd be a good thing if the beta3 announcement carried a note about enabling SSL with a low ssl_renegotiation_limit setting. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Mon, Aug 25, 2014 at 11:46:13PM -0400, Alvaro Herrera wrote: Tom Lane wrote: OK, then maybe end-of-beta is too long. But how much testing will it get during development? I know I never use SSL on development installs. How many hackers do? Just a reminder that I intend to backpatch this (and subsequent fixes). We've gone over two 9.4 betas now. Maybe it'd be a good thing if the beta3 announcement carried a note about enabling SSL with a low ssl_renegotiation_limit setting. To elaborate on my private comments of 2013-10-11, I share Robert's wariness[1] concerning the magic number of 1024 bytes of renegotiation headroom. Use of that number predates your work, but your work turned exhaustion of that headroom into a FATAL error. Situations where the figure is too small will become disruptive, whereas the problem is nearly invisible today. Network congestion is a factor, so the lack of complaints during beta is relatively uninformative. Disabling renegotiation is a quick workaround, fortunately, but needing to use that workaround will damage users' fragile faith in the safety of our minor releases. My recommendation is to either keep this 9.4-only or do focused load testing to determine the actual worst-case headroom requirement. [1] http://www.postgresql.org/message-id/ca+tgmozvgmyzlx7e4arq_5nu4uden7wrvg1xjxg_o9c61ch...@mail.gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Fri, Nov 15, 2013 at 10:49 AM, Andres Freund and...@2ndquadrant.com wrote: Another reason I'm not in a hurry is that the problem we're trying to solve doesn't seem to be causing real-world trouble. So by awhile, I'm thinking let's let it get through 9.4 beta testing. Well, there have been a bunch of customer complaints about it, afair that's what made Alvaro look into it in the first place. So it's not a victimless bug. Well, can any of those people try running with this patch? That'd be a good way of getting some confidence in it. Generally, I agree that something needs to be back-patched here. But we don't want to create a situation where we fix some people and break others, and it's not too obvious that we have a way to get there. Personally, I favor adding some kind of GUC to control the behavior, but I'm not exactly sure what the shape of it ought to be. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
So I committed this patch without backpatching anything. There was some discussion about the exact strategy for backpatching the behavior change, but no final agreement; the suggestions were 0. Backpatch as an ERROR. If it causes failures, people is supposed to change their apps or something. 1. Don't backpatch the ERROR bit at all, so that if the renegotiation fails we would silently continue just as currently 2. Do spit the message, but only as a WARNING. Thinks this may end up causing log disks to fill up. 3. Add it as an ERROR, but make it possible to disable it, presumably via a new GUC. So people can see their security problems and hopefuly fix them, but if they don't, then they can shut it up via server configuration. This would entail a GUC variable that exists in existing branches but not HEAD (we could avoid an upgradability problem of postgresql.conf files by having a no-op phantom GUC variable in HEAD). I was reminded of this once more because I just saw a spurious renegotiation failure in somebody's production setup. Kind of like a recurring nightmare which I thought I had already erradicated. Opinions? Also, should we wait longer for the new renegotiation code to be more battle-tested? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: 1. Don't backpatch the ERROR bit at all, so that if the renegotiation fails we would silently continue just as currently I'm leaning towards the above at this point. I was reminded of this once more because I just saw a spurious renegotiation failure in somebody's production setup. Kind of like a recurring nightmare which I thought I had already erradicated. I saw one yesterday. :( Opinions? Also, should we wait longer for the new renegotiation code to be more battle-tested? I've got a better environment to test this in now and given that I saw it just yesterday, I'm very interested in addressing it. I grow tired of seeing these renegotiation errors. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] SSL renegotiation
On 2013-11-15 10:43:23 -0500, Tom Lane wrote: +1 to waiting awhile. I think if we don't see any problems in HEAD, then back-patching as-is would be the best solution. The other alternatives are essentially acknowledging that you're back-patching something you're afraid isn't production ready. Let's not go there. Agreed. Both on just backpatching it unchanged and waiting for the fix to prove itself a bit. Another reason I'm not in a hurry is that the problem we're trying to solve doesn't seem to be causing real-world trouble. So by awhile, I'm thinking let's let it get through 9.4 beta testing. Well, there have been a bunch of customer complaints about it, afair that's what made Alvaro look into it in the first place. So it's not a victimless bug. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Alvaro Herrera alvhe...@2ndquadrant.com writes: So I committed this patch without backpatching anything. ... ... should we wait longer for the new renegotiation code to be more battle-tested? +1 to waiting awhile. I think if we don't see any problems in HEAD, then back-patching as-is would be the best solution. The other alternatives are essentially acknowledging that you're back-patching something you're afraid isn't production ready. Let's not go there. Another reason I'm not in a hurry is that the problem we're trying to solve doesn't seem to be causing real-world trouble. So by awhile, I'm thinking let's let it get through 9.4 beta testing. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Andres Freund and...@2ndquadrant.com writes: On 2013-11-15 10:43:23 -0500, Tom Lane wrote: Another reason I'm not in a hurry is that the problem we're trying to solve doesn't seem to be causing real-world trouble. So by awhile, I'm thinking let's let it get through 9.4 beta testing. Well, there have been a bunch of customer complaints about it, afair that's what made Alvaro look into it in the first place. So it's not a victimless bug. OK, then maybe end-of-beta is too long. But how much testing will it get during development? I know I never use SSL on development installs. How many hackers do? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On 2013-11-15 10:58:19 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-11-15 10:43:23 -0500, Tom Lane wrote: Another reason I'm not in a hurry is that the problem we're trying to solve doesn't seem to be causing real-world trouble. So by awhile, I'm thinking let's let it get through 9.4 beta testing. Well, there have been a bunch of customer complaints about it, afair that's what made Alvaro look into it in the first place. So it's not a victimless bug. OK, then maybe end-of-beta is too long. But how much testing will it get during development? I know I never use SSL on development installs. How many hackers do? I guess few. And even fewer will actually have connections that live long enough to experience renegotiations :/. I wonder how hard it'd be to rig the buildfarm code to generate ssl certificates and use them during installcheck. If we'd additionally set a low renegotiation limit... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On 09/23/2013 10:51 PM, Alvaro Herrera wrote: + /* are we in the middle of a renegotiation? */ + static bool in_ssl_renegotiation = false; + Since this was committed, I'm getting the following warning: be-secure.c:105:13: warning: ‘in_ssl_renegotiation’ defined but not used [-Wunused-variable] -- Vik -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Since back branches releases are getting closer, I would like to push this to all supported branches. To avoid a compatibility nightmare in case the new die-on-delayed-renegotiation behavior turns out not to be so great, I think it would be OK to set the error level to WARNING in all branches but master (and reset the byte count, to avoid filling the log). I would also add a CONTEXT line with the current counter value and the configured limit, and a HINT to report to pg-hackers. That way we will hopefully have more info on problems in the field. Anybody opposed to this? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Tue, Oct 1, 2013 at 9:16 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Since back branches releases are getting closer, I would like to push this to all supported branches. To avoid a compatibility nightmare in case the new die-on-delayed-renegotiation behavior turns out not to be so great, I think it would be OK to set the error level to WARNING in all branches but master (and reset the byte count, to avoid filling the log). I would also add a CONTEXT line with the current counter value and the configured limit, and a HINT to report to pg-hackers. That way we will hopefully have more info on problems in the field. Anybody opposed to this? Yes, warning suck. If things just failed, users would fix them, but instead they fill up their hard disk, and then things fail much later, usually when they are asleep in bed. If we can't feel comfortable with an ERROR, let's not do it at all. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Tue, Oct 1, 2013 at 4:17 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Oct 1, 2013 at 9:16 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Since back branches releases are getting closer, I would like to push this to all supported branches. To avoid a compatibility nightmare in case the new die-on-delayed-renegotiation behavior turns out not to be so great, I think it would be OK to set the error level to WARNING in all branches but master (and reset the byte count, to avoid filling the log). I would also add a CONTEXT line with the current counter value and the configured limit, and a HINT to report to pg-hackers. That way we will hopefully have more info on problems in the field. Anybody opposed to this? Yes, warning suck. If things just failed, users would fix them, but instead they fill up their hard disk, and then things fail much later, usually when they are asleep in bed. If we can't feel comfortable with an ERROR, let's not do it at all. In principle, I agree. However, if we want to do this as a temporary measure to judge impact, we could do WARNING now and flip it to ERROR in the next minor release. However, do we think we'll actually *get* any reports in of it if we do that? As in would we trust the input? If we do, the it might be worth doing that. If we don't believe we'll get any input from the WARNINGs anyway, we might as well flip it to an ERROR. But if we do flip it to an ERROR, we should have a way to disable that error if, as Alvaro puts it, we end up breaking too many things. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Tue, Oct 1, 2013 at 10:19 AM, Magnus Hagander mag...@hagander.net wrote: If we can't feel comfortable with an ERROR, let's not do it at all. In principle, I agree. However, if we want to do this as a temporary measure to judge impact, we could do WARNING now and flip it to ERROR in the next minor release. I can't imagine that whacking the behavior around multiple times is going to please very many people. And, from a practical standpoint, the time between minor releases is typically on the order of ~3 months. That's not very long. The situations in which trouble occurs are likely to obscure, and a lot of users don't apply every minor release, or don't apply them in a timely fashion. So I can't see that this strategy would increase our confidence very much anyway. In other words... However, do we think we'll actually *get* any reports in of it if we do that? As in would we trust the input? ...no. If we do, the it might be worth doing that. If we don't believe we'll get any input from the WARNINGs anyway, we might as well flip it to an ERROR. But if we do flip it to an ERROR, we should have a way to disable that error if, as Alvaro puts it, we end up breaking too many things. A way of disabling the error seems like an awfully good idea. Since I know my audience, I won't suggest the obvious method of accomplishing that goal, but I think we all know what it is. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On 2013-10-01 10:27:14 -0400, Robert Haas wrote: On Tue, Oct 1, 2013 at 10:19 AM, Magnus Hagander mag...@hagander.net wrote: If we can't feel comfortable with an ERROR, let's not do it at all. In principle, I agree. However, if we want to do this as a temporary measure to judge impact, we could do WARNING now and flip it to ERROR in the next minor release. I can't imagine that whacking the behavior around multiple times is going to please very many people. If we have to whack it around, it's because there's a bug in either our usage of openssl or in openssl itself. Neither is particularly unlikely, but it's not like not erroring or warning out will stop that. The alternate reason for getting the WARNING/ERROR is that there is somebody MITMing the connection and explicitly corrupting renegotiation packets. But that's a reason for making it an ERROR immediately, not making it silent. I think from the security POV it's pretty clear that we should make it an error. So, imo the question is why are we uncomfortable making it an ERROR immediately? I think the most likely reason for problems is users having configured ssl_renegotiation_limit absurdly low, like less than what the particular algorithms used actually need. What about clamping it to 1MB if != 0? We can't make it an actual error in the backbranches... If we do, the it might be worth doing that. If we don't believe we'll get any input from the WARNINGs anyway, we might as well flip it to an ERROR. But if we do flip it to an ERROR, we should have a way to disable that error if, as Alvaro puts it, we end up breaking too many things. A way of disabling the error seems like an awfully good idea. Since I know my audience, I won't suggest the obvious method of accomplishing that goal, but I think we all know what it is. In which scenario would you want to do that? The way to prevent the ERROR it is to disable renegotiation entirely. And that's already possible. Anything else seems like papering over security issues. I guess I am voting for making renegotiation failure an ERROR everywhere and silently clamp renegotiation_limit to a lower bound of 1MB in the backbranches while making the (0, 1MB) an error in HEAD. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Mon, Sep 23, 2013 at 4:51 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Here's an updated version; this mainly simplifies code, per comments from Andres (things were a bit too baroque in places due to the way the code had evolved, and I hadn't gone over it to simplify it). The only behavior change is that the renegotiation is requested 1kB before the limit is hit: the raise to 1% of the configured limit was removed. What basis do we have for thinking that 1kB is definitely enough to avoid spurious disconnects? (I have a bad feeling that you're going to say something along the lines of well, we tried it a bunch of times, and) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Robert Haas escribió: On Mon, Sep 23, 2013 at 4:51 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Here's an updated version; this mainly simplifies code, per comments from Andres (things were a bit too baroque in places due to the way the code had evolved, and I hadn't gone over it to simplify it). The only behavior change is that the renegotiation is requested 1kB before the limit is hit: the raise to 1% of the configured limit was removed. What basis do we have for thinking that 1kB is definitely enough to avoid spurious disconnects? I noticed that the count variable (which is what we use to determine when to start the renegotiation and eventually kill the connection) is only incremented when there's successful SSL transmission: it doesn't count low-level network transmission. If OpenSSL returns a WANT_READ or WANT_WRITE error code, that variable is not incremented. The number of bytes returned does not include network data transmitted only to satisfy the renegotiation. Sadly, with the OpenSSL codebase, there isn't much documented field experience to go by. Even something battle-tested such as Apache's mod_ssl gets this wrong; but apparently they don't care because their sessions are normally so short-lived that they don't get these problems. Also, I spent several days trying to understand the OpenSSL codebase to figure out how this works, and I think there might be bugs in there too, at least with nonblocking sockets. I wasn't able to reproduce an actual failure, though. Funnily enough, their own test utilities do not stress this area too much (at least the ones they include in their release tarballs). (I have a bad feeling that you're going to say something along the lines of well, we tried it a bunch of times, and) Well, I did try a few times and saw no failure :-) I have heard about processes in production environments that are restarted periodically to avoid SSL failures which they blame on renegotiation. Some other guys have ssl_renegotiation_limit=0 because they know it causes network problems. I suggest we need to get this patch out there, so that they can test it; and if 1kB turns out not to be sufficient, we will have field experience including appropriate error messages on what is actually going on. (Right now, the error messages we get are complaining about completely the wrong thing.) I mean, if that 1kB limit is the only quarrel you have with this patch, I'm happy. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Tue, Sep 24, 2013 at 12:30 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I mean, if that 1kB limit is the only quarrel you have with this patch, I'm happy. You should probably be happy, then. :-) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Here's an updated version; this mainly simplifies code, per comments
from Andres (things were a bit too baroque in places due to the way the
code had evolved, and I hadn't gone over it to simplify it).
The only behavior change is that the renegotiation is requested 1kB
before the limit is hit: the raise to 1% of the configured limit was
removed.
The fixup is an incremental on top of the previous version; the other
one is the full v2 patch.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
*** a/src/backend/libpq/be-secure.c
--- b/src/backend/libpq/be-secure.c
***
*** 324,344 secure_write(Port *port, void *ptr, size_t len)
if (port-ssl)
{
int err;
- static long renegotiation_count = 0;
/*
* If SSL renegotiations are enabled and we're getting close to the
* limit, start one now; but avoid it if there's one already in
! * progress. Request the renegotiation before the limit has actually
! * expired; we give it 1% of the specified limit, or 1kB, whichever is
! * greater.
*/
if (ssl_renegotiation_limit !in_ssl_renegotiation
! port-count Min((ssl_renegotiation_limit - 1) * 1024L,
! (long) rint(ssl_renegotiation_limit * 1024L * 0.99)))
{
in_ssl_renegotiation = true;
SSL_set_session_id_context(port-ssl, (void *) SSL_context,
sizeof(SSL_context));
if (SSL_renegotiate(port-ssl) = 0)
--- 324,352
if (port-ssl)
{
int err;
/*
* If SSL renegotiations are enabled and we're getting close to the
* limit, start one now; but avoid it if there's one already in
! * progress. Request the renegotiation 1kB before the limit has
! * actually expired.
*/
if (ssl_renegotiation_limit !in_ssl_renegotiation
! port-count (ssl_renegotiation_limit - 1) * 1024L)
{
in_ssl_renegotiation = true;
+ /*
+ * The way we determine that a renegotiation has completed is by
+ * observing OpenSSL's internal renegotiation counter. Make sure
+ * we start out at zero, and assume that the renegotiation is
+ * complete when the counter advances.
+ *
+ * OpenSSL provides SSL_renegotiation_pending(), but this doesn't
+ * seem to work in testing.
+ */
+ SSL_clear_num_renegotiations(port-ssl);
+
SSL_set_session_id_context(port-ssl, (void *) SSL_context,
sizeof(SSL_context));
if (SSL_renegotiate(port-ssl) = 0)
***
*** 347,379 secure_write(Port *port, void *ptr, size_t len)
errmsg(SSL failure during renegotiation start)));
else
{
! int handshake;
! int retries = 20;
/*
! * The way we determine that a renegotiation has completed is
! * by observing OpenSSL's internal renegotiation counter.
! * Memoize it when starting the renegotiation, and assume that
! * the renegotiation is complete when the counter advances.
! *
! * OpenSSL provides SSL_renegotiation_pending(), but this
! * doesn't seem to work in testing.
*/
! renegotiation_count = SSL_num_renegotiations(port-ssl);
!
! /*
! * A handshake can fail, so be prepared to retry it, but only a
! * few times
! */
! for (;;)
{
! handshake = SSL_do_handshake(port-ssl);
! if (handshake 0)
break; /* done */
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg(SSL handshake failure on renegotiation, retrying)));
! if (retries-- = 0)
ereport(FATAL,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg(unable to complete SSL handshake)));
--- 355,374
errmsg(SSL failure during renegotiation start)));
else
{
! int retries;
/*
! * A handshake can fail, so be prepared to retry it, but only
! * a few times.
*/
! for (retries = 0; retries++;)
{
! if (SSL_do_handshake(port-ssl) 0)
break; /* done */
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg(SSL handshake failure on renegotiation, retrying)));
! if (retries = 20)
ereport(FATAL,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg(unable to complete SSL handshake)));
***
*** 427,439 wloop:
/* is renegotiation complete? */
if (in_ssl_renegotiation
! SSL_num_renegotiations(port-ssl) renegotiation_count)
{
in_ssl_renegotiation = false;
port-count = 0;
- /* avoid overflow issues by resetting counter */
- SSL_clear_num_renegotiations(port-ssl);
- renegotiation_count = SSL_num_renegotiations(port-ssl);
}
/*
--- 422,431
/* is renegotiation complete? */
if (in_ssl_renegotiation
! SSL_num_renegotiations(port-ssl) = 1)
{
in_ssl_renegotiation = false;
port-count = 0;
}
/*
*** a/src/backend/libpq/be-secure.c
---
Re: [HACKERS] SSL renegotiation
Here's the patch I propose to handle renegotiation cleanly. I noticed
in testing that SSL_renegotiate_pending() doesn't seem to actually work
--- if I throw an ereport(FATAL) at the point where I expect the
renegotiation to be complete, it always dies; even if I give it
megabytes of extra traffic while waiting for the renegotiation to
complete. I suspect this is an OpenSSL bug. Instead, in this patch I
check the internal renegotiation counter: grab its current value when
starting the renegotiation, and consider it complete when the counter
has advanced. This works fine.
Another thing not covered by the original code snippet I proposed
upthread is to avoid renegotiating when there was a renegotiation in
progress. This bug has been observed in the field.
Per discussion, I made it close the connection with a FATAL error if the
limit is reached and the renegotiation hasn't taken place. To do
otherwise is not acceptable for a security PoV.
Sean Chittenden mentioned that when retrying the handshake, we should be
careful to only do it a few times, not forever, to avoid a malfeasant
from grabbing hold of a connection indefinitely. I've added that too,
hardcoding the number of retries to 20.
Also, I made this code request a renegotiation slightly before the limit
is actually reached. I noticed that in some cases some traffic can go
by before the renegotiation is actually completed. The difference
should be pretty minimal.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
*** a/src/backend/libpq/be-secure.c
--- b/src/backend/libpq/be-secure.c
***
*** 101,106 char *ssl_crl_file;
--- 101,109
*/
int ssl_renegotiation_limit;
+ /* are we in the middle of a renegotiation? */
+ static bool in_ssl_renegotiation = false;
+
#ifdef USE_SSL
static SSL_CTX *SSL_context = NULL;
static bool ssl_loaded_verify_locations = false;
***
*** 321,350 secure_write(Port *port, void *ptr, size_t len)
if (port-ssl)
{
int err;
! if (ssl_renegotiation_limit port-count ssl_renegotiation_limit * 1024L)
{
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)));
! if (SSL_do_handshake(port-ssl) = 0)
! ereport(COMMERROR,
! (errcode(ERRCODE_PROTOCOL_VIOLATION),
! errmsg(SSL renegotiation failure)));
! if (port-ssl-state != SSL_ST_OK)
! ereport(COMMERROR,
! (errcode(ERRCODE_PROTOCOL_VIOLATION),
! errmsg(SSL failed to send renegotiation request)));
! port-ssl-state |= SSL_ST_ACCEPT;
! SSL_do_handshake(port-ssl);
! if (port-ssl-state != SSL_ST_OK)
! ereport(COMMERROR,
! (errcode(ERRCODE_PROTOCOL_VIOLATION),
! errmsg(SSL renegotiation failure)));
! port-count = 0;
}
wloop:
--- 324,384
if (port-ssl)
{
int err;
+ static long renegotiation_count = 0;
! /*
! * If SSL renegotiations are enabled and we're getting close to the
! * limit, start one now; but avoid it if there's one already in
! * progress. Request the renegotiation before the limit has actually
! * expired; we give it 1% of the specified limit, or 1kB, whichever is
! * greater.
! */
! if (ssl_renegotiation_limit !in_ssl_renegotiation
! port-count Min((ssl_renegotiation_limit - 1) * 1024L,
! (long) rint(ssl_renegotiation_limit * 1024L * 0.99)))
{
+ in_ssl_renegotiation = true;
+
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 failure during renegotiation start)));
! else
! {
! int handshake;
! int retries = 20;
!
! /*
! * The way we determine that a renegotiation has completed is
! * by observing OpenSSL's internal renegotiation counter.
! * Memoize it when starting the renegotiation, and assume that
! * the renegotiation is complete when the counter advances.
! *
! * OpenSSL provides SSL_renegotiation_pending(), but this
! * doesn't seem to work in testing.
! */
! renegotiation_count = SSL_num_renegotiations(port-ssl);
!
! /*
! * A handshake can fail, so be prepared to retry it, but only a
! * few times
! */
! for (;;)
! {
! handshake = SSL_do_handshake(port-ssl);
! if (handshake 0)
! break; /* done */
! ereport(COMMERROR,
! (errcode(ERRCODE_PROTOCOL_VIOLATION),
! errmsg(SSL handshake failure on renegotiation, retrying)));
! if (retries-- = 0)
! ereport(FATAL,
! (errcode(ERRCODE_PROTOCOL_VIOLATION),
! errmsg(unable to complete SSL handshake)));
! }
! }
Re: [HACKERS] SSL renegotiation
On Fri, Jul 12, 2013 at 8:51 PM, Noah Misch n...@leadboat.com wrote: On Fri, Jul 12, 2013 at 04:32:52PM -0400, Alvaro Herrera wrote: Now, should we support the 0.9.6-and-earlier mechanism? My inclination is no; even RHEL 3, the oldest supported Linux distribution, uses 0.9.7 (Heck, even Red Hat Linux 9, released on 2003). To see OpenSSL 0.9.6 you need to go back to Red Hat Linux 7.2, released on 2001 using a Linux kernel 2.4. Surely no one in their right mind would use a current Postgres release on such an ancient animal. Agreed. The OpenSSL Project last applied a security fix to 0.9.6 over eight years ago. Compatibility with 0.9.6 has zero or negative value. +1 from me as well, if any more are needed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Fri, Jul 12, 2013 at 08:51:52PM -0400, Noah Misch wrote: On Fri, Jul 12, 2013 at 04:32:52PM -0400, Alvaro Herrera wrote: Now, should we support the 0.9.6-and-earlier mechanism? My inclination is no; even RHEL 3, the oldest supported Linux distribution, uses 0.9.7 (Heck, even Red Hat Linux 9, released on 2003). To see OpenSSL 0.9.6 you need to go back to Red Hat Linux 7.2, released on 2001 using a Linux kernel 2.4. Surely no one in their right mind would use a current Postgres release on such an ancient animal. Agreed. The OpenSSL Project last applied a security fix to 0.9.6 over eight years ago. Compatibility with 0.9.6 has zero or negative value. You've made a persuasive case that we should actively break backward compatibility here. Would that be complicated to do? Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Tue, Jul 16, 2013 at 10:41:44AM -0700, David Fetter wrote: On Fri, Jul 12, 2013 at 08:51:52PM -0400, Noah Misch wrote: Agreed. The OpenSSL Project last applied a security fix to 0.9.6 over eight years ago. Compatibility with 0.9.6 has zero or negative value. You've made a persuasive case that we should actively break backward compatibility here. Would that be complicated to do? Nope. If Alvaro's code change builds under 0.9.6, malfunctioning only at runtime, I suspect we would add a configure-time version check and possibly a runtime one as well. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Troels Nielsen escribió: Hi, These are the relevant bits from Apache2.4's mod_ssl. [snip] So this is basically the same thing the Pg code is doing. That code supports at least OpenSSL 0.9.7 and later. Some explanation for it can be found here: http://books.google.dk/books?id=IIqwAy4qEl0Cpg=PT184lpg=PT184dq=SSL_do_handshakesource=blots=ma632U4vWvsig=d2qqS0svhu4EwIZZaONdHoTulVIhl=ensa=Xei=xdPdUczoDuf34QSzmoDQDgved=0CIIDEOgBMCo The snippet there is probably the textbook implementation. Ah, thanks for the pointer. I notice this book is from 2002 and documents OpenSSL 0.9.6. So yeah, that's what both mod_ssl and Pg implement. However, reading the text carefully, I see that this snippet is the correct implementation for 0.9.6 and earlier; the same book, speaking about the 0.9.7 release in the future tense, explains that in that release it will be much easier to do renegotiations, without getting into precise details of how exactly is to be done (I suppose the OpenSSL team hadn't finalized the API yet). As far as I can understand, what I propose is the right sequence. Now, should we support the 0.9.6-and-earlier mechanism? My inclination is no; even RHEL 3, the oldest supported Linux distribution, uses 0.9.7 (Heck, even Red Hat Linux 9, released on 2003). To see OpenSSL 0.9.6 you need to go back to Red Hat Linux 7.2, released on 2001 using a Linux kernel 2.4. Surely no one in their right mind would use a current Postgres release on such an ancient animal. So I continue to maintain that we should rip the old mechanism out and replace it with current renegotiation. Now, I've read a bit more of the code and it seems that we should be doing SSL_renegotiate() and then check the SSL_renegotiate_pending() result until it returns that the renegotiation has completed. So the original code looks OK. Perhaps the check on the return code of the first SSL_do_handshake (and SSL_renegotiate) is unnecessary and may lead to unwarranted error messages, though. And it may be wrong to continue the renegotiation if the state is not SSL_ST_OK after the first SSL_do_handshake. If the renegotiation fails, I suppose two things can be done: 1. Quit the connection 2. Carry on pretending nothing happened. I think 2. is correct in the vast majority of cases (as it looks like is being done now). 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. Good point. From a security point of view, it seems that the correct reaction is to close the connection if renegotiation doesn't complete. Along the same lines, it seems to me that accepting SSLv2 (which doesn't support renegotiations at all) when renegotiations have been requested is not a good choice; we should accept only SSLv3 in that case. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Fri, Jul 12, 2013 at 04:32:52PM -0400, Alvaro Herrera wrote: Now, should we support the 0.9.6-and-earlier mechanism? My inclination is no; even RHEL 3, the oldest supported Linux distribution, uses 0.9.7 (Heck, even Red Hat Linux 9, released on 2003). To see OpenSSL 0.9.6 you need to go back to Red Hat Linux 7.2, released on 2001 using a Linux kernel 2.4. Surely no one in their right mind would use a current Postgres release on such an ancient animal. Agreed. The OpenSSL Project last applied a security fix to 0.9.6 over eight years ago. Compatibility with 0.9.6 has zero or negative value. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Thu, Jul 11, 2013 at 4:20 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I'm having a look at the SSL support code, because one of our customers reported it behaves unstably when the network is unreliable. I have yet to reproduce the exact problem they're having, but while reading the code I notice this in be-secure.c:secure_write() : The recap of my experiences you requested... I first saw SSL renegotiation failures on Ubuntu 10.04 LTS (Lucid) with openssl 0.9.8 (something). I think this was because SSL renegotiation had been disabled due to due to CVE 2009-3555 (affecting all versions before 0.9.8l). I think the version now in lucid is 0.9.8k with fixes for SSL renegotiation, but I haven't tested this. The failures I saw with no-renegotiation-SSL for streaming replication looked like this: On the master: 2012-06-25 16:16:26 PDT LOG: SSL renegotiation failure 2012-06-25 16:16:26 PDT LOG: SSL error: unexpected record 2012-06-25 16:16:26 PDT LOG: could not send data to client: Connection reset by peer On the hot standby: 2012-06-25 11:12:11 PDT FATAL: could not receive data from WAL stream: SSL error: sslv3 alert unexpected message 2012-06-25 11:12:11 PDT LOG: record with zero length at 1C5/95D2FE00 Now I'm running Ubuntu 12.04 LTS (Precise) with openssl 1.0.1, and I think all the known renegotiation issues have been dealt with. I still get failures, but they are less informative: postgres@[unknown]:19761 2013-03-15 03:55:12 UTC LOG: SSL renegotiation failure -- Stuart Bishop stu...@stuartbishop.net http://www.stuartbishop.net/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
On Thu, Jul 11, 2013 at 1:13 AM, Sean Chittenden s...@chittenden.org wrote: , 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, allowing the client to refuse to renegotiate leaves the relevant vulnerability unpatched. Renegotiation was introduced to patch a vulnerability in which, without renegotiation, there was the possibility of an attacker gaining knowledge of session keys (and hence the ability to intercept the stream). I think 2 is not viable in this context. Only 1. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
I think this block is better written as:
if (ssl_renegotiation_limit port-count ssl_renegotiation_limit *
1024L)
{
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
{
inthandshake;
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);
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.
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.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL renegotiation
Hi,
These are the relevant bits from Apache2.4's mod_ssl.
SSL_renegotiate(ssl);
SSL_do_handshake(ssl);
if (SSL_get_state(ssl) != SSL_ST_OK) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02225)
Re-negotiation request failed);
ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, r-server);
r-connection-keepalive = AP_CONN_CLOSE;
return HTTP_FORBIDDEN;
}
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02226)
Awaiting re-negotiation handshake);
/* XXX: Should replace setting state with SSL_renegotiate(ssl);
* However, this causes failures in perl-framework currently,
* perhaps pre-test if we have already negotiated?
*/
#ifdef OPENSSL_NO_SSL_INTERN
SSL_set_state(ssl, SSL_ST_ACCEPT);
#else
ssl-state = SSL_ST_ACCEPT;
#endif
SSL_do_handshake(ssl);
sslconn-reneg_state = RENEG_REJECT;
if (SSL_get_state(ssl) != SSL_ST_OK) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02261)
Re-negotiation handshake failed:
Not accepted by client!?);
r-connection-keepalive = AP_CONN_CLOSE;
return HTTP_FORBIDDEN;
}
That code supports at least OpenSSL 0.9.7 and later.
Some explanation for it can be found here:
http://books.google.dk/books?id=IIqwAy4qEl0Cpg=PT184lpg=PT184dq=SSL_do_handshakesource=blots=ma632U4vWvsig=d2qqS0svhu4EwIZZaONdHoTulVIhl=ensa=Xei=xdPdUczoDuf34QSzmoDQDgved=0CIIDEOgBMCo
The snippet there is probably the textbook implementation.
So the original code looks OK. Perhaps the check
on the return code of the first SSL_do_handshake (and SSL_renegotiate)
is unnecessary and may lead to unwarranted error messages, though.
And it may be wrong to continue the renegotiation if
the state is not SSL_ST_OK after the first SSL_do_handshake.
If the renegotiation fails, I suppose two things can be done:
1. Quit the connection
2. Carry on pretending nothing happened.
I think 2. is correct in the vast majority of cases (as it looks like is
being done now).
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.
Kind Regards
Troels Nielsen
On Thu, Jul 11, 2013 at 12:34 AM, Alvaro Herrera
alvhe...@2ndquadrant.comwrote:
I think this block is better written as:
if (ssl_renegotiation_limit port-count
ssl_renegotiation_limit * 1024L)
{
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
{
inthandshake;
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);
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.
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.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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
{
inthandshake;
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
