commit 86c2e62f7428d1e49b0320ea4706704e5a6aca8a
author David Benjamin <davidben@google.com> Fri Sep 20 16:18:00 2024 -0400
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Sep 24 00:44:57 2024 +0000
tree 57efbd52affb3f63ff711111754cf84fe6eedb62
parent df091b2df3619badea8b58a46da7b1ffea0fc863

Rearrange 0-RTT key schedule setup slightly

We used to sometimes setup the 0-RTTs keys before reverifying the
certificate, and sometimes after due to QUIC's needs, which was why the
long comment (see
https://boringssl-review.googlesource.com/c/boringssl/+/38885). That
comment is now inaccurate and we consistently haven't installed 0-RTT
keys yet.

Update the comment and then avoid splitting up the key setup and the
reverify step. This shouldn't have any real consequences, except that
adding the dummy ChangeCipherSpec messgae now happens *after* setting
hs->early_session, and can potentially even happen after we set
in_early_data if we want to.

Bug: 42290594
Change-Id: I427c9cd521bd11982be3056a0099150a50dd9558
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71528
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>

ssl/handshake_client.cc

diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 5478df1..976e969 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -575,28 +575,20 @@
     return ssl_hs_ok;
   }
 
-  ssl->s3->aead_write_ctx->SetVersionIfNullCipher(ssl->session->ssl_version);
-  if (!ssl->method->add_change_cipher_spec(ssl)) {
-    return ssl_hs_error;
-  }
-
-  if (!tls13_init_early_key_schedule(hs, ssl->session.get()) ||
-      !tls13_derive_early_secret(hs)) {
-    return ssl_hs_error;
-  }
-
-  // Stash the early data session, so connection properties may be queried out
-  // of it.
+  // Stash the early data session. This must happen before
+  // |do_early_reverify_server_certificate|, so early connection properties are
+  // available to the callback.
   hs->early_session = UpRef(ssl->session);
   hs->state = state_early_reverify_server_certificate;
   return ssl_hs_ok;
 }
 
 static enum ssl_hs_wait_t do_early_reverify_server_certificate(SSL_HANDSHAKE *hs) {
-  if (hs->ssl->ctx->reverify_on_resume) {
-    // Don't send an alert on error. The alert be in early data, which the
-    // server may not accept anyway. It would also be a mismatch between QUIC
-    // and TCP because the QUIC early keys are deferred below.
+  SSL *const ssl = hs->ssl;
+  if (ssl->ctx->reverify_on_resume) {
+    // Don't send an alert on error. The alert would be in the clear, which the
+    // server is not expecting anyway. Alerts in between ClientHello and
+    // ServerHello cannot usefully be delivered in TLS 1.3.
     //
     // TODO(davidben): The client behavior should be to verify the certificate
     // before deciding whether to offer the session and, if invalid, decline to
@@ -612,9 +604,17 @@
     }
   }
 
+  ssl->s3->aead_write_ctx->SetVersionIfNullCipher(
+      hs->early_session->ssl_version);
+  if (!ssl->method->add_change_cipher_spec(ssl)) {
+    return ssl_hs_error;
+  }
+
   // Defer releasing the 0-RTT key to after certificate reverification, so the
   // QUIC implementation does not accidentally write data too early.
-  if (!tls13_set_traffic_key(hs->ssl, ssl_encryption_early_data, evp_aead_seal,
+  if (!tls13_init_early_key_schedule(hs, hs->early_session.get()) ||
+      !tls13_derive_early_secret(hs) ||
+      !tls13_set_traffic_key(hs->ssl, ssl_encryption_early_data, evp_aead_seal,
                              hs->early_session.get(),
                              hs->early_traffic_secret())) {
     return ssl_hs_error;