In QUIC TlsServerHandshaker, ensure DecryptCallback runs at most once. Protected by FLAGS_quic_reloadable_flag_quic_tls_fix_ticket_decrypt. PiperOrigin-RevId: 389907850
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 3530ba4..00007a5 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -71,6 +71,8 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_drop_unsent_path_response, true) // If true, enable server retransmittable on wire PING. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_enable_server_on_wire_ping, true) +// If true, fix a bug in TlsServerHandshaker where the ticket decrypt callback is cleared without being cancelled first. +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_tls_fix_ticket_decrypt, true) // If true, ignore peer_max_ack_delay during handshake. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_ignore_peer_max_ack_delay_during_handshake, true) // If true, include stream information in idle timeout connection close detail.
diff --git a/quic/core/tls_server_handshaker.cc b/quic/core/tls_server_handshaker.cc index 800d6f8..dcd7ea3 100644 --- a/quic/core/tls_server_handshaker.cc +++ b/quic/core/tls_server_handshaker.cc
@@ -138,6 +138,29 @@ // The callback was cancelled before we could run. return; } + if (handshaker_->fix_ticket_decrypt_) { + TlsServerHandshaker* handshaker = handshaker_; + handshaker_ = nullptr; + + handshaker->decrypted_session_ticket_ = std::move(plaintext); + // DecryptCallback::Run could be called synchronously. When that happens, we + // are currently in the middle of a call to AdvanceHandshake. + // (AdvanceHandshake called SSL_do_handshake, which through some layers + // called SessionTicketOpen, which called TicketCrypter::Decrypt, which + // synchronously called this function.) In that case, the handshake will + // continue to be processed when this function returns. + // + // When this callback is called asynchronously (i.e. the ticket decryption + // is pending), TlsServerHandshaker is not actively processing handshake + // messages. We need to have it resume processing handshake messages by + // calling AdvanceHandshake. + if (handshaker->expected_ssl_error() == SSL_ERROR_PENDING_TICKET) { + handshaker->AdvanceHandshakeFromCallback(); + } + + handshaker->ticket_decryption_callback_ = nullptr; + return; + } handshaker_->decrypted_session_ticket_ = std::move(plaintext); // DecryptCallback::Run could be called synchronously. When that happens, we // are currently in the middle of a call to AdvanceHandshake. @@ -725,15 +748,17 @@ ticket_decryption_callback_ = new DecryptCallback(this); proof_source_->GetTicketCrypter()->Decrypt( in, std::unique_ptr<DecryptCallback>(ticket_decryption_callback_)); + // Decrypt can run the callback synchronously. In that case, the callback // will clear the ticket_decryption_callback_ pointer, and instead of - // returning ssl_ticket_aead_retry, we should continue processing to return - // the decrypted ticket. + // returning ssl_ticket_aead_retry, we should continue processing to + // return the decrypted ticket. // // If the callback is not run synchronously, return ssl_ticket_aead_retry // and when the callback is complete this function will be run again to // return the result. if (ticket_decryption_callback_) { + QUICHE_DCHECK(!ticket_decryption_callback_->IsDone()); set_expected_ssl_error(SSL_ERROR_PENDING_TICKET); if (async_op_timer_.has_value()) { QUIC_CODE_COUNT( @@ -741,6 +766,18 @@ } async_op_timer_ = QuicTimeAccumulator(); async_op_timer_->Start(now()); + + if (!fix_ticket_decrypt_) { + return ssl_ticket_aead_retry; + } + } + } + + if (fix_ticket_decrypt_) { + // If the async ticket decryption is pending, either started by this + // SessionTicketOpen call or one that happened earlier, return + // ssl_ticket_aead_retry. + if (ticket_decryption_callback_ && !ticket_decryption_callback_->IsDone()) { return ssl_ticket_aead_retry; } }
diff --git a/quic/core/tls_server_handshaker.h b/quic/core/tls_server_handshaker.h index d1a476f..7ed1a8c 100644 --- a/quic/core/tls_server_handshaker.h +++ b/quic/core/tls_server_handshaker.h
@@ -198,6 +198,11 @@ // If called, Cancel causes the pending callback to be a no-op. void Cancel(); + // Return true if either + // - Cancel() has been called. + // - Run() has been called, or is in the middle of it. + bool IsDone() const { return handshaker_ == nullptr; } + private: TlsServerHandshaker* handshaker_; }; @@ -348,6 +353,8 @@ HandshakeState state_ = HANDSHAKE_START; bool encryption_established_ = false; bool valid_alpn_received_ = false; + const bool fix_ticket_decrypt_ = + GetQuicReloadableFlag(quic_tls_fix_ticket_decrypt); QuicReferenceCountedPointer<QuicCryptoNegotiatedParameters> crypto_negotiated_params_; TlsServerConnection tls_connection_;
diff --git a/quic/core/tls_server_handshaker_test.cc b/quic/core/tls_server_handshaker_test.cc index 6f8db12..2478d8f 100644 --- a/quic/core/tls_server_handshaker_test.cc +++ b/quic/core/tls_server_handshaker_test.cc
@@ -101,6 +101,7 @@ return fake_proof_source_handle_; } + using TlsServerHandshaker::AdvanceHandshake; using TlsServerHandshaker::expected_ssl_error; private: @@ -708,6 +709,43 @@ EXPECT_TRUE(server_stream()->ResumptionAttempted()); } +TEST_P(TlsServerHandshakerTest, AdvanceHandshakeDuringAsyncDecryptCallback) { + if (GetParam().disable_resumption) { + return; + } + + // Do the first handshake + InitializeFakeClient(); + CompleteCryptoHandshake(); + ExpectHandshakeSuccessful(); + + ticket_crypter_->SetRunCallbacksAsync(true); + // Now do another handshake + InitializeServerWithFakeProofSourceHandle(); + server_handshaker_->SetupProofSourceHandle( + /*select_cert_action=*/FakeProofSourceHandle::Action::DELEGATE_SYNC, + /*compute_signature_action=*/FakeProofSourceHandle::Action:: + DELEGATE_SYNC); + InitializeFakeClient(); + + AdvanceHandshakeWithFakeClient(); + + // Ensure an async DecryptCallback is now pending. + ASSERT_EQ(ticket_crypter_->NumPendingCallbacks(), 1u); + + { + QuicConnection::ScopedPacketFlusher flusher(server_connection_); + server_handshaker_->AdvanceHandshake(); + } + + // This will delete |server_handshaker_|. + server_session_ = nullptr; + + if (GetQuicReloadableFlag(quic_tls_fix_ticket_decrypt)) { + ticket_crypter_->RunPendingCallback(0); // Should not crash. + } +} + TEST_P(TlsServerHandshakerTest, ResumptionWithFailingDecryptCallback) { if (GetParam().disable_resumption) { return;