On 2015-07-28 18:59:02 +0200, Andres Freund wrote:
> Attached are:
>
> a) a slightly evolved version of Michael's patch disabling renegotiation
> by default that I'm planning to apply to 9.4 - 9.0
>
> b) a patch removing renegotiation entirely from master and 9.5
>
> Unless somebody protests soon I'm going to push something like that
> after having dinner.
>
> I am wondering whether b) ought to remove Port->count, but I'm currently
> leaning to leaving it in place for now; perhaps adding a comment in the
> struct. I'm actually thinking we very well might want to add something
> like it to all backends, but more importantly it'd make the diff larger
> with mostly unrelated changes.
And really attached.
>From 768dd6560e262d7597d0efb37043a96e1594508e Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Tue, 28 Jul 2015 18:41:32 +0200
Subject: [PATCH] Disable ssl renegotiation by default.
While postgres' use of SSL renegotiation is a good idea in theory, it
turned out to not work well in practice. The specification and openssl's
implementation of it have lead to several security issues. Postgres' use
of renegotiation also had its share of bugs.
Additionally OpenSSL has a bunch of bugs around renegotiation, reported
and open for years, that regularly lead to connections breaking with
obscure error messages. We tried increasingly complex workarounds around
these bugs, but we didn't find anything complete.
Since these connection breakages often lead to hard to debug problems,
e.g. spuriously failing base backups and significant latency spikes when
synchronous replication is used, we have decided to change the default
setting for ssl renegotiation to 0 (disabled) in the released
backbranches and remove it entirely in 9.5 and master..
Author: Michael Paquier, with changes by me
Discussion: 20150624144148.gq4...@alap3.anarazel.de
Backpatch: 9.0-9.4; 9.5 and master get a different patch
---
doc/src/sgml/config.sgml | 10 +++++++++-
src/backend/utils/misc/guc.c | 2 +-
src/backend/utils/misc/postgresql.conf.sample | 2 +-
3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index c669f75..871b04a 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1040,7 +1040,7 @@ include_dir 'conf.d'
cryptanalysis when large amounts of traffic can be examined, but it
also carries a large performance penalty. The sum of sent and received
traffic is used to check the limit. If this parameter is set to 0,
- renegotiation is disabled. The default is <literal>512MB</>.
+ renegotiation is disabled. The default is <literal>0</>.
</para>
<note>
<para>
@@ -1052,6 +1052,14 @@ include_dir 'conf.d'
disabled.
</para>
</note>
+
+ <warning>
+ <para>
+ Due to bugs in <productname>OpenSSL</> enabling ssl renegotiation, by
+ configuring a non-zero <varname>ssl_renegotiation_limit</>, is likely
+ to lead to problems like long-lived connections breaking.
+ </para>
+ </warning>
</listitem>
</varlistentry>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6ad0892..396c68b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2457,7 +2457,7 @@ static struct config_int ConfigureNamesInt[] =
GUC_UNIT_KB,
},
&ssl_renegotiation_limit,
- 512 * 1024, 0, MAX_KILOBYTES,
+ 0, 0, MAX_KILOBYTES,
NULL, NULL, NULL
},
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 8dfd485..3845d57 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -83,7 +83,7 @@
# (change requires restart)
#ssl_prefer_server_ciphers = on # (change requires restart)
#ssl_ecdh_curve = 'prime256v1' # (change requires restart)
-#ssl_renegotiation_limit = 512MB # amount of data between renegotiations
+#ssl_renegotiation_limit = 0 # amount of data between renegotiations
#ssl_cert_file = 'server.crt' # (change requires restart)
#ssl_key_file = 'server.key' # (change requires restart)
#ssl_ca_file = '' # (change requires restart)
--
2.4.0.rc2.1.g3d6bc9a
>From 0d488c5f1d7eba48aa19894339c69488094878a1 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Tue, 28 Jul 2015 17:59:55 +0200
Subject: [PATCH] Remove ssl renegotiation support.
While postgres' use of SSL renegotiation is a good idea in theory, it
turned out to not work well in practice. The specification and openssl's
implementation of it have lead to several security issues. Postgres' use
of renegotiation also had its share of bugs.
Additionally OpenSSL has a bunch of bugs around renegotiation, reported
and open for years, that regularly lead to connections breaking with
obscure error messages. We tried increasingly complex workarounds around
these bugs, but we didn't find anything complete.
Since these connection breakages often lead to hard to debug problems,
e.g. spuriously failing base backups and significant latency spikes when
synchronous replication is used, we have decided to change the default
setting for ssl renegotiation to 0 (disabled) in the released
backbranches and remove it entirely in 9.5 and master.
Author: Andres Freund
Discussion: 20150624144148.gq4...@alap3.anarazel.de
Backpatch: 9.5 and master, 9.0-9.4 get a different patch
---
doc/src/sgml/config.sgml | 29 -----------
src/backend/libpq/be-secure-openssl.c | 70 +--------------------------
src/backend/libpq/be-secure.c | 7 ---
src/backend/utils/misc/guc.c | 11 -----
src/backend/utils/misc/postgresql.conf.sample | 1 -
src/include/libpq/libpq-be.h | 5 --
6 files changed, 2 insertions(+), 121 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index bbe1eb0..e900dccb 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1034,35 +1034,6 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
- <varlistentry id="guc-ssl-renegotiation-limit" xreflabel="ssl_renegotiation_limit">
- <term><varname>ssl_renegotiation_limit</varname> (<type>integer</type>)
- <indexterm>
- <primary><varname>ssl_renegotiation_limit</> configuration parameter</primary>
- </indexterm>
- </term>
- <listitem>
- <para>
- Specifies how much data can flow over an <acronym>SSL</>-encrypted
- connection before renegotiation of the session keys will take
- place. Renegotiation decreases an attacker's chances of doing
- cryptanalysis when large amounts of traffic can be examined, but it
- also carries a large performance penalty. The sum of sent and received
- traffic is used to check the limit. If this parameter is set to 0,
- renegotiation is disabled. The default is <literal>512MB</>.
- </para>
- <note>
- <para>
- SSL libraries from before November 2009 are insecure when using SSL
- renegotiation, due to a vulnerability in the SSL protocol. As a
- stop-gap fix for this vulnerability, some vendors shipped SSL
- libraries incapable of doing renegotiation. If any such libraries
- are in use on the client or server, SSL renegotiation should be
- disabled.
- </para>
- </note>
- </listitem>
- </varlistentry>
-
<varlistentry id="guc-ssl-ciphers" xreflabel="ssl_ciphers">
<term><varname>ssl_ciphers</varname> (<type>string</type>)
<indexterm>
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index f0774fe..e9bc282 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -16,12 +16,8 @@
* backend can restart automatically, it is important that
* we select an algorithm that continues to provide confidentiality
* even if the attacker has the server's private key. Ephemeral
- * DH (EDH) keys provide this, and in fact provide Perfect Forward
- * Secrecy (PFS) except for situations where the session can
- * be hijacked during a periodic handshake/renegotiation.
- * Even that backdoor can be closed if client certificates
- * are used (since the imposter will be unable to successfully
- * complete renegotiation).
+ * DH (EDH) keys provide this and more (Perfect Forward Secrecy
+ * aka PFS).
*
* N.B., the static private key should still be protected to
* the largest extent possible, to minimize the risk of
@@ -37,12 +33,6 @@
* session. In this case you'll need to temporarily disable
* EDH by commenting out the callback.
*
- * ...
- *
- * Because the risk of cryptanalysis increases as large
- * amounts of data are sent with the same session key, the
- * session keys are periodically renegotiated.
- *
*-------------------------------------------------------------------------
*/
@@ -92,9 +82,6 @@ static const char *SSLerrmessage(void);
static char *X509_NAME_to_cstring(X509_NAME *name);
-/* are we in the middle of a renegotiation? */
-static bool in_ssl_renegotiation = false;
-
static SSL_CTX *SSL_context = NULL;
/* ------------------------------------------------------------ */
@@ -570,37 +557,6 @@ be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
ssize_t n;
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);
-
- /* without this, renegotiation fails when a client cert is used */
- 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")));
- }
-
errno = 0;
n = SSL_write(port->ssl, ptr, len);
err = SSL_get_error(port->ssl, n);
@@ -646,28 +602,6 @@ be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
break;
}
- if (n >= 0)
- {
- /* 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")));
- }
-
return n;
}
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 4a650cc..26d8faa 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -43,13 +43,6 @@ char *ssl_key_file;
char *ssl_ca_file;
char *ssl_crl_file;
-/*
- * How much data can be sent across a secure connection
- * (total in both directions) before we require renegotiation.
- * Set to 0 to disable renegotiation completely.
- */
-int ssl_renegotiation_limit;
-
#ifdef USE_SSL
bool ssl_loaded_verify_locations = false;
#endif
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1bed525..1b7b914 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2578,17 +2578,6 @@ static struct config_int ConfigureNamesInt[] =
},
{
- {"ssl_renegotiation_limit", PGC_USERSET, CONN_AUTH_SECURITY,
- gettext_noop("Set the amount of traffic to send and receive before renegotiating the encryption keys."),
- NULL,
- GUC_UNIT_KB,
- },
- &ssl_renegotiation_limit,
- 512 * 1024, 0, MAX_KILOBYTES,
- NULL, NULL, NULL
- },
-
- {
{"tcp_keepalives_count", PGC_USERSET, CLIENT_CONN_OTHER,
gettext_noop("Maximum number of TCP keepalive retransmits."),
gettext_noop("This controls the number of consecutive keepalive retransmits that can be "
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 06dfc06..e5d275d 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -83,7 +83,6 @@
# (change requires restart)
#ssl_prefer_server_ciphers = on # (change requires restart)
#ssl_ecdh_curve = 'prime256v1' # (change requires restart)
-#ssl_renegotiation_limit = 512MB # amount of data between renegotiations
#ssl_cert_file = 'server.crt' # (change requires restart)
#ssl_key_file = 'server.key' # (change requires restart)
#ssl_ca_file = '' # (change requires restart)
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 6171ef3..caaa8b5 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -93,11 +93,6 @@ typedef struct
#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
--
2.4.0.rc2.1.g3d6bc9a
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
