Always enable ecdh_auto.

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

Change-Id: Iaabc82de766cd00968844a71aaac29bd59841cd4
Reviewed-on: https://boringssl-review.googlesource.com/4531
Reviewed-by: Adam Langley <agl@google.com>

include/openssl/ssl.h

@@ -1650,7 +1650,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
@@ -1711,6 +1710,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)
 
@@ -1719,6 +1724,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)
 
@@ -1809,10 +1820,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)
@@ -2201,10 +2208,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);
@@ -2291,6 +2322,12 @@ OPENSSL_EXPORT const SSL_METHOD *DTLSv1_client_method(void);
 OPENSSL_EXPORT const SSL_METHOD *DTLSv1_2_server_method(void);
 OPENSSL_EXPORT const SSL_METHOD *DTLSv1_2_client_method(void);
 
+/* 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.
  *

ssl/internal.h

@@ -454,13 +454,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];

ssl/s3_lib.c

@@ -676,7 +676,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;
@@ -766,10 +766,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);
 
@@ -945,7 +941,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;
@@ -993,10 +989,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);
 

ssl/s3_srvr.c

@@ -1410,9 +1410,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
@@ -1421,6 +1419,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;

ssl/ssl_cert.c

@@ -216,7 +216,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;

ssl/ssl_lib.c

@@ -2004,7 +2004,6 @@ void ssl_get_compatible_server_ciphers(SSL *s, uint32_t *out_mask_k,
   int rsa_enc, rsa_sign, dh_tmp;
   uint32_t mask_k, mask_a;
   int have_ecc_cert, ecdsa_ok;
-  int have_ecdh_tmp;
   X509 *x;
 
   if (c == NULL) {
@@ -2016,8 +2015,6 @@ void ssl_get_compatible_server_ciphers(SSL *s, uint32_t *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);
@@ -2053,7 +2050,7 @@ void ssl_get_compatible_server_ciphers(SSL *s, uint32_t *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;
   }
 
@@ -2625,15 +2622,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) {

ssl/t1_lib.c

@@ -633,22 +633,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

ssl/test/bssl_shim.cc

@@ -413,10 +413,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;
   }