(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-763f8e410...@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
>From 481e019441dadc0826e6e9b7d4a56c49e63c654b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@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-l2...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
