Rework SSL renegotiation code

The existing renegotiation code was home for several bugs: it might
erroneously report that renegotiation had failed; it might try to
execute another renegotiation while the previous one was pending; it
failed to terminate the connection if the renegotiation never actually
took place; if a renegotiation was started, the byte count was reset,
even if the renegotiation wasn't completed (this isn't good from a
security perspective because it means continuing to use a session that
should be considered compromised due to volume of data transferred.)

The new code is structured to avoid these pitfalls: renegotiation is
started a little earlier than the limit has expired; the handshake
sequence is retried until it has actually returned successfully, and no
more than that, but if it fails too many times, the connection is
closed.  The byte count is reset only when the renegotiation has
succeeded, and if the renegotiation byte count limit expires, the
connection is terminated.

This commit only touches the master branch, because some of the changes
are controversial.  If everything goes well, a back-patch might be
considered.

Per discussion started by message
20130710212017.GB4941@eldon.alvh.no-ip.org

diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index cd088d2e0afb9c37e306ac6abed39de5e38055f9..c451420990eeb87613d9a33c3dab5b1e0f5c809a 100644 (file)
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -101,6 +101,9 @@ char           *ssl_crl_file;
  */
 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;
@@ -322,29 +325,55 @@ secure_write(Port *port, void *ptr, size_t len)
        {
                int                     err;
 
-               if (ssl_renegotiation_limit && port->count > ssl_renegotiation_limit * 1024L)
+               /*
+                * 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)
                                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;
+                                                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")));
+                               }
+                       }
                }
 
 wloop:
@@ -390,6 +419,25 @@ wloop:
                                n = -1;
                                break;
                }
+
+               /* is renegotiation complete? */
+               if (in_ssl_renegotiation &&
+                       SSL_num_renegotiations(port->ssl) >= 1)
+               {
+                       in_ssl_renegotiation = false;
+                       port->count = 0;
+               }
+
+               /*
+                * if renegotiation is still ongoing, and we've gone beyond the limit,
+                * kill the connection now -- continuing to use it can be considered a
+                * security problem.
+                */
+               if (in_ssl_renegotiation &&
+                       port->count > ssl_renegotiation_limit * 1024L)
+                       ereport(FATAL,
+                                       (errcode(ERRCODE_PROTOCOL_VIOLATION),
+                                        errmsg("SSL failed to renegotiate connection before limit expired")));
        }
        else
 #endif

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1756b48c4fe3566970cdc670a362e5ff726abb5b..8db8b3f6ecf087d4feb5dfb5466f491f6df192a3 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -126,7 +126,6 @@ extern char *default_tablespace;
 extern char *temp_tablespaces;
 extern bool ignore_checksum_failure;
 extern bool synchronize_seqscans;
-extern int     ssl_renegotiation_limit;
 extern char *SSLCipherSuites;
 
 #ifdef TRACE_SORT

diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index ef8e25570a650ad453be0a2c320bb315ae25569b..b3abab15611f78f1a955474415c6a9c21795bbce 100644 (file)
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -92,6 +92,11 @@ typedef struct
 } pg_gssinfo;
 #endif
 
+/*
+ * SSL renegotiations
+ */
+extern int     ssl_renegotiation_limit;
+
 /*
  * This is used by the postmaster in its communication with frontends. It
  * contains all state information needed during this communication before the