From ffaef3f1526ed87a46f82fa4924d5b08f2a2e631 Mon Sep 17 00:00:00 2001 From: "Dr. Stephen Henson" Date: Thu, 17 Dec 2015 02:57:20 +0000 Subject: [PATCH] Always generate DH keys for ephemeral DH cipher suites. Reviewed-by: Matt Caswell --- doc/ssl/SSL_CTX_set_tmp_dh_callback.pod | 29 +++++-------------------- include/openssl/ssl.h | 4 ++-- ssl/s3_lib.c | 17 ++++----------- ssl/statem/statem_srvr.c | 17 +++------------ 4 files changed, 14 insertions(+), 53 deletions(-) diff --git a/doc/ssl/SSL_CTX_set_tmp_dh_callback.pod b/doc/ssl/SSL_CTX_set_tmp_dh_callback.pod index 3e08b88203..57bf211075 100644 --- a/doc/ssl/SSL_CTX_set_tmp_dh_callback.pod +++ b/doc/ssl/SSL_CTX_set_tmp_dh_callback.pod @@ -48,25 +48,8 @@ even if he gets hold of the normal (certified) key, as this key was only used for signing. In order to perform a DH key exchange the server must use a DH group -(DH parameters) and generate a DH key. -The server will always generate a new DH key during the negotiation -if either the DH parameters are supplied via callback or the -SSL_OP_SINGLE_DH_USE option of SSL_CTX_set_options(3) is set (or both). -It will immediately create a DH key if DH parameters are supplied via -SSL_CTX_set_tmp_dh() and SSL_OP_SINGLE_DH_USE is not set. -In this case, -it may happen that a key is generated on initialization without later -being needed, while on the other hand the computer time during the -negotiation is being saved. - -If "strong" primes were used to generate the DH parameters, it is not strictly -necessary to generate a new key for each handshake but it does improve forward -secrecy. If it is not assured that "strong" primes were used, -SSL_OP_SINGLE_DH_USE must be used in order to prevent small subgroup -attacks. Always using SSL_OP_SINGLE_DH_USE has an impact on the -computer time needed during negotiation, but it is not very large, so -application authors/users should consider always enabling this option. -The option is required to implement perfect forward secrecy (PFS). +(DH parameters) and generate a DH key. The server will always generate +a new DH key during the negotiation. As generating DH parameters is extremely time consuming, an application should not generate the parameters on the fly but supply the parameters. @@ -92,10 +75,9 @@ can supply the DH parameters via a callback function. Previous versions of the callback used B and B parameters to control parameter generation for export and non-export cipher suites. Modern servers that do not support export ciphersuites -are advised to either use SSL_CTX_set_tmp_dh() in combination with -SSL_OP_SINGLE_DH_USE, or alternatively, use the callback but ignore -B and B and simply supply at least 2048-bit -parameters in the callback. +are advised to either use SSL_CTX_set_tmp_dh() or alternatively, use +the callback but ignore B and B and simply +supply at least 2048-bit parameters in the callback. =head1 EXAMPLES @@ -127,7 +109,6 @@ partly left out.) if (SSL_CTX_set_tmp_dh(ctx, dh_2048) != 1) { /* Error. */ } - SSL_CTX_set_options(ctx, SSL_OP_SINGLE_DH_USE); ... =head1 RETURN VALUES diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index e841360e17..307af48d64 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -411,8 +411,8 @@ typedef int (*custom_ext_parse_cb) (SSL *s, unsigned int ext_type, # define SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION 0x00040000U /* Does nothing: retained for compatibility */ # define SSL_OP_SINGLE_ECDH_USE 0x0 -/* If set, always create a new key when using tmp_dh parameters */ -# define SSL_OP_SINGLE_DH_USE 0x00100000U +/* Does nothing: retained for compatibility */ +# define SSL_OP_SINGLE_DH_USE 0x0 /* Does nothing: retained for compatibiity */ # define SSL_OP_EPHEMERAL_RSA 0x0 /* diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 4fc4426cd9..f7cdd93bb1 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -3499,13 +3499,6 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg) SSLerr(SSL_F_SSL3_CTRL, ERR_R_DH_LIB); return (ret); } - if (!(s->options & SSL_OP_SINGLE_DH_USE)) { - if (!DH_generate_key(dh)) { - DH_free(dh); - SSLerr(SSL_F_SSL3_CTRL, ERR_R_DH_LIB); - return (ret); - } - } DH_free(s->cert->dh_tmp); s->cert->dh_tmp = dh; ret = 1; @@ -3887,12 +3880,10 @@ long ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, long larg, void *parg) SSLerr(SSL_F_SSL3_CTX_CTRL, ERR_R_DH_LIB); return 0; } - if (!(ctx->options & SSL_OP_SINGLE_DH_USE)) { - if (!DH_generate_key(new)) { - SSLerr(SSL_F_SSL3_CTX_CTRL, ERR_R_DH_LIB); - DH_free(new); - return 0; - } + if (!DH_generate_key(new)) { + SSLerr(SSL_F_SSL3_CTX_CTRL, ERR_R_DH_LIB); + DH_free(new); + return 0; } DH_free(cert->dh_tmp); cert->dh_tmp = new; diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index b8b18b74e9..abdff176f7 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1800,20 +1800,9 @@ int tls_construct_server_key_exchange(SSL *s) } s->s3->tmp.dh = dh; - if ((dhp->pub_key == NULL || - dhp->priv_key == NULL || - (s->options & SSL_OP_SINGLE_DH_USE))) { - if (!DH_generate_key(dh)) { - SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, ERR_R_DH_LIB); - goto err; - } - } else { - dh->pub_key = BN_dup(dhp->pub_key); - dh->priv_key = BN_dup(dhp->priv_key); - if ((dh->pub_key == NULL) || (dh->priv_key == NULL)) { - SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, ERR_R_DH_LIB); - goto err; - } + if (!DH_generate_key(dh)) { + SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, ERR_R_DH_LIB); + goto err; } r[0] = dh->p; r[1] = dh->g; -- GitLab