Defer early keys to QUIC clients to after certificate reverification.
On a client using SSL_CTX_set_reverify_on_resume, we currently release
the early data keys before reverification rather than afterwards. This
means the QUIC implementation needs to watch for SSL_do_handshake's
return value before using the keys we've released. It is better to be
robust, so defer releasing the keys in the first place.
To avoid oddities around TCP and QUIC differences, tweak the 0-RTT cert
reverification to not send an alert on error. Sending such an alert
under early data is somewhat questionable given the server may not be
able to read it anyway.
Bug: 303
Change-Id: I42c16f9f046322d0b03cb0b425e11471f2fbe52a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/38885
Reviewed-by: Nick Harper <nharper@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/handshake.cc b/ssl/handshake.cc
index 707c6d2..33efc81 100644
--- a/ssl/handshake.cc
+++ b/ssl/handshake.cc
@@ -386,7 +386,8 @@
// SSL_VERIFY_NONE
// 3. We don't call the OCSP callback.
// 4. We only support custom verify callbacks.
-enum ssl_verify_result_t ssl_reverify_peer_cert(SSL_HANDSHAKE *hs) {
+enum ssl_verify_result_t ssl_reverify_peer_cert(SSL_HANDSHAKE *hs,
+ bool send_alert) {
SSL *const ssl = hs->ssl;
assert(ssl->s3->established_session == nullptr);
assert(hs->config->verify_mode != SSL_VERIFY_NONE);
@@ -399,7 +400,9 @@
if (ret == ssl_verify_invalid) {
OPENSSL_PUT_ERROR(SSL, SSL_R_CERTIFICATE_VERIFY_FAILED);
- ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
+ if (send_alert) {
+ ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
+ }
}
return ret;
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 23f48c1..4041fe9 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -458,8 +458,7 @@
if (!tls13_init_early_key_schedule(
hs, MakeConstSpan(ssl->session->master_key,
ssl->session->master_key_length)) ||
- !tls13_derive_early_secret(hs) ||
- !tls13_set_early_secret_for_quic(hs)) {
+ !tls13_derive_early_secret(hs)) {
return ssl_hs_error;
}
if (ssl->quic_method == nullptr &&
@@ -477,17 +476,30 @@
static enum ssl_hs_wait_t do_early_reverify_server_certificate(SSL_HANDSHAKE *hs) {
if (hs->ssl->ctx->reverify_on_resume) {
- switch (ssl_reverify_peer_cert(hs)) {
- case ssl_verify_ok:
- break;
- case ssl_verify_invalid:
- return ssl_hs_error;
- case ssl_verify_retry:
- hs->state = state_early_reverify_server_certificate;
- return ssl_hs_certificate_verify;
+ // 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.
+ //
+ // TODO(davidben): The client behavior should be to verify the certificate
+ // before deciding whether to offer the session and, if invalid, decline to
+ // send the session.
+ switch (ssl_reverify_peer_cert(hs, /*send_alert=*/false)) {
+ case ssl_verify_ok:
+ break;
+ case ssl_verify_invalid:
+ return ssl_hs_error;
+ case ssl_verify_retry:
+ hs->state = state_early_reverify_server_certificate;
+ return ssl_hs_certificate_verify;
}
}
+ // Defer releasing the 0-RTT key to after certificate reverification, so the
+ // QUIC implementation does not accidentally write data too early.
+ if (!tls13_set_early_secret_for_quic(hs)) {
+ return ssl_hs_error;
+ }
+
hs->in_early_data = true;
hs->can_early_write = true;
hs->state = state_read_server_hello;
@@ -907,7 +919,7 @@
static enum ssl_hs_wait_t do_reverify_server_certificate(SSL_HANDSHAKE *hs) {
assert(hs->ssl->ctx->reverify_on_resume);
- switch (ssl_reverify_peer_cert(hs)) {
+ switch (ssl_reverify_peer_cert(hs, /*send_alert=*/true)) {
case ssl_verify_ok:
break;
case ssl_verify_invalid:
diff --git a/ssl/internal.h b/ssl/internal.h
index 5f81b22..dca1b95 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1911,7 +1911,8 @@
enum ssl_verify_result_t ssl_verify_peer_cert(SSL_HANDSHAKE *hs);
// ssl_reverify_peer_cert verifies the peer certificate for |hs| when resuming a
// session.
-enum ssl_verify_result_t ssl_reverify_peer_cert(SSL_HANDSHAKE *hs);
+enum ssl_verify_result_t ssl_reverify_peer_cert(SSL_HANDSHAKE *hs,
+ bool send_alert);
enum ssl_hs_wait_t ssl_get_finished(SSL_HANDSHAKE *hs);
bool ssl_send_finished(SSL_HANDSHAKE *hs);
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 6211c56..41d6bc2 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -5298,6 +5298,53 @@
}
}
+TEST_F(QUICMethodTest, NoZeroRTTKeysBeforeReverify) {
+ const SSL_QUIC_METHOD quic_method = {
+ SetEncryptionSecretsCallback,
+ AddHandshakeDataCallback,
+ FlushFlightCallback,
+ SendAlertCallback,
+ };
+
+ SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH);
+ SSL_CTX_set_early_data_enabled(client_ctx_.get(), 1);
+ SSL_CTX_set_reverify_on_resume(client_ctx_.get(), 1);
+ SSL_CTX_set_early_data_enabled(server_ctx_.get(), 1);
+ ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method));
+ ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method));
+
+ bssl::UniquePtr<SSL_SESSION> session = CreateClientSessionForQUIC();
+ ASSERT_TRUE(session);
+
+ ASSERT_TRUE(CreateClientAndServer());
+ SSL_set_session(client_.get(), session.get());
+
+ // Configure the certificate (re)verification to never complete. The client
+ // handshake should pause.
+ SSL_set_custom_verify(
+ client_.get(), SSL_VERIFY_PEER,
+ [](SSL *ssl, uint8_t *out_alert) -> ssl_verify_result_t {
+ return ssl_verify_retry;
+ });
+ ASSERT_EQ(SSL_do_handshake(client_.get()), -1);
+ ASSERT_EQ(SSL_get_error(client_.get(), -1),
+ SSL_ERROR_WANT_CERTIFICATE_VERIFY);
+
+ // The early data keys have not yet been released.
+ EXPECT_FALSE(transport_->client()->HasSecrets(ssl_encryption_early_data));
+
+ // After the verification completes, the handshake progresses to the 0-RTT
+ // point and releases keys.
+ SSL_set_custom_verify(
+ client_.get(), SSL_VERIFY_PEER,
+ [](SSL *ssl, uint8_t *out_alert) -> ssl_verify_result_t {
+ return ssl_verify_ok;
+ });
+ ASSERT_EQ(SSL_do_handshake(client_.get()), 1);
+ EXPECT_TRUE(SSL_in_early_data(client_.get()));
+ EXPECT_TRUE(transport_->client()->HasSecrets(ssl_encryption_early_data));
+}
+
// Test only releasing data to QUIC one byte at a time on request, to maximize
// state machine pauses. Additionally, test that existing asynchronous callbacks
// still work.
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index fa3f3a6..8bb3339 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -593,7 +593,7 @@
static enum ssl_hs_wait_t do_server_certificate_reverify(
SSL_HANDSHAKE *hs) {
- switch (ssl_reverify_peer_cert(hs)) {
+ switch (ssl_reverify_peer_cert(hs, /*send_alert=*/true)) {
case ssl_verify_ok:
break;
case ssl_verify_invalid:
