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 Herrera http://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")));
! }
! }
}
wloop:
***************
*** 390,395 **** wloop:
--- 424,451 ----
n = -1;
break;
}
+
+ /* 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);
+ }
+
+ /*
+ * 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
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 125,131 **** 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
--- 125,130 ----
*** a/src/include/libpq/libpq-be.h
--- b/src/include/libpq/libpq-be.h
***************
*** 93,98 **** typedef struct
--- 93,103 ----
#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
* backend is run. The Port structure is kept in malloc'd memory and is
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
