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.

Reviewed by Andres Freund

diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 2752d1640a50946ca703055062a56e474b14014e..7455978f88a5e73233222a391103449dbeaababe 100644 (file)
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -90,6 +90,14 @@ static void close_SSL(PGconn *);
 static char *SSLerrmessage(void);
 static void SSLerrfree(char *buf);
 
+static int my_sock_read(BIO *h, char *buf, int size);
+static BIO_METHOD *my_BIO_s_socket(void);
+static int my_SSL_set_fd(PGconn *conn, int fd);
+
+static bool my_block_raw_read = false;
+
+static int (*orig_sock_read) (BIO *h, char *buf, int size);
+
 static bool pq_init_ssl_lib = true;
 static bool pq_init_crypto_lib = true;
 
@@ -274,7 +282,7 @@ pqsecure_open_client(PGconn *conn)
                /* Create a connection-specific SSL object */
                if (!(conn->ssl = SSL_new(SSL_context)) ||
                        !SSL_set_app_data(conn->ssl, conn) ||
-                       !SSL_set_fd(conn->ssl, conn->sock))
+                       !my_SSL_set_fd(conn, conn->sock))
                {
                        char       *err = SSLerrmessage();
 
@@ -509,9 +517,28 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
                int                     err;
 
                DISABLE_SIGPIPE(conn, spinfo, return -1);
-
+               /*
+                * To work-around an issue with OpenSSL and renegotiation, don't let
+                * SSL_write() 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 by setting a kill-switch, my_block_raw_read, which tells
+                * my_sock_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);
+               my_block_raw_read = true;
                n = SSL_write(conn->ssl, ptr, len);
+               my_block_raw_read = false;
                err = SSL_get_error(conn->ssl, n);
                switch (err)
                {
@@ -1641,6 +1668,70 @@ SSLerrfree(char *buf)
                free(buf);
 }
 
+/*
+ * Private substitute BIO: this allows us to intercept reads, so that
+ * we can prevent SSL_write() from seeing data.
+ *
+ * These functions are closely modelled on the standard socket BIO in OpenSSL;
+ * see sock_read() and sock_write() in OpenSSL's crypto/bio/bss_sock.c.
+ * XXX OpenSSL 1.0.1e considers many more errcodes than just EINTR as reasons
+ * to retry; do we need to adopt their logic for that?
+ */
+
+static bool my_bio_initialized = false;
+static BIO_METHOD my_bio_methods;
+
+static int
+my_sock_read(BIO *h, char *buf, int size)
+{
+       /* If the kill-switch is set, pretend that there is no data. */
+       if (my_block_raw_read)
+       {
+               BIO_clear_retry_flags(h);
+               BIO_set_retry_read(h);
+               errno = EWOULDBLOCK;
+               return -1;
+       }
+
+       return orig_sock_read(h, buf, size);
+}
+
+static BIO_METHOD *
+my_BIO_s_socket(void)
+{
+       if (!my_bio_initialized)
+       {
+               memcpy(&my_bio_methods, BIO_s_socket(), sizeof(BIO_METHOD));
+               orig_sock_read = my_bio_methods.bread;
+               my_bio_methods.bread = my_sock_read;
+               my_bio_initialized = true;
+       }
+       return &my_bio_methods;
+}
+
+/* This should exactly match openssl's SSL_set_fd except for using my BIO */
+static int
+my_SSL_set_fd(PGconn *conn, int fd)
+{
+       int                     ret = 0;
+       BIO                *bio = NULL;
+
+       bio = BIO_new(my_BIO_s_socket());
+       if (bio == NULL)
+       {
+               SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB);
+               goto err;
+       }
+       /* Use 'ptr' to store pointer to PGconn */
+       bio->ptr = conn;
+
+       SSL_set_bio(conn->ssl, bio, bio);
+       BIO_set_fd(bio, fd, BIO_NOCLOSE);
+       ret = 1;
+err:
+       return ret;
+}
+
 /*
  *     Return pointer to OpenSSL object.
  */