Always enable ecdh_auto.chromium-2357

This is a really dumb API wart. Now that we have a limited set of curves that
are all reasonable, the automatic logic should just always kick in. This makes
set_ecdh_auto a no-op and, instead of making it the first choice, uses it as
the fallback behavior should none of the older curve selection APIs be used.

Currently, by default, server sockets can only use the plain RSA key exchange.

BUG=481139

(cherry picked from commit dd978784d7cc2564d6916135cca94a2e3a62ed5c)

Change-Id: I2ef653236c55df0d990c01cfdcaedbd757062e46

8 files changed, 75 insertions, 48 deletions

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 563da67..b086c41 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1595,7 +1595,6 @@ DECLARE_PEM_rw(SSL_SESSION, SSL_SESSION)
 #define SSL_CTRL_GET_CURVES 90
 #define SSL_CTRL_SET_CURVES 91
 #define SSL_CTRL_SET_CURVES_LIST 92
-#define SSL_CTRL_SET_ECDH_AUTO 94
 #define SSL_CTRL_SET_SIGALGS 97
 #define SSL_CTRL_SET_SIGALGS_LIST 98
 #define SSL_CTRL_CERT_FLAGS 99
@@ -1656,6 +1655,12 @@ DECLARE_PEM_rw(SSL_SESSION, SSL_SESSION)
   SSL_CTX_ctrl(ctx, SSL_CTRL_SET_TMP_RSA, 0, (char *)rsa)
 #define SSL_CTX_set_tmp_dh(ctx, dh) \
   SSL_CTX_ctrl(ctx, SSL_CTRL_SET_TMP_DH, 0, (char *)dh)
+
+/* SSL_CTX_set_tmp_ecdh configures |ctx| to use the curve from |ecdh| (a const
+ * EC_KEY *) as the curve for ephemeral ECDH keys. For historical reasons, this
+ * API expects an |EC_KEY|, but only the curve is used. It returns one on
+ * success and zero on error. If unset, an appropriate curve will be chosen
+ * automatically. (This is recommended.) */
 #define SSL_CTX_set_tmp_ecdh(ctx, ecdh) \
   SSL_CTX_ctrl(ctx, SSL_CTRL_SET_TMP_ECDH, 0, (char *)ecdh)
 
@@ -1664,6 +1669,12 @@ DECLARE_PEM_rw(SSL_SESSION, SSL_SESSION)
   SSL_ctrl(ssl, SSL_CTRL_SET_TMP_RSA, 0, (char *)rsa)
 #define SSL_set_tmp_dh(ssl, dh) \
   SSL_ctrl(ssl, SSL_CTRL_SET_TMP_DH, 0, (char *)dh)
+
+/* SSL_set_tmp_ecdh configures |ssl| to use the curve from |ecdh| (a const
+ * EC_KEY *) as the curve for ephemeral ECDH keys. For historical reasons, this
+ * API expects an |EC_KEY|, but only the curve is used. It returns one on
+ * success and zero on error. If unset, an appropriate curve will be chosen
+ * automatically. (This is recommended.) */
 #define SSL_set_tmp_ecdh(ssl, ecdh) \
   SSL_ctrl(ssl, SSL_CTRL_SET_TMP_ECDH, 0, (char *)ecdh)
 
@@ -1754,10 +1765,6 @@ DECLARE_PEM_rw(SSL_SESSION, SSL_SESSION)
   SSL_ctrl(ctx, SSL_CTRL_SET_CURVES, clistlen, (char *)clist)
 #define SSL_set1_curves_list(ctx, s) \
   SSL_ctrl(ctx, SSL_CTRL_SET_CURVES_LIST, 0, (char *)s)
-#define SSL_CTX_set_ecdh_auto(ctx, onoff) \
-  SSL_CTX_ctrl(ctx, SSL_CTRL_SET_ECDH_AUTO, onoff, NULL)
-#define SSL_set_ecdh_auto(s, onoff) \
-  SSL_ctrl(s, SSL_CTRL_SET_ECDH_AUTO, onoff, NULL)
 
 #define SSL_CTX_set1_sigalgs(ctx, slist, slistlen) \
   SSL_CTX_ctrl(ctx, SSL_CTRL_SET_SIGALGS, slistlen, (int *)slist)
@@ -2207,10 +2214,34 @@ OPENSSL_EXPORT void SSL_CTX_set_tmp_dh_callback(
 OPENSSL_EXPORT void SSL_set_tmp_dh_callback(SSL *ssl,
                                             DH *(*dh)(SSL *ssl, int is_export,
                                                       int keylength));
+
+/* SSL_CTX_set_tmp_ecdh_callback configures |ctx| to use |callback| to determine
+ * the curve for ephemeral ECDH keys. |callback| should ignore |is_export| and
+ * |keylength| and return an |EC_KEY| of the selected curve or NULL on
+ * error. Only the curve is used, so the |EC_KEY| needn't have a generated
+ * keypair.
+ *
+ * If the callback is unset, an appropriate curve will be chosen automatically.
+ * (This is recommended.)
+ *
+ * WARNING: The caller does not take ownership of the resulting |EC_KEY|, so
+ * |callback| must save and release the object elsewhere. */
 OPENSSL_EXPORT void SSL_CTX_set_tmp_ecdh_callback(
-    SSL_CTX *ctx, EC_KEY *(*ecdh)(SSL *ssl, int is_export, int keylength));
+    SSL_CTX *ctx, EC_KEY *(*callback)(SSL *ssl, int is_export, int keylength));
+
+/* SSL_set_tmp_ecdh_callback configures |ssl| to use |callback| to determine the
+ * curve for ephemeral ECDH keys. |callback| should ignore |is_export| and
+ * |keylength| and return an |EC_KEY| of the selected curve or NULL on
+ * error. Only the curve is used, so the |EC_KEY| needn't have a generated
+ * keypair.
+ *
+ * If the callback is unset, an appropriate curve will be chosen automatically.
+ * (This is recommended.)
+ *
+ * WARNING: The caller does not take ownership of the resulting |EC_KEY|, so
+ * |callback| must save and release the object elsewhere. */
 OPENSSL_EXPORT void SSL_set_tmp_ecdh_callback(
-    SSL *ssl, EC_KEY *(*ecdh)(SSL *ssl, int is_export, int keylength));
+    SSL *ssl, EC_KEY *(*callback)(SSL *ssl, int is_export, int keylength));
 
 OPENSSL_EXPORT const void *SSL_get_current_compression(SSL *s);
 OPENSSL_EXPORT const void *SSL_get_current_expansion(SSL *s);
@@ -2248,6 +2279,15 @@ OPENSSL_EXPORT int SSL_get_rc4_state(const SSL *ssl, const RC4_KEY **read_key,
                                      const RC4_KEY **write_key);
 
 
+/* Deprecated functions. */
+
+/* SSL_CTX_set_ecdh_auto returns one. */
+#define SSL_CTX_set_ecdh_auto(ctx, onoff) 1
+
+/* SSL_set_ecdh_auto returns one. */
+#define SSL_set_ecdh_auto(ssl, onoff) 1
+
+
 /* Android compatibility section.
  *
  * These functions are declared, temporarily, for Android because
diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index fe0e760..41544ee 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -755,7 +755,7 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg) {
     case SSL_CTRL_SET_TMP_ECDH: {
       /* For historical reasons, this API expects an |EC_KEY|, but only the
        * group is used. */
-      EC_KEY *ec_key = (EC_KEY *)parg;
+      const EC_KEY *ec_key = (const EC_KEY *)parg;
       if (ec_key == NULL || EC_KEY_get0_group(ec_key) == NULL) {
         OPENSSL_PUT_ERROR(SSL, ssl3_ctrl, ERR_R_PASSED_NULL_PARAMETER);
         return ret;
@@ -845,10 +845,6 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg) {
       return tls1_set_curves(&s->tlsext_ellipticcurvelist,
                              &s->tlsext_ellipticcurvelist_length, parg, larg);
 
-    case SSL_CTRL_SET_ECDH_AUTO:
-      s->cert->ecdh_tmp_auto = larg;
-      return 1;
-
     case SSL_CTRL_SET_SIGALGS:
       return tls1_set_sigalgs(s->cert, parg, larg, 0);
 
@@ -1024,7 +1020,7 @@ long ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, long larg, void *parg) {
     case SSL_CTRL_SET_TMP_ECDH: {
       /* For historical reasons, this API expects an |EC_KEY|, but only the
        * group is used. */
-      EC_KEY *ec_key = (EC_KEY *)parg;
+      const EC_KEY *ec_key = (const EC_KEY *)parg;
       if (ec_key == NULL || EC_KEY_get0_group(ec_key) == NULL) {
         OPENSSL_PUT_ERROR(SSL, ssl3_ctx_ctrl, ERR_R_PASSED_NULL_PARAMETER);
         return 0;
@@ -1072,10 +1068,6 @@ long ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, long larg, void *parg) {
       return tls1_set_curves(&ctx->tlsext_ellipticcurvelist,
                              &ctx->tlsext_ellipticcurvelist_length, parg, larg);
 
-    case SSL_CTRL_SET_ECDH_AUTO:
-      ctx->cert->ecdh_tmp_auto = larg;
-      return 1;
-
     case SSL_CTRL_SET_SIGALGS:
       return tls1_set_sigalgs(ctx->cert, parg, larg, 0);
 
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 25482a2..98e667b 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -1417,9 +1417,7 @@ int ssl3_send_server_key_exchange(SSL *s) {
     } else if (alg_k & SSL_kECDHE) {
       /* Determine the curve to use. */
       int nid = NID_undef;
-      if (cert->ecdh_tmp_auto) {
-        nid = tls1_get_shared_curve(s);
-      } else if (cert->ecdh_nid != NID_undef) {
+      if (cert->ecdh_nid != NID_undef) {
         nid = cert->ecdh_nid;
       } else if (cert->ecdh_tmp_cb != NULL) {
         /* Note: |ecdh_tmp_cb| does NOT pass ownership of the result
@@ -1428,6 +1426,8 @@ int ssl3_send_server_key_exchange(SSL *s) {
         if (template != NULL && EC_KEY_get0_group(template) != NULL) {
           nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(template));
         }
+      } else {
+        nid = tls1_get_shared_curve(s);
       }
       if (nid == NID_undef) {
         al = SSL_AD_HANDSHAKE_FAILURE;
diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index 185c12b..58be2d5 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -214,7 +214,6 @@ CERT *ssl_cert_dup(CERT *cert) {
 
   ret->ecdh_nid = cert->ecdh_nid;
   ret->ecdh_tmp_cb = cert->ecdh_tmp_cb;
-  ret->ecdh_tmp_auto = cert->ecdh_tmp_auto;
 
   for (i = 0; i < SSL_PKEY_NUM; i++) {
     CERT_PKEY *cpk = cert->pkeys + i;
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index b21b521..fec2182 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1999,7 +1999,6 @@ void ssl_get_compatible_server_ciphers(SSL *s, unsigned long *out_mask_k,
   int rsa_enc, rsa_sign, dh_tmp;
   unsigned long mask_k, mask_a;
   int have_ecc_cert, ecdsa_ok;
-  int have_ecdh_tmp;
   X509 *x;
 
   if (c == NULL) {
@@ -2011,8 +2010,6 @@ void ssl_get_compatible_server_ciphers(SSL *s, unsigned long *out_mask_k,
 
   dh_tmp = (c->dh_tmp != NULL || c->dh_tmp_cb != NULL);
 
-  have_ecdh_tmp = (c->ecdh_nid != NID_undef || c->ecdh_tmp_cb != NULL ||
-                   c->ecdh_tmp_auto);
   rsa_enc = ssl_has_key(s, SSL_PKEY_RSA_ENC);
   rsa_sign = ssl_has_key(s, SSL_PKEY_RSA_SIGN);
   have_ecc_cert = ssl_has_key(s, SSL_PKEY_ECC);
@@ -2050,7 +2047,7 @@ void ssl_get_compatible_server_ciphers(SSL *s, unsigned long *out_mask_k,
 
   /* If we are considering an ECC cipher suite that uses an ephemeral EC
    * key, check it. */
-  if (have_ecdh_tmp && tls1_check_ec_tmp_key(s)) {
+  if (tls1_check_ec_tmp_key(s)) {
     mask_k |= SSL_kECDHE;
   }
 
@@ -2613,15 +2610,16 @@ void SSL_set_tmp_dh_callback(SSL *ssl, DH *(*dh)(SSL *ssl, int is_export,
 }
 
 void SSL_CTX_set_tmp_ecdh_callback(SSL_CTX *ctx,
-                                   EC_KEY *(*ecdh)(SSL *ssl, int is_export,
-                                                   int keylength)) {
-  SSL_CTX_callback_ctrl(ctx, SSL_CTRL_SET_TMP_ECDH_CB, (void (*)(void))ecdh);
+                                   EC_KEY *(*callback)(SSL *ssl, int is_export,
+                                                       int keylength)) {
+  SSL_CTX_callback_ctrl(ctx, SSL_CTRL_SET_TMP_ECDH_CB,
+                        (void (*)(void))callback);
 }
 
 void SSL_set_tmp_ecdh_callback(SSL *ssl,
-                               EC_KEY *(*ecdh)(SSL *ssl, int is_export,
-                                               int keylength)) {
-  SSL_callback_ctrl(ssl, SSL_CTRL_SET_TMP_ECDH_CB, (void (*)(void))ecdh);
+                               EC_KEY *(*callback)(SSL *ssl, int is_export,
+                                                   int keylength)) {
+  SSL_callback_ctrl(ssl, SSL_CTRL_SET_TMP_ECDH_CB, (void (*)(void))callback);
 }
 
 int SSL_CTX_use_psk_identity_hint(SSL_CTX *ctx, const char *identity_hint) {
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index ed62fab..cc4d48d 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -434,13 +434,15 @@ typedef struct cert_st {
 
   DH *dh_tmp;
   DH *(*dh_tmp_cb)(SSL *ssl, int is_export, int keysize);
+
   /* ecdh_nid, if not |NID_undef|, is the NID of the curve to use for ephemeral
-   * ECDH keys. */
+   * ECDH keys. If unset, |ecdh_tmp_cb| is consulted. */
   int ecdh_nid;
-  /* Callback for generating ephemeral ECDH keys */
+  /* ecdh_tmp_cb is a callback for selecting the curve to use for ephemeral ECDH
+   * keys. If NULL, a curve is selected automatically. See
+   * |SSL_CTX_set_tmp_ecdh_callback|. */
   EC_KEY *(*ecdh_tmp_cb)(SSL *ssl, int is_export, int keysize);
-  /* Select ECDH parameters automatically */
-  int ecdh_tmp_auto;
+
   /* Flags related to certificates */
   unsigned int cert_flags;
   CERT_PKEY pkeys[SSL_PKEY_NUM];
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 0009d13..11c5635 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -606,22 +606,22 @@ done:
 }
 
 int tls1_check_ec_tmp_key(SSL *s) {
-  if (s->cert->ecdh_tmp_auto) {
-    /* If using |ecdh_tmp_auto|, ECDH is acceptable if there is a shared
-     * curve. */
-    return tls1_get_shared_curve(s) != NID_undef;
-  }
-
   if (s->cert->ecdh_nid != NID_undef) {
-    /* If the curve is preconfigured, ECDH is acceptable if the peer supports
+    /* If the curve is preconfigured, ECDH is acceptable iff the peer supports
      * the curve. */
     uint16_t curve_id;
     return tls1_ec_nid2curve_id(&curve_id, s->cert->ecdh_nid) &&
            tls1_check_curve_id(s, curve_id);
   }
 
-  /* Otherwise, assume the callback will provide an acceptable curve. */
-  return s->cert->ecdh_tmp_cb != NULL;
+  if (s->cert->ecdh_tmp_cb != NULL) {
+    /* Assume the callback will provide an acceptable curve. */
+    return 1;
+  }
+
+  /* Otherwise, the curve gets selected automatically. ECDH is acceptable iff
+   * there is a shared curve. */
+  return tls1_get_shared_curve(s) != NID_undef;
 }
 
 /* List of supported signature algorithms and hashes. Should make this
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index d79074b..b59a719 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -394,10 +394,6 @@ static ScopedSSL_CTX SetupCtx(const TestConfig *config) {
     SSL_CTX_set_read_ahead(ssl_ctx.get(), 1);
   }
 
-  if (!SSL_CTX_set_ecdh_auto(ssl_ctx.get(), 1)) {
-    return nullptr;
-  }
-
   if (!SSL_CTX_set_cipher_list(ssl_ctx.get(), "ALL")) {
     return nullptr;
   }