Only predict X25519 in TLS 1.3.

We'd previously been assuming we'd want to predict P-256 and X25519 but,
on reflection, that's nonsense. Although, today, P-256 is widespread and
X25519 is less so, that's not the right question to ask. Those servers
are all 1.2.

The right question is whether we believe enough servers will get to TLS
1.3 before X25519 to justify wasting 64 bytes on all other connections.
Given that OpenSSL has already shipped X25519 and Microsoft was doing
interop testing on X25519 around when we were shipping it, I think the
answer is no.

Moreover, if we are wrong, it will be easier to go from predicting one
group to two rather than the inverse (provided we send a fake one with
GREASE). I anticipate prediction-miss HelloRetryRequest logic across the
TLS/TCP ecosystem will be largely untested (no one wants to pay an RTT),
so taking a group out of the predicted set will likely be a risky
operation.

Only predicting one group also makes things a bit simpler. I haven't
done this here, but we'll be able to fold the 1.2 and 1.3 ecdh_ctx's
together, even.

Change-Id: Ie7e42d3105aca48eb9d97e2e05a16c5379aa66a3
Reviewed-on: https://boringssl-review.googlesource.com/10960
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

6 files changed, 34 insertions, 69 deletions

diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index f94156f7..e19b347b 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -88,6 +88,7 @@ SSL,262,NO_CIPHERS_SPECIFIED
 SSL,177,NO_CIPHER_MATCH
 SSL,253,NO_COMMON_SIGNATURE_ALGORITHMS
 SSL,178,NO_COMPRESSION_SPECIFIED
+SSL,265,NO_GROUPS_SPECIFIED
 SSL,179,NO_METHOD_SPECIFIED
 SSL,180,NO_P256_SUPPORT
 SSL,181,NO_PRIVATE_KEY_ASSIGNED
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index d7e5add9..d629b8f3 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4777,6 +4777,7 @@ BORINGSSL_MAKE_DELETER(SSL_SESSION, SSL_SESSION_free)
 #define SSL_R_NO_CIPHERS_SPECIFIED 262
 #define SSL_R_RENEGOTIATION_EMS_MISMATCH 263
 #define SSL_R_DUPLICATE_KEY_SHARE 264
+#define SSL_R_NO_GROUPS_SPECIFIED 265
 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020
diff --git a/ssl/internal.h b/ssl/internal.h
index eff56723..232364ee 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -896,9 +896,8 @@ struct ssl_handshake_st {
   uint8_t secret[EVP_MAX_MD_SIZE];
   uint8_t traffic_secret_0[EVP_MAX_MD_SIZE];
 
-  /* groups is the set of active ECDH offers from the client in TLS 1.3. */
-  SSL_ECDH_CTX *groups;
-  size_t groups_len;
+  /* ecdh_ctx is the active client ECDH offer in TLS 1.3. */
+  SSL_ECDH_CTX ecdh_ctx;
 
   /* retry_group is the group ID selected by the server in HelloRetryRequest in
    * TLS 1.3. */
@@ -930,8 +929,6 @@ struct ssl_handshake_st {
 
 SSL_HANDSHAKE *ssl_handshake_new(enum ssl_hs_wait_t (*do_handshake)(SSL *ssl));
 
-void ssl_handshake_clear_groups(SSL_HANDSHAKE *hs);
-
 /* ssl_handshake_free releases all memory associated with |hs|. */
 void ssl_handshake_free(SSL_HANDSHAKE *hs);
 
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index e77e8ca6..52c93aa9 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -142,19 +142,6 @@ SSL_HANDSHAKE *ssl_handshake_new(enum ssl_hs_wait_t (*do_handshake)(SSL *ssl)) {
   return hs;
 }
 
-void ssl_handshake_clear_groups(SSL_HANDSHAKE *hs) {
-  if (hs->groups == NULL) {
-    return;
-  }
-
-  for (size_t i = 0; i < hs->groups_len; i++) {
-    SSL_ECDH_CTX_cleanup(&hs->groups[i]);
-  }
-  OPENSSL_free(hs->groups);
-  hs->groups = NULL;
-  hs->groups_len = 0;
-}
-
 void ssl_handshake_free(SSL_HANDSHAKE *hs) {
   if (hs == NULL) {
     return;
@@ -162,7 +149,7 @@ void ssl_handshake_free(SSL_HANDSHAKE *hs) {
 
   OPENSSL_cleanse(hs->secret, sizeof(hs->secret));
   OPENSSL_cleanse(hs->traffic_secret_0, sizeof(hs->traffic_secret_0));
-  ssl_handshake_clear_groups(hs);
+  SSL_ECDH_CTX_cleanup(&hs->ecdh_ctx);
   OPENSSL_free(hs->key_share_bytes);
   OPENSSL_free(hs->public_key);
   OPENSSL_free(hs->peer_sigalgs);
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 32d95940..81dbdc4b 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2093,8 +2093,7 @@ static int ext_key_share_add_clienthello(SSL *ssl, CBB *out) {
     return 0;
   }
 
-  const uint16_t *groups;
-  size_t groups_len;
+  uint16_t group_id;
   if (ssl->s3->hs->retry_group) {
     /* Append the new key share to the old list. */
     if (!CBB_add_bytes(&kse_bytes, ssl->s3->hs->key_share_bytes,
@@ -2105,35 +2104,27 @@ static int ext_key_share_add_clienthello(SSL *ssl, CBB *out) {
     ssl->s3->hs->key_share_bytes = NULL;
     ssl->s3->hs->key_share_bytes_len = 0;
 
-    groups = &ssl->s3->hs->retry_group;
-    groups_len = 1;
+    group_id = ssl->s3->hs->retry_group;
   } else {
+    /* Predict the most preferred group. */
+    const uint16_t *groups;
+    size_t groups_len;
     tls1_get_grouplist(ssl, 0 /* local groups */, &groups, &groups_len);
-    /* Only send the top two preferred key shares. */
-    if (groups_len > 2) {
-      groups_len = 2;
+    if (groups_len == 0) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_NO_GROUPS_SPECIFIED);
+      return 0;
     }
-  }
 
-  ssl->s3->hs->groups = OPENSSL_malloc(groups_len * sizeof(SSL_ECDH_CTX));
-  if (ssl->s3->hs->groups == NULL) {
-    return 0;
+    group_id = groups[0];
   }
-  memset(ssl->s3->hs->groups, 0, groups_len * sizeof(SSL_ECDH_CTX));
-  ssl->s3->hs->groups_len = groups_len;
-
-  for (size_t i = 0; i < groups_len; i++) {
-    if (!CBB_add_u16(&kse_bytes, groups[i])) {
-      return 0;
-    }
 
-    CBB key_exchange;
-    if (!CBB_add_u16_length_prefixed(&kse_bytes, &key_exchange) ||
-        !SSL_ECDH_CTX_init(&ssl->s3->hs->groups[i], groups[i]) ||
-        !SSL_ECDH_CTX_offer(&ssl->s3->hs->groups[i], &key_exchange) ||
-        !CBB_flush(&kse_bytes)) {
-      return 0;
-    }
+  CBB key_exchange;
+  if (!CBB_add_u16(&kse_bytes, group_id) ||
+      !CBB_add_u16_length_prefixed(&kse_bytes, &key_exchange) ||
+      !SSL_ECDH_CTX_init(&ssl->s3->hs->ecdh_ctx, group_id) ||
+      !SSL_ECDH_CTX_offer(&ssl->s3->hs->ecdh_ctx, &key_exchange) ||
+      !CBB_flush(&kse_bytes)) {
+    return 0;
   }
 
   if (!ssl->s3->hs->retry_group) {
@@ -2162,28 +2153,21 @@ int ssl_ext_key_share_parse_serverhello(SSL *ssl, uint8_t **out_secret,
     return 0;
   }
 
-  SSL_ECDH_CTX *group_ctx = NULL;
-  for (size_t i = 0; i < ssl->s3->hs->groups_len; i++) {
-    if (SSL_ECDH_CTX_get_id(&ssl->s3->hs->groups[i]) == group_id) {
-      group_ctx = &ssl->s3->hs->groups[i];
-      break;
-    }
-  }
-
-  if (group_ctx == NULL) {
+  if (SSL_ECDH_CTX_get_id(&ssl->s3->hs->ecdh_ctx) != group_id) {
     *out_alert = SSL_AD_ILLEGAL_PARAMETER;
     OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE);
     return 0;
   }
 
-  if (!SSL_ECDH_CTX_finish(group_ctx, out_secret, out_secret_len, out_alert,
-                           CBS_data(&peer_key), CBS_len(&peer_key))) {
+  if (!SSL_ECDH_CTX_finish(&ssl->s3->hs->ecdh_ctx, out_secret, out_secret_len,
+                           out_alert, CBS_data(&peer_key),
+                           CBS_len(&peer_key))) {
     *out_alert = SSL_AD_INTERNAL_ERROR;
     return 0;
   }
 
   ssl->s3->new_session->key_exchange_info = group_id;
-  ssl_handshake_clear_groups(ssl->s3->hs);
+  SSL_ECDH_CTX_cleanup(&ssl->s3->hs->ecdh_ctx);
   return 1;
 }
 
diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c
index 09c013e7..ee73f73d 100644
--- a/ssl/tls13_client.c
+++ b/ssl/tls13_client.c
@@ -85,20 +85,15 @@ static enum ssl_hs_wait_t do_process_hello_retry_request(SSL *ssl,
     return ssl_hs_error;
   }
 
-  for (size_t i = 0; i < ssl->s3->hs->groups_len; i++) {
-    /* Check that the HelloRetryRequest does not request a key share that was
-     * provided in the initial ClientHello.
-     *
-     * TODO(svaldez): Don't enforce this check when the HelloRetryRequest is due
-     * to a cookie. */
-    if (SSL_ECDH_CTX_get_id(&ssl->s3->hs->groups[i]) == group_id) {
-      ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
-      OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE);
-      return ssl_hs_error;
-    }
+  /* Check that the HelloRetryRequest does not request the key share that was
+   * provided in the initial ClientHello. */
+  if (SSL_ECDH_CTX_get_id(&ssl->s3->hs->ecdh_ctx) == group_id) {
+    ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_CURVE);
+    return ssl_hs_error;
   }
 
-  ssl_handshake_clear_groups(ssl->s3->hs);
+  SSL_ECDH_CTX_cleanup(&ssl->s3->hs->ecdh_ctx);
   ssl->s3->hs->retry_group = group_id;
 
   hs->state = state_send_second_client_hello;
@@ -679,7 +674,7 @@ int tls13_process_new_session_ticket(SSL *ssl) {
 }
 
 void ssl_clear_tls13_state(SSL *ssl) {
-  ssl_handshake_clear_groups(ssl->s3->hs);
+  SSL_ECDH_CTX_cleanup(&ssl->s3->hs->ecdh_ctx);
 
   OPENSSL_free(ssl->s3->hs->key_share_bytes);
   ssl->s3->hs->key_share_bytes = NULL;