Check the second ClientHello's PSK binder on resumption.
We perform all our negotiation based on the first ClientHello (for
consistency with what |select_certificate_cb| observed), which is in the
transcript, so we can ignore most of the second one.
However, we ought to check the second PSK binder. That covers the client
key share, which we do consume. In particular, we'll want to check if it
we ever send half-RTT data on these connections (we do not currently do
this). It is also a tricky computation, so we enforce the peer handled
it correctly.
Tested that both Chrome and Firefox continue to interop with this check,
when configuring uncommon curve preferences that trigger HRR. (Normally
neither browser sees HRRs against BoringSSL servers.)
Update-Note: This does enforce some client behavior that we hadn't been
enforcing previously. However, it only figures into TLS 1.3 (not many
implementations yet), and only clients which hit HelloRetryRequest
(rare), so this should be low risk.
Change-Id: I42126585ec0685d009542094192e674cbd22520d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/37124
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index 6a00dfa..1e87bb9 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -241,10 +241,6 @@
return ssl_hs_error;
}
- if (!ssl_hash_message(hs, msg)) {
- return ssl_hs_error;
- }
-
hs->tls13_state = state_select_session;
return ssl_hs_ok;
}
@@ -263,20 +259,11 @@
return ssl_ticket_aead_ignore_ticket;
}
- // Verify that the pre_shared_key extension is the last extension in
- // ClientHello.
- if (CBS_data(&pre_shared_key) + CBS_len(&pre_shared_key) !=
- client_hello->extensions + client_hello->extensions_len) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_PRE_SHARED_KEY_MUST_BE_LAST);
- *out_alert = SSL_AD_ILLEGAL_PARAMETER;
- return ssl_ticket_aead_error;
- }
-
CBS ticket, binders;
uint32_t client_ticket_age;
- if (!ssl_ext_pre_shared_key_parse_clienthello(hs, &ticket, &binders,
- &client_ticket_age, out_alert,
- &pre_shared_key)) {
+ if (!ssl_ext_pre_shared_key_parse_clienthello(
+ hs, &ticket, &binders, &client_ticket_age, out_alert, client_hello,
+ &pre_shared_key)) {
return ssl_ticket_aead_error;
}
@@ -449,6 +436,10 @@
return ssl_hs_error;
}
+ if (!ssl_hash_message(hs, msg)) {
+ return ssl_hs_error;
+ }
+
if (ssl->s3->early_data_accepted) {
if (!tls13_derive_early_secrets(hs)) {
return ssl_hs_error;
@@ -532,6 +523,41 @@
return ssl_hs_error;
}
+ // We perform all our negotiation based on the first ClientHello (for
+ // consistency with what |select_certificate_cb| observed), which is in the
+ // transcript, so we can ignore most of this second one.
+ //
+ // We do, however, check the second PSK binder. This covers the client key
+ // share, in case we ever send half-RTT data (we currently do not). It is also
+ // a tricky computation, so we enforce the peer handled it correctly.
+ if (ssl->s3->session_reused) {
+ CBS pre_shared_key;
+ if (!ssl_client_hello_get_extension(&client_hello, &pre_shared_key,
+ TLSEXT_TYPE_pre_shared_key)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INCONSISTENT_CLIENT_HELLO);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return ssl_hs_error;
+ }
+
+ CBS ticket, binders;
+ uint32_t client_ticket_age;
+ uint8_t alert = SSL_AD_DECODE_ERROR;
+ if (!ssl_ext_pre_shared_key_parse_clienthello(
+ hs, &ticket, &binders, &client_ticket_age, &alert, &client_hello,
+ &pre_shared_key)) {
+ ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
+ return ssl_hs_error;
+ }
+
+ // Note it is important that we do not obtain a new |SSL_SESSION| from
+ // |ticket|. We have already selected parameters based on the first
+ // ClientHello (in the transcript) and must not switch partway through.
+ if (!tls13_verify_psk_binder(hs, hs->new_session.get(), msg, &binders)) {
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECRYPT_ERROR);
+ return ssl_hs_error;
+ }
+ }
+
bool need_retry;
if (!resolve_ecdhe_secret(hs, &need_retry, &client_hello)) {
if (need_retry) {
