PostgreSQL - Weak DH group
Hello everyone,
I sent few days ago to the security DL a mail reporting a vulnerability in
how Postgres is requesting DH params to be used later for encryption
algorithms. So, due to there is no problem sharing with this group, here is
what I sent:
------------------------------------------------------------------------------------------------------------------------------------------
Hi folks,
We are working with Postgres 9.3.14 and executing nmap we
found that it is using “weak DH group” (nmap –script ssl-dh-params). Weak =
1024 bits.
See nmap output (1)
We don’t know if other versions are affected or not. The
environment used is a RHEL 6 x86_6, OpenSSL version 1.0.2i with FIPS module.
This issue is similar to what this post explains about using weak DH
parameters: http://www.usefuljs.net/2016/09/29/imperfect-forward-secrecy/
Following with the code, it seems that PostgreSQL has
missed the keyLength OpenSSL parameter, and it delivers into a weak crypto
configuration.. Affected Code:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=
blob;f=src/backend/libpq/be-secure-openssl.c;h=
8d8f12952a4a4f14a15f8647b96935e13d68fb39;hb=48d50840d53eb62842c0d9b54eab9c
d7c9a3a46d
(Thanks to Damian in order to found the affected code)
(1) nmap output:
# nmap –script ssl-dh-params -p 5432 <ip>
Starting Nmap 7.25BETA2 ( https://nmap.org )
Nmap scan report for <ip>
Host is up (0.00035s latency).
PORT STATE SERVICE
5432/tcp open postgresql
| ssl-dh-params:
| VULNERABLE:
| Diffie-Hellman Key Exchange Insufficient Group Strength
| State: VULNERABLE
| Transport Layer Security (TLS) services that use Diffie-Hellman
groups
| of insufficient strength, especially those using one of a few
commonly
| shared groups, may be susceptible to passive eavesdropping attacks.
| Check results:
| WEAK DH GROUP 1
| Cipher Suite: TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
| Modulus Type: Safe prime
| Modulus Source: Unknown/Custom-generated
| Modulus Length: 1024
| Generator Length: 8
| Public Key Length: 1024
| References:
------------------------------------------------------------------------------------------------------------------------------------------
Thanks in advance
Nicolas Guini
On 10/05/2016 05:15 PM, Nicolas Guini wrote:
We are working with Postgres 9.3.14 and executing nmap we
found that it is using “weak DH group” (nmap –script ssl-dh-params). Weak =
1024 bits.
Yeah, it seems that we're a bit behind the times on this...
This issue is similar to what this post explains about using weak DH
parameters: http://www.usefuljs.net/2016/09/29/imperfect-forward-secrecy/
The blog post points out that, as counterintuitive as it sounds, the
SSL_CTX_set_tmp_dh_callback() callback should ignore the keylength
argument, and always return a DH group with 2048 bits, or stronger. As
you pointed out, that's not what our callback does.
We should fix this in master, at least. I'm not sure about backporting,
there might be compatibility issues. It seems that at least OpenJDK
(Java) didn't support DH groups larger than 1024 bits, until version 8.
That's fairly recent, OpenJDK 8 was released in March 2014.
ECDHE family of ciphers are not affected, and are preferred over plain
DHE, I believe, so disabling DHE and removing the DH parameter loading
code altogether is one option. Clearly not backportable, though.
Meanwhile, users can work-around this by creating DH parameters with
something like "openssl dhparam -out $PGDATA/dh1024.pem 2048". Yes, the
file needs to be called "dh1024.pem", even though the actual key length
is 2048 bits.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/05/2016 09:57 PM, Heikki Linnakangas wrote:
On 10/05/2016 05:15 PM, Nicolas Guini wrote:
We are working with Postgres 9.3.14 and executing nmap we
found that it is using “weak DH group” (nmap –script ssl-dh-params). Weak =
1024 bits.Yeah, it seems that we're a bit behind the times on this...
This issue is similar to what this post explains about using weak DH
parameters: http://www.usefuljs.net/2016/09/29/imperfect-forward-secrecy/The blog post points out that, as counterintuitive as it sounds, the
SSL_CTX_set_tmp_dh_callback() callback should ignore the keylength
argument, and always return a DH group with 2048 bits, or stronger. As
you pointed out, that's not what our callback does.
I propose the attached patch. It gives up on trying to deal with
multiple key lengths (as noted earlier, OpenSSL just always passed
keylength=1024, so that was useless). Instead of using the callback, it
just sets fixed DH parameters with SSL_CTX_set_tmp_dh(), like we do for
the ECDH curve. The DH parameters are loaded from a file called
"dh_params.pem" (instead of "dh1024.pem"), if present, otherwise the
built-in 2048 bit parameters are used.
I removed the code for generating DH parameters on-the-fly altogether.
The OpenSSL man page clearly says that that should not be done, because
generating the parameters takes a long time. And because OpenSSL always
passed keylength=1024, AFAICS that had been always dead code.
If we want to get really fancy, we could generate the parameters the
first time you turn ssl=on, and store them in $PGDATA. But the
generation takes a very long time, so the admin might think it's stuck.
I note that supplying custom DH parameters in the file is completely
undocumented :-(. We should add a note in the docs on how to generate
the custom DH parameters with openssl. Also, there's no easy way of
telling if your custom parameters are actually been used. If you copy
the params file in $PGDATA, but you have a typo in the name, you won't
notice. Perhaps we should print a line in the log when the file is loaded.
I'm afraid of back-patching this, out of fear that older clients would
stop working, or would downgrade to an even weaker cipher. You could fix
it by putting weaker 1024 bit params in dh_params.pem, so maybe we could
do it if we put a warning and instructions for doing that in the release
notes. Thoughts?
- Heikki
Attachments:
0001-Always-use-2048-bit-DH-parameters-for-OpenSSL-epheme.patchtext/x-diff; name=0001-Always-use-2048-bit-DH-parameters-for-OpenSSL-epheme.patchDownload
From 69f74e3cced093c9ae2cce830e31c3fd76a8a06b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Thu, 6 Oct 2016 16:22:29 +0300
Subject: [PATCH 1/1] Always use 2048 bit DH parameters for OpenSSL ephemeral
DH ciphers.
1024 bits is considered weak these days, but OpenSSL always passes 1024 as
the key length to the tmp_dh callback. All the code to handle other key
lengths as in fact dead.
To remedy those issues:
* Only include hard-coded 2048-bit parameters.
* Set the parameters directly with SSL_CTX_set_tmp_dh(), without the
callback
* The file for the DH parameters is now called "dh_params.pem", instead of
"dh1024.pem" (the files for other key lengths, dh512.pem, dh2048.pem, etc.
were never actually used)
Per report by Nicolas Guini
Discussion: <CAMxBoUyjOOautVozN6ofzym828aNrDjuCcOTcCquxjwS-L2hGQ@mail.gmail.com>
---
src/backend/libpq/be-secure-openssl.c | 198 +++++++++-------------------------
1 file changed, 48 insertions(+), 150 deletions(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 668f217..fc7dd6a 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -65,18 +65,19 @@
#include "tcop/tcopprot.h"
#include "utils/memutils.h"
+/* name of the file containing DH parameters */
+#define DH_PARAMS_FILE "dh_params.pem"
static int my_sock_read(BIO *h, char *buf, int size);
static int my_sock_write(BIO *h, const char *buf, int size);
static BIO_METHOD *my_BIO_s_socket(void);
static int my_SSL_set_fd(Port *port, int fd);
-static DH *load_dh_file(int keylength);
+static DH *load_dh_file(void);
static DH *load_dh_buffer(const char *, size_t);
-static DH *generate_dh_parameters(int prime_len, int generator);
-static DH *tmp_dh_cb(SSL *s, int is_export, int keylength);
static int verify_cb(int, X509_STORE_CTX *);
static void info_cb(const SSL *ssl, int type, int args);
+static void initialize_dh(void);
static void initialize_ecdh(void);
static const char *SSLerrmessage(unsigned long ecode);
@@ -94,14 +95,14 @@ static SSL_CTX *SSL_context = NULL;
* sessions even if the static private key is compromised,
* so we are *highly* motivated to ensure that we can use
* EDH even if the DBA... or an attacker... deletes the
- * $DataDir/dh*.pem files.
+ * $DataDir/dh_params.pem file.
*
* We could refuse SSL connections unless a good DH parameter
* file exists, but some clients may quietly renegotiate an
* unsecured connection without fully informing the user.
* Very uncool.
*
- * Alternatively, the backend could attempt to load these files
+ * Alternatively, the backend could attempt to load the file
* on startup if SSL is enabled - and refuse to start if any
* do not exist - but this would tend to piss off DBAs.
*
@@ -111,19 +112,6 @@ static SSL_CTX *SSL_context = NULL;
* for suggestions.
*/
-static const char file_dh512[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MEYCQQD1Kv884bEpQBgRjXyEpwpy1obEAxnIByl6ypUM2Zafq9AKUJsCRtMIPWak\n\
-XUGfnHy9iUsiGSa6q6Jew1XpKgVfAgEC\n\
------END DH PARAMETERS-----\n";
-
-static const char file_dh1024[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MIGHAoGBAPSI/VhOSdvNILSd5JEHNmszbDgNRR0PfIizHHxbLY7288kjwEPwpVsY\n\
-jY67VYy4XTjTNP18F1dDox0YbN4zISy1Kv884bEpQBgRjXyEpwpy1obEAxnIByl6\n\
-ypUM2Zafq9AKUJsCRtMIPWakXUGfnHy9iUsiGSa6q6Jew1XpL3jHAgEC\n\
------END DH PARAMETERS-----\n";
-
static const char file_dh2048[] =
"-----BEGIN DH PARAMETERS-----\n\
MIIBCAKCAQEA9kJXtwh/CBdyorrWqULzBej5UxE5T7bxbrlLOCDaAadWoxTpj0BV\n\
@@ -134,21 +122,6 @@ Q6MdGGzeMyEstSr/POGxKUAYEY18hKcKctaGxAMZyAcpesqVDNmWn6vQClCbAkbT\n\
CD1mpF1Bn5x8vYlLIhkmuquiXsNV6TILOwIBAg==\n\
-----END DH PARAMETERS-----\n";
-static const char file_dh4096[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MIICCAKCAgEA+hRyUsFN4VpJ1O8JLcCo/VWr19k3BCgJ4uk+d+KhehjdRqNDNyOQ\n\
-l/MOyQNQfWXPeGKmOmIig6Ev/nm6Nf9Z2B1h3R4hExf+zTiHnvVPeRBhjdQi81rt\n\
-Xeoh6TNrSBIKIHfUJWBh3va0TxxjQIs6IZOLeVNRLMqzeylWqMf49HsIXqbcokUS\n\
-Vt1BkvLdW48j8PPv5DsKRN3tloTxqDJGo9tKvj1Fuk74A+Xda1kNhB7KFlqMyN98\n\
-VETEJ6c7KpfOo30mnK30wqw3S8OtaIR/maYX72tGOno2ehFDkq3pnPtEbD2CScxc\n\
-alJC+EL7RPk5c/tgeTvCngvc1KZn92Y//EI7G9tPZtylj2b56sHtMftIoYJ9+ODM\n\
-sccD5Piz/rejE3Ome8EOOceUSCYAhXn8b3qvxVI1ddd1pED6FHRhFvLrZxFvBEM9\n\
-ERRMp5QqOaHJkM+Dxv8Cj6MqrCbfC4u+ZErxodzuusgDgvZiLF22uxMZbobFWyte\n\
-OvOzKGtwcTqO/1wV5gKkzu1ZVswVUQd5Gg8lJicwqRWyyNRczDDoG9jVDxmogKTH\n\
-AaqLulO7R8Ifa1SwF2DteSGVtgWEN8gDpN3RBmmPTDngyF2DHb5qmpnznwtFKdTL\n\
-KWbuHn491xNO25CQWMtem80uKw+pTnisBRF/454n1Jnhub144YRBoN8CAQI=\n\
------END DH PARAMETERS-----\n";
-
/* ------------------------------------------------------------ */
/* Public interface */
@@ -261,13 +234,12 @@ be_tls_init(void)
SSLerrmessage(ERR_get_error()))));
}
- /* set up ephemeral DH keys, and disallow SSL v2/v3 while at it */
- SSL_CTX_set_tmp_dh_callback(SSL_context, tmp_dh_cb);
+ /* disallow SSL v2/v3 */
SSL_CTX_set_options(SSL_context,
- SSL_OP_SINGLE_DH_USE |
SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
- /* set up ephemeral ECDH keys */
+ /* set up ephemeral DH and ECDH keys */
+ initialize_dh();
initialize_ecdh();
/* set up the allowed cipher list */
@@ -800,43 +772,32 @@ err:
* what we expect it to contain.
*/
static DH *
-load_dh_file(int keylength)
+load_dh_file(void)
{
FILE *fp;
- char fnbuf[MAXPGPATH];
+ char *filename = DH_PARAMS_FILE;
DH *dh = NULL;
int codes;
/* attempt to open file. It's not an error if it doesn't exist. */
- snprintf(fnbuf, sizeof(fnbuf), "dh%d.pem", keylength);
- if ((fp = fopen(fnbuf, "r")) == NULL)
+ if ((fp = fopen(filename, "r")) == NULL)
return NULL;
-/* flock(fileno(fp), LOCK_SH); */
dh = PEM_read_DHparams(fp, NULL, NULL, NULL);
-/* flock(fileno(fp), LOCK_UN); */
fclose(fp);
- /* is the prime the correct size? */
- if (dh != NULL && 8 * DH_size(dh) < keylength)
- {
- elog(LOG, "DH errors (%s): %d bits expected, %d bits found",
- fnbuf, keylength, 8 * DH_size(dh));
- dh = NULL;
- }
-
/* make sure the DH parameters are usable */
if (dh != NULL)
{
if (DH_check(dh, &codes) == 0)
{
- elog(LOG, "DH_check error (%s): %s", fnbuf,
+ elog(LOG, "DH_check error (%s): %s", filename,
SSLerrmessage(ERR_get_error()));
return NULL;
}
if (codes & DH_CHECK_P_NOT_PRIME)
{
- elog(LOG, "DH error (%s): p is not prime", fnbuf);
+ elog(LOG, "DH error (%s): p is not prime", filename);
return NULL;
}
if ((codes & DH_NOT_SUITABLE_GENERATOR) &&
@@ -844,7 +805,7 @@ load_dh_file(int keylength)
{
elog(LOG,
"DH error (%s): neither suitable generator or safe prime",
- fnbuf);
+ filename);
return NULL;
}
}
@@ -878,102 +839,6 @@ load_dh_buffer(const char *buffer, size_t len)
}
/*
- * Generate DH parameters.
- *
- * Last resort if we can't load precomputed nor hardcoded
- * parameters.
- */
-static DH *
-generate_dh_parameters(int prime_len, int generator)
-{
- DH *dh;
-
- if ((dh = DH_new()) == NULL)
- return NULL;
-
- if (DH_generate_parameters_ex(dh, prime_len, generator, NULL))
- return dh;
-
- DH_free(dh);
- return NULL;
-}
-
-/*
- * Generate an ephemeral DH key. Because this can take a long
- * time to compute, we can use precomputed parameters of the
- * common key sizes.
- *
- * Since few sites will bother to precompute these parameter
- * files, we also provide a fallback to the parameters provided
- * by the OpenSSL project.
- *
- * These values can be static (once loaded or computed) since
- * the OpenSSL library can efficiently generate random keys from
- * the information provided.
- */
-static DH *
-tmp_dh_cb(SSL *s, int is_export, int keylength)
-{
- DH *r = NULL;
- static DH *dh = NULL;
- static DH *dh512 = NULL;
- static DH *dh1024 = NULL;
- static DH *dh2048 = NULL;
- static DH *dh4096 = NULL;
-
- switch (keylength)
- {
- case 512:
- if (dh512 == NULL)
- dh512 = load_dh_file(keylength);
- if (dh512 == NULL)
- dh512 = load_dh_buffer(file_dh512, sizeof file_dh512);
- r = dh512;
- break;
-
- case 1024:
- if (dh1024 == NULL)
- dh1024 = load_dh_file(keylength);
- if (dh1024 == NULL)
- dh1024 = load_dh_buffer(file_dh1024, sizeof file_dh1024);
- r = dh1024;
- break;
-
- case 2048:
- if (dh2048 == NULL)
- dh2048 = load_dh_file(keylength);
- if (dh2048 == NULL)
- dh2048 = load_dh_buffer(file_dh2048, sizeof file_dh2048);
- r = dh2048;
- break;
-
- case 4096:
- if (dh4096 == NULL)
- dh4096 = load_dh_file(keylength);
- if (dh4096 == NULL)
- dh4096 = load_dh_buffer(file_dh4096, sizeof file_dh4096);
- r = dh4096;
- break;
-
- default:
- if (dh == NULL)
- dh = load_dh_file(keylength);
- r = dh;
- }
-
- /* this may take a long time, but it may be necessary... */
- if (r == NULL || 8 * DH_size(r) < keylength)
- {
- ereport(DEBUG2,
- (errmsg_internal("DH: generating parameters (%d bits)",
- keylength)));
- r = generate_dh_parameters(keylength, DH_GENERATOR_2);
- }
-
- return r;
-}
-
-/*
* Certificate verification callback
*
* This callback allows us to log intermediate problems during
@@ -1034,6 +899,39 @@ info_cb(const SSL *ssl, int type, int args)
}
}
+/*
+ * Obtain DH parameters for generating ephemeral DH keys. Because
+ * this can take a long time to compute, these must be precomputed.
+ *
+ * Since few sites will bother to precompute these parameter files,
+ * we also also provide a fallback to the parameters provided by the
+ * OpenSSL project.
+ *
+ * These values can be static (once loaded or computed) since the
+ * OpenSSL library can efficiently generate random keys from the
+ * information provided.
+ */
+static void
+initialize_dh(void)
+{
+ DH *dh;
+
+ dh = load_dh_file();
+ if (!dh)
+ dh = load_dh_buffer(file_dh2048, sizeof file_dh2048);
+ if (!dh)
+ ereport(FATAL,
+ (errmsg("DH: could not create key")));
+
+ SSL_CTX_set_options(SSL_context, SSL_OP_SINGLE_DH_USE);
+ SSL_CTX_set_tmp_dh(SSL_context, dh);
+}
+
+/*
+ * Set ECDH parameters for generating ephemeral Elliptic Curve DH
+ * keys. This is much simpler than the DH parameters, as we just
+ * need to provide the name of the curve to OpenSSL.
+ */
static void
initialize_ecdh(void)
{
--
2.9.3
Re: Heikki Linnakangas 2016-10-06 <fd6eb3cc-1585-1469-fd9e-763f8e410b19@iki.fi>
I propose the attached patch. It gives up on trying to deal with multiple
key lengths (as noted earlier, OpenSSL just always passed keylength=1024, so
that was useless). Instead of using the callback, it just sets fixed DH
parameters with SSL_CTX_set_tmp_dh(), like we do for the ECDH curve. The DH
parameters are loaded from a file called "dh_params.pem" (instead of
"dh1024.pem"), if present, otherwise the built-in 2048 bit parameters are
used.
Shouldn't this be a GUC pointing to a configurable location like
ssl_cert_file? This way, people reading the security section of the
default postgresql.conf would notice that there's something (new) to
configure. (And I wouldn't want to start creating symlinks from PGDATA
to /etc/ssl/something again...)
Thanks,
Christoph
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/06/2016 10:26 PM, Christoph Berg wrote:
Re: Heikki Linnakangas 2016-10-06 <fd6eb3cc-1585-1469-fd9e-763f8e410b19@iki.fi>
I propose the attached patch. It gives up on trying to deal with multiple
key lengths (as noted earlier, OpenSSL just always passed keylength=1024, so
that was useless). Instead of using the callback, it just sets fixed DH
parameters with SSL_CTX_set_tmp_dh(), like we do for the ECDH curve. The DH
parameters are loaded from a file called "dh_params.pem" (instead of
"dh1024.pem"), if present, otherwise the built-in 2048 bit parameters are
used.Shouldn't this be a GUC pointing to a configurable location like
ssl_cert_file? This way, people reading the security section of the
default postgresql.conf would notice that there's something (new) to
configure. (And I wouldn't want to start creating symlinks from PGDATA
to /etc/ssl/something again...)
Perhaps. The DH parameters are not quite like other configuration files,
though. The actual parameters used don't matter, you just want to
generate different parameters for every cluster. The key length of the
parameters can be considered configuration, though.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
06.10.2016, 16:52, Heikki Linnakangas kirjoitti:
I propose the attached patch. It gives up on trying to deal with
multiple key lengths (as noted earlier, OpenSSL just always passed
keylength=1024, so that was useless). Instead of using the callback, it
just sets fixed DH parameters with SSL_CTX_set_tmp_dh(), like we do for
the ECDH curve. The DH parameters are loaded from a file called
"dh_params.pem" (instead of "dh1024.pem"), if present, otherwise the
built-in 2048 bit parameters are used.
We've been using the same built-in parameters for 14 years now, they
apparently came from
https://web.archive.org/web/20011212141438/http://www.skip-vpn.org/spec/numbers.html
(the original page is no longer available) and are shared by countless
other systems.
While we're not using the most common Oakley groups which are presumed
to have been broken by various parties (https://weakdh.org) I think it'd
be worthwhile to replace the currently built-in parameters with custom
ones. And maybe even regenerate parameters for every minor release.
HAProxy made a similar change last year, see
https://github.com/haproxy/haproxy/commit/d3a341a96fb6107d2b8e3d7a9c0afa2ff43bb0b6
/ Oskari
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
(We dropped the ball back in October, continuing the discussion now)
On 10/10/2016 06:24 PM, Heikki Linnakangas wrote:
On 10/06/2016 10:26 PM, Christoph Berg wrote:
Re: Heikki Linnakangas 2016-10-06 <fd6eb3cc-1585-1469-fd9e-763f8e410b19@iki.fi>
I propose the attached patch. It gives up on trying to deal with multiple
key lengths (as noted earlier, OpenSSL just always passed keylength=1024, so
that was useless). Instead of using the callback, it just sets fixed DH
parameters with SSL_CTX_set_tmp_dh(), like we do for the ECDH curve. The DH
parameters are loaded from a file called "dh_params.pem" (instead of
"dh1024.pem"), if present, otherwise the built-in 2048 bit parameters are
used.Shouldn't this be a GUC pointing to a configurable location like
ssl_cert_file? This way, people reading the security section of the
default postgresql.conf would notice that there's something (new) to
configure. (And I wouldn't want to start creating symlinks from PGDATA
to /etc/ssl/something again...)Perhaps. The DH parameters are not quite like other configuration files,
though. The actual parameters used don't matter, you just want to
generate different parameters for every cluster. The key length of the
parameters can be considered configuration, though.
Actually, adding a GUC also solves another grief I had about this. There
is currently no easy way to tell if your parameter file is being used,
or if the server is failing to read it and is falling back to the
hard-coded parameters. With a GUC, if the GUC is set it's a good
indication that the admin expects the file to really exist and to
contain valid parameters. So if the GUC is set, we should throw an error
if it cannot be used. And if it's not set, we use the built-in defaults.
I rebased the patch, did some other clean up of error reporting, and
added a GUC along those lines, as well as docs. How does this look?
It's late in the release cycle, but it would be nice to sneak this into
v10. Using weak 1024 bit DH parameters is arguably a security issue; it
was originally reported as such. There's a work-around for older
versions: generate custom 2048 bit parameters and place them in a file
called "dh1024.pem", but that's completely undocumented.
Thoughts? Objections to committing this now, instead of waiting for v11?
- Heikki
Attachments:
0001-Always-use-2048-bit-DH-parameters-for-OpenSSL-epheme.patchtext/x-diff; name=0001-Always-use-2048-bit-DH-parameters-for-OpenSSL-epheme.patchDownload
From 481e019441dadc0826e6e9b7d4a56c49e63c654b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Thu, 13 Jul 2017 18:29:48 +0300
Subject: [PATCH 1/1] Always use 2048 bit DH parameters for OpenSSL ephemeral
DH ciphers.
1024 bits is considered weak these days, but OpenSSL always passes 1024 as
the key length to the tmp_dh callback. All the code to handle other key
lengths as in fact dead.
To remedy those issues:
* Only include hard-coded 2048-bit parameters.
* Set the parameters directly with SSL_CTX_set_tmp_dh(), without the
callback
* The name of the file containing the DH parameters is now a GUC. This
replaces the old hardcoded "dh1024.pem" filename. (The files for other
key lengths, dh512.pem, dh2048.pem, etc. were never actually used.)
Per report by Nicolas Guini
Discussion: https://www.postgresql.org/message-id/CAMxBoUyjOOautVozN6ofzym828aNrDjuCcOTcCquxjwS-L2hGQ@mail.gmail.com
---
doc/src/sgml/config.sgml | 23 +++
src/backend/libpq/be-secure-openssl.c | 265 +++++++++-----------------
src/backend/libpq/be-secure.c | 1 +
src/backend/utils/misc/guc.c | 11 ++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/libpq/libpq.h | 1 +
6 files changed, 132 insertions(+), 170 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 80d1679b14..6c1452a9bc 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1203,6 +1203,29 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-ssl-dh-params-file" xreflabel="ssl_dh_params_file">
+ <term><varname>ssl_dh_params_file</varname> (<type>string</type>)
+ <indexterm>
+ <primary><varname>ssl_ecdh_curve</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specifies the name of the file containing Diffie-Hellman parameters used for
+ so-called Perfect Forward Secrecy SSL ciphers. The default is empty, in
+ which case compiled-in default DH parameters used. Using custom DH
+ parameters reduces the exposure if an attacker manages to crack the
+ well-known compiled-in DH parameters. You can create your own DH parameters
+ file with the command <command>openssl dhparam -out dhparams.pem 2048</command>.
+ </para>
+
+ <para>
+ This parameter can only be set in the <filename>postgresql.conf</>
+ file or on the server command line.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-krb-server-keyfile" xreflabel="krb_server_keyfile">
<term><varname>krb_server_keyfile</varname> (<type>string</type>)
<indexterm>
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 67145e9412..ed1779d6b6 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -61,23 +61,22 @@
#include "libpq/libpq.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "storage/fd.h"
#include "storage/latch.h"
#include "tcop/tcopprot.h"
#include "utils/memutils.h"
-
static int my_sock_read(BIO *h, char *buf, int size);
static int my_sock_write(BIO *h, const char *buf, int size);
static BIO_METHOD *my_BIO_s_socket(void);
static int my_SSL_set_fd(Port *port, int fd);
-static DH *load_dh_file(int keylength);
+static DH *load_dh_file(char *filename, bool isServerStart);
static DH *load_dh_buffer(const char *, size_t);
-static DH *generate_dh_parameters(int prime_len, int generator);
-static DH *tmp_dh_cb(SSL *s, int is_export, int keylength);
static int ssl_passwd_cb(char *buf, int size, int rwflag, void *userdata);
static int verify_cb(int, X509_STORE_CTX *);
static void info_cb(const SSL *ssl, int type, int args);
+static bool initialize_dh(SSL_CTX *context, bool isServerStart);
static bool initialize_ecdh(SSL_CTX *context, bool isServerStart);
static const char *SSLerrmessage(unsigned long ecode);
@@ -96,17 +95,14 @@ static bool ssl_passwd_cb_called = false;
* As discussed above, EDH protects the confidentiality of
* sessions even if the static private key is compromised,
* so we are *highly* motivated to ensure that we can use
- * EDH even if the DBA... or an attacker... deletes the
- * $DataDir/dh*.pem files.
+ * EDH even if the DBA has not provided custom DH parameters.
*
* We could refuse SSL connections unless a good DH parameter
* file exists, but some clients may quietly renegotiate an
* unsecured connection without fully informing the user.
- * Very uncool.
- *
- * Alternatively, the backend could attempt to load these files
- * on startup if SSL is enabled - and refuse to start if any
- * do not exist - but this would tend to piss off DBAs.
+ * Very uncool. Alternatively, the system could refuse to start
+ * if a DH parameters if not specified, but this would tend to
+ * piss off DBAs.
*
* If you want to create your own hardcoded DH parameters
* for fun and profit, review "Assigned Number for SKIP
@@ -114,19 +110,6 @@ static bool ssl_passwd_cb_called = false;
* for suggestions.
*/
-static const char file_dh512[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MEYCQQD1Kv884bEpQBgRjXyEpwpy1obEAxnIByl6ypUM2Zafq9AKUJsCRtMIPWak\n\
-XUGfnHy9iUsiGSa6q6Jew1XpKgVfAgEC\n\
------END DH PARAMETERS-----\n";
-
-static const char file_dh1024[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MIGHAoGBAPSI/VhOSdvNILSd5JEHNmszbDgNRR0PfIizHHxbLY7288kjwEPwpVsY\n\
-jY67VYy4XTjTNP18F1dDox0YbN4zISy1Kv884bEpQBgRjXyEpwpy1obEAxnIByl6\n\
-ypUM2Zafq9AKUJsCRtMIPWakXUGfnHy9iUsiGSa6q6Jew1XpL3jHAgEC\n\
------END DH PARAMETERS-----\n";
-
static const char file_dh2048[] =
"-----BEGIN DH PARAMETERS-----\n\
MIIBCAKCAQEA9kJXtwh/CBdyorrWqULzBej5UxE5T7bxbrlLOCDaAadWoxTpj0BV\n\
@@ -137,21 +120,6 @@ Q6MdGGzeMyEstSr/POGxKUAYEY18hKcKctaGxAMZyAcpesqVDNmWn6vQClCbAkbT\n\
CD1mpF1Bn5x8vYlLIhkmuquiXsNV6TILOwIBAg==\n\
-----END DH PARAMETERS-----\n";
-static const char file_dh4096[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MIICCAKCAgEA+hRyUsFN4VpJ1O8JLcCo/VWr19k3BCgJ4uk+d+KhehjdRqNDNyOQ\n\
-l/MOyQNQfWXPeGKmOmIig6Ev/nm6Nf9Z2B1h3R4hExf+zTiHnvVPeRBhjdQi81rt\n\
-Xeoh6TNrSBIKIHfUJWBh3va0TxxjQIs6IZOLeVNRLMqzeylWqMf49HsIXqbcokUS\n\
-Vt1BkvLdW48j8PPv5DsKRN3tloTxqDJGo9tKvj1Fuk74A+Xda1kNhB7KFlqMyN98\n\
-VETEJ6c7KpfOo30mnK30wqw3S8OtaIR/maYX72tGOno2ehFDkq3pnPtEbD2CScxc\n\
-alJC+EL7RPk5c/tgeTvCngvc1KZn92Y//EI7G9tPZtylj2b56sHtMftIoYJ9+ODM\n\
-sccD5Piz/rejE3Ome8EOOceUSCYAhXn8b3qvxVI1ddd1pED6FHRhFvLrZxFvBEM9\n\
-ERRMp5QqOaHJkM+Dxv8Cj6MqrCbfC4u+ZErxodzuusgDgvZiLF22uxMZbobFWyte\n\
-OvOzKGtwcTqO/1wV5gKkzu1ZVswVUQd5Gg8lJicwqRWyyNRczDDoG9jVDxmogKTH\n\
-AaqLulO7R8Ifa1SwF2DteSGVtgWEN8gDpN3RBmmPTDngyF2DHb5qmpnznwtFKdTL\n\
-KWbuHn491xNO25CQWMtem80uKw+pTnisBRF/454n1Jnhub144YRBoN8CAQI=\n\
------END DH PARAMETERS-----\n";
-
/* ------------------------------------------------------------ */
/* Public interface */
@@ -316,13 +284,14 @@ be_tls_init(bool isServerStart)
goto error;
}
- /* set up ephemeral DH keys, and disallow SSL v2/v3 while at it */
- SSL_CTX_set_tmp_dh_callback(context, tmp_dh_cb);
+ /* disallow SSL v2/v3 */
SSL_CTX_set_options(context,
SSL_OP_SINGLE_DH_USE |
SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
- /* set up ephemeral ECDH keys */
+ /* set up ephemeral DH and ECDH keys */
+ if (!initialize_dh(context, isServerStart))
+ goto error;
if (!initialize_ecdh(context, isServerStart))
goto error;
@@ -918,53 +887,57 @@ err:
* what we expect it to contain.
*/
static DH *
-load_dh_file(int keylength)
+load_dh_file(char *filename, bool isServerStart)
{
FILE *fp;
- char fnbuf[MAXPGPATH];
DH *dh = NULL;
int codes;
/* attempt to open file. It's not an error if it doesn't exist. */
- snprintf(fnbuf, sizeof(fnbuf), "dh%d.pem", keylength);
- if ((fp = fopen(fnbuf, "r")) == NULL)
+ if ((fp = AllocateFile(filename, "r")) == NULL)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode_for_file_access(),
+ errmsg("could not open DH parameters file \"%s\": %m",
+ filename)));
return NULL;
+ }
-/* flock(fileno(fp), LOCK_SH); */
dh = PEM_read_DHparams(fp, NULL, NULL, NULL);
-/* flock(fileno(fp), LOCK_UN); */
- fclose(fp);
+ FreeFile(fp);
- /* is the prime the correct size? */
- if (dh != NULL && 8 * DH_size(dh) < keylength)
+ if (dh == NULL)
{
- elog(LOG, "DH errors (%s): %d bits expected, %d bits found",
- fnbuf, keylength, 8 * DH_size(dh));
- dh = NULL;
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("could not load DH parameters file: %s",
+ SSLerrmessage(ERR_get_error()))));
+ return NULL;
}
/* make sure the DH parameters are usable */
- if (dh != NULL)
+ if (DH_check(dh, &codes) == 0)
{
- if (DH_check(dh, &codes) == 0)
- {
- elog(LOG, "DH_check error (%s): %s", fnbuf,
- SSLerrmessage(ERR_get_error()));
- return NULL;
- }
- if (codes & DH_CHECK_P_NOT_PRIME)
- {
- elog(LOG, "DH error (%s): p is not prime", fnbuf);
- return NULL;
- }
- if ((codes & DH_NOT_SUITABLE_GENERATOR) &&
- (codes & DH_CHECK_P_NOT_SAFE_PRIME))
- {
- elog(LOG,
- "DH error (%s): neither suitable generator or safe prime",
- fnbuf);
- return NULL;
- }
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("invalid DH parameters: %s",
+ SSLerrmessage(ERR_get_error()))));
+ return NULL;
+ }
+ if (codes & DH_CHECK_P_NOT_PRIME)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("invalid DH parameters: p is not prime")));
+ return NULL;
+ }
+ if ((codes & DH_NOT_SUITABLE_GENERATOR) &&
+ (codes & DH_CHECK_P_NOT_SAFE_PRIME))
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("invalid DH parameters: neither suitable generator or safe prime")));
+ return NULL;
}
return dh;
@@ -996,102 +969,6 @@ load_dh_buffer(const char *buffer, size_t len)
}
/*
- * Generate DH parameters.
- *
- * Last resort if we can't load precomputed nor hardcoded
- * parameters.
- */
-static DH *
-generate_dh_parameters(int prime_len, int generator)
-{
- DH *dh;
-
- if ((dh = DH_new()) == NULL)
- return NULL;
-
- if (DH_generate_parameters_ex(dh, prime_len, generator, NULL))
- return dh;
-
- DH_free(dh);
- return NULL;
-}
-
-/*
- * Generate an ephemeral DH key. Because this can take a long
- * time to compute, we can use precomputed parameters of the
- * common key sizes.
- *
- * Since few sites will bother to precompute these parameter
- * files, we also provide a fallback to the parameters provided
- * by the OpenSSL project.
- *
- * These values can be static (once loaded or computed) since
- * the OpenSSL library can efficiently generate random keys from
- * the information provided.
- */
-static DH *
-tmp_dh_cb(SSL *s, int is_export, int keylength)
-{
- DH *r = NULL;
- static DH *dh = NULL;
- static DH *dh512 = NULL;
- static DH *dh1024 = NULL;
- static DH *dh2048 = NULL;
- static DH *dh4096 = NULL;
-
- switch (keylength)
- {
- case 512:
- if (dh512 == NULL)
- dh512 = load_dh_file(keylength);
- if (dh512 == NULL)
- dh512 = load_dh_buffer(file_dh512, sizeof file_dh512);
- r = dh512;
- break;
-
- case 1024:
- if (dh1024 == NULL)
- dh1024 = load_dh_file(keylength);
- if (dh1024 == NULL)
- dh1024 = load_dh_buffer(file_dh1024, sizeof file_dh1024);
- r = dh1024;
- break;
-
- case 2048:
- if (dh2048 == NULL)
- dh2048 = load_dh_file(keylength);
- if (dh2048 == NULL)
- dh2048 = load_dh_buffer(file_dh2048, sizeof file_dh2048);
- r = dh2048;
- break;
-
- case 4096:
- if (dh4096 == NULL)
- dh4096 = load_dh_file(keylength);
- if (dh4096 == NULL)
- dh4096 = load_dh_buffer(file_dh4096, sizeof file_dh4096);
- r = dh4096;
- break;
-
- default:
- if (dh == NULL)
- dh = load_dh_file(keylength);
- r = dh;
- }
-
- /* this may take a long time, but it may be necessary... */
- if (r == NULL || 8 * DH_size(r) < keylength)
- {
- ereport(DEBUG2,
- (errmsg_internal("DH: generating parameters (%d bits)",
- keylength)));
- r = generate_dh_parameters(keylength, DH_GENERATOR_2);
- }
-
- return r;
-}
-
-/*
* Passphrase collection callback
*
* If OpenSSL is told to use a passphrase-protected server key, by default
@@ -1172,6 +1049,54 @@ info_cb(const SSL *ssl, int type, int args)
}
}
+/*
+ * Set DH parameters for generating ephemeral DH keys. The
+ * DH parameters can take a long time to compute, so they must be
+ * precomputed.
+ *
+ * Since few sites will bother to create a parameter file, we also
+ * also provide a fallback to the parameters provided by the
+ * OpenSSL project.
+ *
+ * These values can be static (once loaded or computed) since the
+ * OpenSSL library can efficiently generate random keys from the
+ * information provided.
+ */
+static bool
+initialize_dh(SSL_CTX *context, bool isServerStart)
+{
+ DH *dh = NULL;
+
+ SSL_CTX_set_options(context, SSL_OP_SINGLE_DH_USE);
+
+ if (ssl_dh_params_file[0])
+ dh = load_dh_file(ssl_dh_params_file, isServerStart);
+ if (!dh)
+ dh = load_dh_buffer(file_dh2048, sizeof file_dh2048);
+ if (!dh)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ (errmsg("DH: could not load DH parameters"))));
+ return false;
+ }
+
+ if (SSL_CTX_set_tmp_dh(context, dh) != 1)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ (errmsg("DH: could not set DH parameters: %s",
+ SSLerrmessage(ERR_get_error())))));
+ return false;
+ }
+ return true;
+}
+
+/*
+ * Set ECDH parameters for generating ephemeral Elliptic Curve DH
+ * keys. This is much simpler than the DH parameters, as we just
+ * need to provide the name of the curve to OpenSSL.
+ */
static bool
initialize_ecdh(SSL_CTX *context, bool isServerStart)
{
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 785dadb6c2..53fefd1b29 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -44,6 +44,7 @@ char *ssl_cert_file;
char *ssl_key_file;
char *ssl_ca_file;
char *ssl_crl_file;
+char *ssl_dh_params_file;
#ifdef USE_SSL
bool ssl_loaded_verify_locations = false;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 82e54c084b..246fea8693 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3607,6 +3607,17 @@ static struct config_string ConfigureNamesString[] =
},
{
+ {"ssl_dh_params_file", PGC_SIGHUP, CONN_AUTH_SECURITY,
+ gettext_noop("Location of the SSL DH params file."),
+ NULL,
+ GUC_SUPERUSER_ONLY
+ },
+ &ssl_dh_params_file,
+ "",
+ NULL, NULL, NULL
+ },
+
+ {
{"application_name", PGC_USERSET, LOGGING_WHAT,
gettext_noop("Sets the application name to be reported in statistics and logs."),
NULL,
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 2b1ebb797e..72a9098ce6 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -80,6 +80,7 @@
#ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
#ssl_prefer_server_ciphers = on
#ssl_ecdh_curve = 'prime256v1'
+#ssl_dh_params_file = ''
#ssl_cert_file = 'server.crt'
#ssl_key_file = 'server.key'
#ssl_ca_file = ''
diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index 78851b1060..fd2dd5853c 100644
--- a/src/include/libpq/libpq.h
+++ b/src/include/libpq/libpq.h
@@ -79,6 +79,7 @@ extern char *ssl_cert_file;
extern char *ssl_key_file;
extern char *ssl_ca_file;
extern char *ssl_crl_file;
+extern char *ssl_dh_params_file;
extern int secure_initialize(bool isServerStart);
extern bool secure_loaded_verify_locations(void);
--
2.11.0
On Thu, Jul 13, 2017 at 5:32 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I rebased the patch, did some other clean up of error reporting, and added a
GUC along those lines, as well as docs. How does this look?It's late in the release cycle, but it would be nice to sneak this into v10.
Using weak 1024 bit DH parameters is arguably a security issue; it was
originally reported as such. There's a work-around for older versions:
generate custom 2048 bit parameters and place them in a file called
"dh1024.pem", but that's completely undocumented.Thoughts?
The patch looks in good shape to me.
#include "utils/memutils.h"
-
static int my_sock_read(BIO *h, char *buf, int size);
That's unnecessary noise.
+ * Very uncool. Alternatively, the system could refuse to start
+ * if a DH parameters if not specified, but this would tend to
+ * piss off DBAs.
"is not specified".Objections to committing this now, instead of waiting for v11?
But I am -1 for the sneak part. It is not the time to have a new
feature in 10, the focus is to stabilize.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier wrote:
On Thu, Jul 13, 2017 at 5:32 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Objections to committing this now, instead of waiting for v11?
But I am -1 for the sneak part. It is not the time to have a new
feature in 10, the focus is to stabilize.
But if we were treating it as a security issue, would we backpatch it?
If we do, then it definitely makes sense to put something in pg10. I'm
not sure that this patch is it, though -- perhaps it makes sense to put
a minimal fix in older branches, and let the new feature wait for pg11?
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 13 July 2017 at 16:32, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
(We dropped the ball back in October, continuing the discussion now)
On 10/10/2016 06:24 PM, Heikki Linnakangas wrote:
On 10/06/2016 10:26 PM, Christoph Berg wrote:
Re: Heikki Linnakangas 2016-10-06
<fd6eb3cc-1585-1469-fd9e-763f8e410b19@iki.fi>I propose the attached patch. It gives up on trying to deal with
multiple
key lengths (as noted earlier, OpenSSL just always passed
keylength=1024, so
that was useless). Instead of using the callback, it just sets fixed DH
parameters with SSL_CTX_set_tmp_dh(), like we do for the ECDH curve. The
DH
parameters are loaded from a file called "dh_params.pem" (instead of
"dh1024.pem"), if present, otherwise the built-in 2048 bit parameters
are
used.Shouldn't this be a GUC pointing to a configurable location like
ssl_cert_file? This way, people reading the security section of the
default postgresql.conf would notice that there's something (new) to
configure. (And I wouldn't want to start creating symlinks from PGDATA
to /etc/ssl/something again...)Perhaps. The DH parameters are not quite like other configuration files,
though. The actual parameters used don't matter, you just want to
generate different parameters for every cluster. The key length of the
parameters can be considered configuration, though.Actually, adding a GUC also solves another grief I had about this. There is
currently no easy way to tell if your parameter file is being used, or if
the server is failing to read it and is falling back to the hard-coded
parameters. With a GUC, if the GUC is set it's a good indication that the
admin expects the file to really exist and to contain valid parameters. So
if the GUC is set, we should throw an error if it cannot be used. And if
it's not set, we use the built-in defaults.I rebased the patch, did some other clean up of error reporting, and added a
GUC along those lines, as well as docs. How does this look?It's late in the release cycle, but it would be nice to sneak this into v10.
Using weak 1024 bit DH parameters is arguably a security issue; it was
originally reported as such. There's a work-around for older versions:
generate custom 2048 bit parameters and place them in a file called
"dh1024.pem", but that's completely undocumented.Thoughts? Objections to committing this now, instead of waiting for v11?
+1 to include important open items such as this
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: Alvaro Herrera 2017-07-13 <20170713170402.74uuoivrgd3c6tnw@alvherre.pgsql>
Objections to committing this now, instead of waiting for v11?
But I am -1 for the sneak part. It is not the time to have a new
feature in 10, the focus is to stabilize.But if we were treating it as a security issue, would we backpatch it?
If we do, then it definitely makes sense to put something in pg10. I'm
not sure that this patch is it, though -- perhaps it makes sense to put
a minimal fix in older branches, and let the new feature wait for pg11?
Making it user-configurable seems pretty minimal to me. Everything
else would probably require lengthy explanations about which file
could hold which contents, and this confusion seems to be part of the
problem.
Fwiw, wouldn't it make sense to recreate the default 2048 DH group as
well, maybe each time a new major is branched?
Christoph
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/13/2017 08:04 PM, Alvaro Herrera wrote:
Michael Paquier wrote:
On Thu, Jul 13, 2017 at 5:32 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Objections to committing this now, instead of waiting for v11?
But I am -1 for the sneak part. It is not the time to have a new
feature in 10, the focus is to stabilize.But if we were treating it as a security issue, would we backpatch it?
If we do, then it definitely makes sense to put something in pg10. I'm
not sure that this patch is it, though -- perhaps it makes sense to put
a minimal fix in older branches, and let the new feature wait for pg11?
I don't think this can be backpatched. It changes the default DH
parameters from 1024 bits to 2048 bits. That's a good thing for
security, but older clients might not support it, and would refuse to
connect or would fall back to something less secure. I don't think there
are many such clients around anymore, but it's nevertheless not
something we want to do in a stable release I think the best we can do
is to document the issue and the workaround. To recap, to use stronger
DH parameters in stable versions, you need to do "openssl dhparam -out
$PGDATA/dh1024.pem 2048".
But I'd like to take the opportunity to change this for new
installations, with v10, instead of waiting for another year. Of course,
you could say that for any new feature, too, but that doesn't
necessarily mean that it's a bad argument :-). It's a judgment call, for
sure.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/13/2017 01:07 PM, Simon Riggs wrote:
On 13 July 2017 at 16:32, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
(We dropped the ball back in October, continuing the discussion now)
On 10/10/2016 06:24 PM, Heikki Linnakangas wrote:
On 10/06/2016 10:26 PM, Christoph Berg wrote:
Re: Heikki Linnakangas 2016-10-06
<fd6eb3cc-1585-1469-fd9e-763f8e410b19@iki.fi>I propose the attached patch. It gives up on trying to deal with
multiple
key lengths (as noted earlier, OpenSSL just always passed
keylength=1024, so
that was useless). Instead of using the callback, it just sets fixed DH
parameters with SSL_CTX_set_tmp_dh(), like we do for the ECDH curve. The
DH
parameters are loaded from a file called "dh_params.pem" (instead of
"dh1024.pem"), if present, otherwise the built-in 2048 bit parameters
are
used.Shouldn't this be a GUC pointing to a configurable location like
ssl_cert_file? This way, people reading the security section of the
default postgresql.conf would notice that there's something (new) to
configure. (And I wouldn't want to start creating symlinks from PGDATA
to /etc/ssl/something again...)Perhaps. The DH parameters are not quite like other configuration files,
though. The actual parameters used don't matter, you just want to
generate different parameters for every cluster. The key length of the
parameters can be considered configuration, though.Actually, adding a GUC also solves another grief I had about this. There is
currently no easy way to tell if your parameter file is being used, or if
the server is failing to read it and is falling back to the hard-coded
parameters. With a GUC, if the GUC is set it's a good indication that the
admin expects the file to really exist and to contain valid parameters. So
if the GUC is set, we should throw an error if it cannot be used. And if
it's not set, we use the built-in defaults.I rebased the patch, did some other clean up of error reporting, and added a
GUC along those lines, as well as docs. How does this look?It's late in the release cycle, but it would be nice to sneak this into v10.
Using weak 1024 bit DH parameters is arguably a security issue; it was
originally reported as such. There's a work-around for older versions:
generate custom 2048 bit parameters and place them in a file called
"dh1024.pem", but that's completely undocumented.Thoughts? Objections to committing this now, instead of waiting for v11?
+1 to include important open items such as this
I have not looked at the patch itself, but conceptually +1 to include in
pg10.
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Heikki Linnakangas <hlinnaka@iki.fi> writes:
I don't think this can be backpatched. It changes the default DH
parameters from 1024 bits to 2048 bits. That's a good thing for
security, but older clients might not support it, and would refuse to
connect or would fall back to something less secure.
Do we have any hard information about which versions of which clients
might not support that? (In particular I'm wondering if any still exist
in the wild.)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jul 13, 2017 at 1:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
I don't think this can be backpatched. It changes the default DH
parameters from 1024 bits to 2048 bits. That's a good thing for
security, but older clients might not support it, and would refuse to
connect or would fall back to something less secure.Do we have any hard information about which versions of which clients
might not support that? (In particular I'm wondering if any still exist
in the wild.)
Yeah. If we break clients for v10 two months from release, some
drivers won't be updated by release time, and that sounds pretty
unfriendly to me. On the other hand, if there is only a theoretical
risk of breakage and no clients that we actually know about will have
a problem with it, then the argument for waiting is weaker. I'm not
generally very excited about changing things after beta2, which is
where are, but if this is a security issue then we might need to hold
our nose and go ahead. I'm against it if it's likely to cause
real-world connectivity problems, though.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/13/2017 10:13 PM, Robert Haas wrote:
On Thu, Jul 13, 2017 at 1:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
I don't think this can be backpatched. It changes the default DH
parameters from 1024 bits to 2048 bits. That's a good thing for
security, but older clients might not support it, and would refuse to
connect or would fall back to something less secure.Do we have any hard information about which versions of which clients
might not support that? (In particular I'm wondering if any still exist
in the wild.)Yeah. If we break clients for v10 two months from release, some
drivers won't be updated by release time, and that sounds pretty
unfriendly to me. On the other hand, if there is only a theoretical
risk of breakage and no clients that we actually know about will have
a problem with it, then the argument for waiting is weaker. I'm not
generally very excited about changing things after beta2, which is
where are, but if this is a security issue then we might need to hold
our nose and go ahead. I'm against it if it's likely to cause
real-world connectivity problems, though.
Googling around, I believe Java 6 is the only straggler [1]https://wiki.mozilla.org/Security/Server_Side_TLS#DHE_and_ECDHE_support. So we would
be breaking that. Java 7 also doesn't support DH parameters > 1024 bits,
but it supports ECDHE, which is prioritized over DH ciphers, so it
doesn't matter.
Java 6 was released back in 2006. The last public release was in 2013.
It wouldn't surprise me to still see it bundled with random proprietary
software packages, though. The official PostgreSQL JDBC driver still
supports it, but there has been discussion recently on dropping support
for it, and even for Java 7. [2]/messages/by-id/69ae857b-15cc-36dd-f380-6620ef1effb9@8kdata.com
I would be OK with breaking DH with Java 6 in PostgreSQL 10, especially
since there's a simple workaround (generate a 1024-bit DH parameters
file). I would be less enthusiastic about doing that in a minor release,
although maybe that wouldn't be too bad either, if we put a prominent
notice with the workaround in the release notes.
[1]: https://wiki.mozilla.org/Security/Server_Side_TLS#DHE_and_ECDHE_support
[2]: /messages/by-id/69ae857b-15cc-36dd-f380-6620ef1effb9@8kdata.com
/messages/by-id/69ae857b-15cc-36dd-f380-6620ef1effb9@8kdata.com
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/13/2017 11:07 PM, Heikki Linnakangas wrote:
On 07/13/2017 10:13 PM, Robert Haas wrote:
On Thu, Jul 13, 2017 at 1:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
I don't think this can be backpatched. It changes the default DH
parameters from 1024 bits to 2048 bits. That's a good thing for
security, but older clients might not support it, and would refuse to
connect or would fall back to something less secure.Do we have any hard information about which versions of which clients
might not support that? (In particular I'm wondering if any still exist
in the wild.)Yeah. If we break clients for v10 two months from release, some
drivers won't be updated by release time, and that sounds pretty
unfriendly to me. On the other hand, if there is only a theoretical
risk of breakage and no clients that we actually know about will have
a problem with it, then the argument for waiting is weaker. I'm not
generally very excited about changing things after beta2, which is
where are, but if this is a security issue then we might need to hold
our nose and go ahead. I'm against it if it's likely to cause
real-world connectivity problems, though.Googling around, I believe Java 6 is the only straggler [1]. So we would
be breaking that. Java 7 also doesn't support DH parameters > 1024 bits,
but it supports ECDHE, which is prioritized over DH ciphers, so it
doesn't matter.Java 6 was released back in 2006. The last public release was in 2013.
It wouldn't surprise me to still see it bundled with random proprietary
software packages, though. The official PostgreSQL JDBC driver still
supports it, but there has been discussion recently on dropping support
for it, and even for Java 7. [2]I would be OK with breaking DH with Java 6 in PostgreSQL 10, especially
since there's a simple workaround (generate a 1024-bit DH parameters
file). I would be less enthusiastic about doing that in a minor release,
although maybe that wouldn't be too bad either, if we put a prominent
notice with the workaround in the release notes.
Some more information on the situation with JDK version 6: I installed
Debian wheezy on a VM, with a OpenJDK 6, and tested connecting to a
patched server with the JDBC driver. It worked! Googling around, it
seems that this was fixed in later versions of OpenJDK 6
(https://bugs.openjdk.java.net/browse/JDK-8062834). I then downloaded
the latest Oracle JRE binary version, 6u45, available from
http://www.oracle.com/technetwork/java/javase/downloads/java-archive-downloads-javase6-419409.html.
With that, it does not work, you get errors like:
org.postgresql.util.PSQLException: SSL error:
java.lang.RuntimeException: Could not generate DH keypair
...
Caused by: java.security.InvalidAlgorithmParameterException: Prime size
must be multiple of 64, and can only range from 512 to 1024 (inclusive)
So, the last binary version downloadable from Oracle is affected, but
recent versions of OpenJDK 6 work.
Rebased patch attached, with proposed release notes included. Barring
new objections or arguments, I'll commit this (only) to v10 later today.
- Heikki
Attachments:
0001-Always-use-2048-bit-DH-parameters-for-OpenSSL-epheme.patchtext/x-patch; name=0001-Always-use-2048-bit-DH-parameters-for-OpenSSL-epheme.patchDownload
From 93ef6ce1c203028384cf9febf3b4add715fff26f Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 31 Jul 2017 13:39:01 +0300
Subject: [PATCH 1/2] Always use 2048 bit DH parameters for OpenSSL ephemeral
DH ciphers.
1024 bits is considered weak these days, but OpenSSL always passes 1024 as
the key length to the tmp_dh callback. All the code to handle other key
lengths is, in fact, dead.
To remedy those issues:
* Only include hard-coded 2048-bit parameters.
* Set the parameters directly with SSL_CTX_set_tmp_dh(), without the
callback
* The name of the file containing the DH parameters is now a GUC. This
replaces the old hardcoded "dh1024.pem" filename. (The files for other
key lengths, dh512.pem, dh2048.pem, etc. were never actually used.)
Per report by Nicolas Guini and Damian Quiroga. Reviewed by Michael Paquier.
Discussion: https://www.postgresql.org/message-id/CAMxBoUyjOOautVozN6ofzym828aNrDjuCcOTcCquxjwS-L2hGQ@mail.gmail.com
---
doc/src/sgml/config.sgml | 24 +++
src/backend/libpq/be-secure-openssl.c | 264 +++++++++-----------------
src/backend/libpq/be-secure.c | 1 +
src/backend/utils/misc/guc.c | 11 ++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/libpq/libpq.h | 1 +
6 files changed, 133 insertions(+), 169 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b45b7f7f69..c33d6a0349 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1203,6 +1203,30 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-ssl-dh-params-file" xreflabel="ssl_dh_params_file">
+ <term><varname>ssl_dh_params_file</varname> (<type>string</type>)
+ <indexterm>
+ <primary><varname>ssl_dh_params_file</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specifies the name of the file containing Diffie-Hellman parameters
+ used for so-called ephemeral DH family of SSL ciphers. The default is
+ empty, in which case compiled-in default DH parameters used. Using
+ custom DH parameters reduces the exposure if an attacker manages to
+ crack the well-known compiled-in DH parameters. You can create your own
+ DH parameters file with the command
+ <command>openssl dhparam -out dhparams.pem 2048</command>.
+ </para>
+
+ <para>
+ This parameter can only be set in the <filename>postgresql.conf</>
+ file or on the server command line.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-krb-server-keyfile" xreflabel="krb_server_keyfile">
<term><varname>krb_server_keyfile</varname> (<type>string</type>)
<indexterm>
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 67145e9412..dc307c101f 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -61,6 +61,7 @@
#include "libpq/libpq.h"
#include "miscadmin.h"
#include "pgstat.h"
+#include "storage/fd.h"
#include "storage/latch.h"
#include "tcop/tcopprot.h"
#include "utils/memutils.h"
@@ -71,13 +72,12 @@ static int my_sock_write(BIO *h, const char *buf, int size);
static BIO_METHOD *my_BIO_s_socket(void);
static int my_SSL_set_fd(Port *port, int fd);
-static DH *load_dh_file(int keylength);
+static DH *load_dh_file(char *filename, bool isServerStart);
static DH *load_dh_buffer(const char *, size_t);
-static DH *generate_dh_parameters(int prime_len, int generator);
-static DH *tmp_dh_cb(SSL *s, int is_export, int keylength);
static int ssl_passwd_cb(char *buf, int size, int rwflag, void *userdata);
static int verify_cb(int, X509_STORE_CTX *);
static void info_cb(const SSL *ssl, int type, int args);
+static bool initialize_dh(SSL_CTX *context, bool isServerStart);
static bool initialize_ecdh(SSL_CTX *context, bool isServerStart);
static const char *SSLerrmessage(unsigned long ecode);
@@ -96,17 +96,14 @@ static bool ssl_passwd_cb_called = false;
* As discussed above, EDH protects the confidentiality of
* sessions even if the static private key is compromised,
* so we are *highly* motivated to ensure that we can use
- * EDH even if the DBA... or an attacker... deletes the
- * $DataDir/dh*.pem files.
+ * EDH even if the DBA has not provided custom DH parameters.
*
* We could refuse SSL connections unless a good DH parameter
* file exists, but some clients may quietly renegotiate an
* unsecured connection without fully informing the user.
- * Very uncool.
- *
- * Alternatively, the backend could attempt to load these files
- * on startup if SSL is enabled - and refuse to start if any
- * do not exist - but this would tend to piss off DBAs.
+ * Very uncool. Alternatively, the system could refuse to start
+ * if a DH parameters is not specified, but this would tend to
+ * piss off DBAs.
*
* If you want to create your own hardcoded DH parameters
* for fun and profit, review "Assigned Number for SKIP
@@ -114,19 +111,6 @@ static bool ssl_passwd_cb_called = false;
* for suggestions.
*/
-static const char file_dh512[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MEYCQQD1Kv884bEpQBgRjXyEpwpy1obEAxnIByl6ypUM2Zafq9AKUJsCRtMIPWak\n\
-XUGfnHy9iUsiGSa6q6Jew1XpKgVfAgEC\n\
------END DH PARAMETERS-----\n";
-
-static const char file_dh1024[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MIGHAoGBAPSI/VhOSdvNILSd5JEHNmszbDgNRR0PfIizHHxbLY7288kjwEPwpVsY\n\
-jY67VYy4XTjTNP18F1dDox0YbN4zISy1Kv884bEpQBgRjXyEpwpy1obEAxnIByl6\n\
-ypUM2Zafq9AKUJsCRtMIPWakXUGfnHy9iUsiGSa6q6Jew1XpL3jHAgEC\n\
------END DH PARAMETERS-----\n";
-
static const char file_dh2048[] =
"-----BEGIN DH PARAMETERS-----\n\
MIIBCAKCAQEA9kJXtwh/CBdyorrWqULzBej5UxE5T7bxbrlLOCDaAadWoxTpj0BV\n\
@@ -137,21 +121,6 @@ Q6MdGGzeMyEstSr/POGxKUAYEY18hKcKctaGxAMZyAcpesqVDNmWn6vQClCbAkbT\n\
CD1mpF1Bn5x8vYlLIhkmuquiXsNV6TILOwIBAg==\n\
-----END DH PARAMETERS-----\n";
-static const char file_dh4096[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MIICCAKCAgEA+hRyUsFN4VpJ1O8JLcCo/VWr19k3BCgJ4uk+d+KhehjdRqNDNyOQ\n\
-l/MOyQNQfWXPeGKmOmIig6Ev/nm6Nf9Z2B1h3R4hExf+zTiHnvVPeRBhjdQi81rt\n\
-Xeoh6TNrSBIKIHfUJWBh3va0TxxjQIs6IZOLeVNRLMqzeylWqMf49HsIXqbcokUS\n\
-Vt1BkvLdW48j8PPv5DsKRN3tloTxqDJGo9tKvj1Fuk74A+Xda1kNhB7KFlqMyN98\n\
-VETEJ6c7KpfOo30mnK30wqw3S8OtaIR/maYX72tGOno2ehFDkq3pnPtEbD2CScxc\n\
-alJC+EL7RPk5c/tgeTvCngvc1KZn92Y//EI7G9tPZtylj2b56sHtMftIoYJ9+ODM\n\
-sccD5Piz/rejE3Ome8EOOceUSCYAhXn8b3qvxVI1ddd1pED6FHRhFvLrZxFvBEM9\n\
-ERRMp5QqOaHJkM+Dxv8Cj6MqrCbfC4u+ZErxodzuusgDgvZiLF22uxMZbobFWyte\n\
-OvOzKGtwcTqO/1wV5gKkzu1ZVswVUQd5Gg8lJicwqRWyyNRczDDoG9jVDxmogKTH\n\
-AaqLulO7R8Ifa1SwF2DteSGVtgWEN8gDpN3RBmmPTDngyF2DHb5qmpnznwtFKdTL\n\
-KWbuHn491xNO25CQWMtem80uKw+pTnisBRF/454n1Jnhub144YRBoN8CAQI=\n\
------END DH PARAMETERS-----\n";
-
/* ------------------------------------------------------------ */
/* Public interface */
@@ -316,13 +285,14 @@ be_tls_init(bool isServerStart)
goto error;
}
- /* set up ephemeral DH keys, and disallow SSL v2/v3 while at it */
- SSL_CTX_set_tmp_dh_callback(context, tmp_dh_cb);
+ /* disallow SSL v2/v3 */
SSL_CTX_set_options(context,
SSL_OP_SINGLE_DH_USE |
SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
- /* set up ephemeral ECDH keys */
+ /* set up ephemeral DH and ECDH keys */
+ if (!initialize_dh(context, isServerStart))
+ goto error;
if (!initialize_ecdh(context, isServerStart))
goto error;
@@ -918,53 +888,57 @@ err:
* what we expect it to contain.
*/
static DH *
-load_dh_file(int keylength)
+load_dh_file(char *filename, bool isServerStart)
{
FILE *fp;
- char fnbuf[MAXPGPATH];
DH *dh = NULL;
int codes;
/* attempt to open file. It's not an error if it doesn't exist. */
- snprintf(fnbuf, sizeof(fnbuf), "dh%d.pem", keylength);
- if ((fp = fopen(fnbuf, "r")) == NULL)
+ if ((fp = AllocateFile(filename, "r")) == NULL)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode_for_file_access(),
+ errmsg("could not open DH parameters file \"%s\": %m",
+ filename)));
return NULL;
+ }
-/* flock(fileno(fp), LOCK_SH); */
dh = PEM_read_DHparams(fp, NULL, NULL, NULL);
-/* flock(fileno(fp), LOCK_UN); */
- fclose(fp);
+ FreeFile(fp);
- /* is the prime the correct size? */
- if (dh != NULL && 8 * DH_size(dh) < keylength)
+ if (dh == NULL)
{
- elog(LOG, "DH errors (%s): %d bits expected, %d bits found",
- fnbuf, keylength, 8 * DH_size(dh));
- dh = NULL;
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("could not load DH parameters file: %s",
+ SSLerrmessage(ERR_get_error()))));
+ return NULL;
}
/* make sure the DH parameters are usable */
- if (dh != NULL)
+ if (DH_check(dh, &codes) == 0)
{
- if (DH_check(dh, &codes) == 0)
- {
- elog(LOG, "DH_check error (%s): %s", fnbuf,
- SSLerrmessage(ERR_get_error()));
- return NULL;
- }
- if (codes & DH_CHECK_P_NOT_PRIME)
- {
- elog(LOG, "DH error (%s): p is not prime", fnbuf);
- return NULL;
- }
- if ((codes & DH_NOT_SUITABLE_GENERATOR) &&
- (codes & DH_CHECK_P_NOT_SAFE_PRIME))
- {
- elog(LOG,
- "DH error (%s): neither suitable generator or safe prime",
- fnbuf);
- return NULL;
- }
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("invalid DH parameters: %s",
+ SSLerrmessage(ERR_get_error()))));
+ return NULL;
+ }
+ if (codes & DH_CHECK_P_NOT_PRIME)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("invalid DH parameters: p is not prime")));
+ return NULL;
+ }
+ if ((codes & DH_NOT_SUITABLE_GENERATOR) &&
+ (codes & DH_CHECK_P_NOT_SAFE_PRIME))
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("invalid DH parameters: neither suitable generator or safe prime")));
+ return NULL;
}
return dh;
@@ -996,102 +970,6 @@ load_dh_buffer(const char *buffer, size_t len)
}
/*
- * Generate DH parameters.
- *
- * Last resort if we can't load precomputed nor hardcoded
- * parameters.
- */
-static DH *
-generate_dh_parameters(int prime_len, int generator)
-{
- DH *dh;
-
- if ((dh = DH_new()) == NULL)
- return NULL;
-
- if (DH_generate_parameters_ex(dh, prime_len, generator, NULL))
- return dh;
-
- DH_free(dh);
- return NULL;
-}
-
-/*
- * Generate an ephemeral DH key. Because this can take a long
- * time to compute, we can use precomputed parameters of the
- * common key sizes.
- *
- * Since few sites will bother to precompute these parameter
- * files, we also provide a fallback to the parameters provided
- * by the OpenSSL project.
- *
- * These values can be static (once loaded or computed) since
- * the OpenSSL library can efficiently generate random keys from
- * the information provided.
- */
-static DH *
-tmp_dh_cb(SSL *s, int is_export, int keylength)
-{
- DH *r = NULL;
- static DH *dh = NULL;
- static DH *dh512 = NULL;
- static DH *dh1024 = NULL;
- static DH *dh2048 = NULL;
- static DH *dh4096 = NULL;
-
- switch (keylength)
- {
- case 512:
- if (dh512 == NULL)
- dh512 = load_dh_file(keylength);
- if (dh512 == NULL)
- dh512 = load_dh_buffer(file_dh512, sizeof file_dh512);
- r = dh512;
- break;
-
- case 1024:
- if (dh1024 == NULL)
- dh1024 = load_dh_file(keylength);
- if (dh1024 == NULL)
- dh1024 = load_dh_buffer(file_dh1024, sizeof file_dh1024);
- r = dh1024;
- break;
-
- case 2048:
- if (dh2048 == NULL)
- dh2048 = load_dh_file(keylength);
- if (dh2048 == NULL)
- dh2048 = load_dh_buffer(file_dh2048, sizeof file_dh2048);
- r = dh2048;
- break;
-
- case 4096:
- if (dh4096 == NULL)
- dh4096 = load_dh_file(keylength);
- if (dh4096 == NULL)
- dh4096 = load_dh_buffer(file_dh4096, sizeof file_dh4096);
- r = dh4096;
- break;
-
- default:
- if (dh == NULL)
- dh = load_dh_file(keylength);
- r = dh;
- }
-
- /* this may take a long time, but it may be necessary... */
- if (r == NULL || 8 * DH_size(r) < keylength)
- {
- ereport(DEBUG2,
- (errmsg_internal("DH: generating parameters (%d bits)",
- keylength)));
- r = generate_dh_parameters(keylength, DH_GENERATOR_2);
- }
-
- return r;
-}
-
-/*
* Passphrase collection callback
*
* If OpenSSL is told to use a passphrase-protected server key, by default
@@ -1172,6 +1050,54 @@ info_cb(const SSL *ssl, int type, int args)
}
}
+/*
+ * Set DH parameters for generating ephemeral DH keys. The
+ * DH parameters can take a long time to compute, so they must be
+ * precomputed.
+ *
+ * Since few sites will bother to create a parameter file, we also
+ * also provide a fallback to the parameters provided by the
+ * OpenSSL project.
+ *
+ * These values can be static (once loaded or computed) since the
+ * OpenSSL library can efficiently generate random keys from the
+ * information provided.
+ */
+static bool
+initialize_dh(SSL_CTX *context, bool isServerStart)
+{
+ DH *dh = NULL;
+
+ SSL_CTX_set_options(context, SSL_OP_SINGLE_DH_USE);
+
+ if (ssl_dh_params_file[0])
+ dh = load_dh_file(ssl_dh_params_file, isServerStart);
+ if (!dh)
+ dh = load_dh_buffer(file_dh2048, sizeof file_dh2048);
+ if (!dh)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ (errmsg("DH: could not load DH parameters"))));
+ return false;
+ }
+
+ if (SSL_CTX_set_tmp_dh(context, dh) != 1)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ (errmsg("DH: could not set DH parameters: %s",
+ SSLerrmessage(ERR_get_error())))));
+ return false;
+ }
+ return true;
+}
+
+/*
+ * Set ECDH parameters for generating ephemeral Elliptic Curve DH
+ * keys. This is much simpler than the DH parameters, as we just
+ * need to provide the name of the curve to OpenSSL.
+ */
static bool
initialize_ecdh(SSL_CTX *context, bool isServerStart)
{
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 785dadb6c2..53fefd1b29 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -44,6 +44,7 @@ char *ssl_cert_file;
char *ssl_key_file;
char *ssl_ca_file;
char *ssl_crl_file;
+char *ssl_dh_params_file;
#ifdef USE_SSL
bool ssl_loaded_verify_locations = false;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 82e54c084b..246fea8693 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3607,6 +3607,17 @@ static struct config_string ConfigureNamesString[] =
},
{
+ {"ssl_dh_params_file", PGC_SIGHUP, CONN_AUTH_SECURITY,
+ gettext_noop("Location of the SSL DH params file."),
+ NULL,
+ GUC_SUPERUSER_ONLY
+ },
+ &ssl_dh_params_file,
+ "",
+ NULL, NULL, NULL
+ },
+
+ {
{"application_name", PGC_USERSET, LOGGING_WHAT,
gettext_noop("Sets the application name to be reported in statistics and logs."),
NULL,
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 1906b5a33c..df5d2f3f22 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -80,6 +80,7 @@
#ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
#ssl_prefer_server_ciphers = on
#ssl_ecdh_curve = 'prime256v1'
+#ssl_dh_params_file = ''
#ssl_cert_file = 'server.crt'
#ssl_key_file = 'server.key'
#ssl_ca_file = ''
diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index 78851b1060..fd2dd5853c 100644
--- a/src/include/libpq/libpq.h
+++ b/src/include/libpq/libpq.h
@@ -79,6 +79,7 @@ extern char *ssl_cert_file;
extern char *ssl_key_file;
extern char *ssl_ca_file;
extern char *ssl_crl_file;
+extern char *ssl_dh_params_file;
extern int secure_initialize(bool isServerStart);
extern bool secure_loaded_verify_locations(void);
--
2.11.0
0002-Update-release-notes-for-the-DH-parameter-changes.patchtext/x-patch; name=0002-Update-release-notes-for-the-DH-parameter-changes.patchDownload
From af22f1ed09fcce06c20d39df9bb9f1478e5ddabc Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 31 Jul 2017 14:23:17 +0300
Subject: [PATCH 2/2] Update release notes for the DH parameter changes.
---
doc/src/sgml/release-10.sgml | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/doc/src/sgml/release-10.sgml b/doc/src/sgml/release-10.sgml
index cf743aa2f7..b5ee88984e 100644
--- a/doc/src/sgml/release-10.sgml
+++ b/doc/src/sgml/release-10.sgml
@@ -408,6 +408,43 @@
<listitem>
<!--
+2017-07-31 [XXXXXXXXX] Always use 2048 bit DH parameters for OpenSSL ephemeral
+-->
+ <para>
+ Add configuration option <xref linkend="guc-ssl-dh-params-file"> to
+ specify filename for custom OpenSSL DH parameters (Heikki Linnakangas)
+ </para>
+
+ <para>
+ This replaces the hardcoded, undocumented <filename>dh1024.pem</>
+ filename. Note that <filename>dh1024.pem</> is no longer used by default;
+ you must set the option to use custom DH parameters.
+ </para>
+ </listitem>
+
+ <listitem>
+<!--
+2017-07-31 [XXXXXXXXX] Always use 2048 bit DH parameters for OpenSSL ephemeral
+-->
+ <para>
+ Increase the size of DH parameters used for OpenSSL ephemeral DH ciphers
+ to 2048 bits (Heikki Linnakangas)
+ </para>
+
+ <para>
+ The size of the compiled-in DH parameters has been increased from 1024
+ to 2048 bits, making DH key exchange more resistent to a brute-force
+ attack. However, some old SSL implementations, notably some revisions of
+ Java Runtime Environment version 6, will not accept DH parameters longer
+ than 1024 bits, and will not be able to connect over SSL. As a
+ work-around, you can use custom 1024-bit DH parameters, instead of the
+ compiled-in defaults. See <xref linkend="guc-ssl-dh-params-file"> for
+ information on using custom DH parameters.
+ </para>
+ </listitem>
+
+ <listitem>
+<!--
2017-02-13 [7ada2d31f] Remove contrib/tsearch2.
-->
<para>
--
2.11.0
On 07/31/2017 02:27 PM, Heikki Linnakangas wrote:
Rebased patch attached, with proposed release notes included. Barring
new objections or arguments, I'll commit this (only) to v10 later today.
Ok, committed for v10. Thanks Nicolas and Damien, and everyone else
involved!
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers