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 = SSL_write(conn->ssl, ptr, len);
+ pqsecure_block_raw_read = false;
err = SSL_get_error(conn->ssl, n);
switch (err)
{
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 57c572b..11d0f8b 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -124,6 +124,8 @@ struct sigpipe_info
#define RESTORE_SIGPIPE(conn, spinfo)
#endif /* WIN32 */
+bool pqsecure_block_raw_read = false;
+
/* ------------------------------------------------------------ */
/* Procedures common to all secure sessions */
/* ------------------------------------------------------------ */
@@ -227,6 +229,13 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
int result_errno = 0;
char sebuf[256];
+ /* If the kill-switch is set, pretend that there is no data. */
+ if (pqsecure_block_raw_read)
+ {
+ errno = EWOULDBLOCK;
+ return -1;
+ }
+
n = recv(conn->sock, ptr, len, 0);
if (n < 0)
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 008fd67..1a94d16 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -618,6 +618,8 @@ extern int pqWriteReady(PGconn *conn);
/* === in fe-secure.c === */
+extern bool pqsecure_block_raw_read;
+
extern int pqsecure_initialize(PGconn *);
extern void pqsecure_destroy(void);
extern PostgresPollingStatusType pqsecure_open_client(PGconn *);
--
2.1.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
