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>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index f94156f..e19b347 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -88,6 +88,7 @@
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 d7e5add..d629b8f 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4777,6 +4777,7 @@
#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 eff5672..232364e 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -896,9 +896,8 @@
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 @@
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 e77e8ca..52c93aa 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -142,19 +142,6 @@
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 @@
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 32d9594..81dbdc4 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2093,8 +2093,7 @@
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,36 +2104,28 @@
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;
}
+
+ group_id = groups[0];
}
- ssl->s3->hs->groups = OPENSSL_malloc(groups_len * sizeof(SSL_ECDH_CTX));
- if (ssl->s3->hs->groups == NULL) {
+ 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;
}
- 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;
- }
- }
if (!ssl->s3->hs->retry_group) {
/* Save the contents of the extension to repeat it in the second
@@ -2162,28 +2153,21 @@
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 09c013e..ee73f73 100644
--- a/ssl/tls13_client.c
+++ b/ssl/tls13_client.c
@@ -85,20 +85,15 @@
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 @@
}
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;
