Always generate DH keys for ephemeral DH cipher suites

Modified version of the commit ffaef3f15 in the master branch by Stephen
Henson. This makes the SSL_OP_SINGLE_DH_USE option a no-op and always
generates a new DH key for every handshake regardless.

This is a follow on from CVE-2016-0701. This branch is not impacted by
that CVE because it does not support X9.42 style parameters. It is still
possible to generate parameters based on primes that are not "safe",
although by default OpenSSL does not do this. The documentation does
sign post that using such parameters is unsafe if the private DH key is
reused. However to avoid accidental problems or future attacks this commit
has been backported to this branch.

Issue reported by Antonio Sanso

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>

diff --git a/doc/ssl/SSL_CTX_set_tmp_dh_callback.pod b/doc/ssl/SSL_CTX_set_tmp_dh_callback.pod
index b754c16a86e66eaeab66df876000ffd760dd7c70..234fbc845002f33bf188f1e0aff5c8b1975c01bf 100644 (file)
--- 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.
@@ -93,10 +76,9 @@ can supply the DH parameters via a callback function.
 Previous versions of the callback used B<is_export> and B<keylength>
 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<keylength> and B<is_export> 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<keylength> and B<is_export> and simply
+supply at least 2048-bit parameters in the callback.
 
 =head1 EXAMPLES
 
@@ -128,7 +110,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/ssl/s3_lib.c b/ssl/s3_lib.c
index d3d8221b661ab6fa4ce6c914f70d2432c93f0bc5..167e3cc9e4955d853ec064705a567a55e9d4b6cd 100644 (file)
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -3164,13 +3164,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);
-                }
-            }
             if (s->cert->dh_tmp != NULL)
                 DH_free(s->cert->dh_tmp);
             s->cert->dh_tmp = dh;
@@ -3482,13 +3475,6 @@ 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 (cert->dh_tmp != NULL)
                 DH_free(cert->dh_tmp);
             cert->dh_tmp = new;

diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 7eb7ea681eb0fd46fc417f66de9478a843f92d39..04cf93a0f7c683e694f9ec99ec9b061266869497 100644 (file)
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -1729,20 +1729,9 @@ int ssl3_send_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_SSL3_SEND_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_SSL3_SEND_SERVER_KEY_EXCHANGE, ERR_R_DH_LIB);
-                    goto err;
-                }
+            if (!DH_generate_key(dh)) {
+                SSLerr(SSL_F_SSL3_SEND_SERVER_KEY_EXCHANGE, ERR_R_DH_LIB);
+                goto err;
             }
             r[0] = dh->p;
             r[1] = dh->g;

diff --git a/ssl/ssl.h b/ssl/ssl.h
index b8456c692bd946217ec19a6fa2e00cd645b0954a..105047ee4d5e7e5c87248b91f310b2715978a0b4 100644 (file)
--- a/ssl/ssl.h
+++ b/ssl/ssl.h
@@ -602,7 +602,7 @@ struct ssl_session_st {
 # define SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION        0x00040000L
 /* If set, always create a new key when using tmp_ecdh parameters */
 # define SSL_OP_SINGLE_ECDH_USE                          0x00080000L
-/* If set, always create a new key when using tmp_dh parameters */
+/* Does nothing: retained for compatibility */
 # define SSL_OP_SINGLE_DH_USE                            0x00100000L
 /* Does nothing: retained for compatibiity */
 # define SSL_OP_EPHEMERAL_RSA                            0x0