commit 9001a96aeb5ab6d4f23c726e9ad6c0d05e1b1196
author rch <rch@google.com> Tue Dec 17 10:44:04 2019 -0800
committer Copybara-Service <copybara-worker@google.com> Tue Dec 17 10:44:40 2019 -0800
tree a785c5a4f82419640f662af3a93a9632eab70a34
parent 3341640f47173590ecc5fbe019f3d17d4706ca16

gfe-relnote: Call SetDefaultEncryptionLevel after setting crypto_negotiated_params in TlsServerHandshaker and TlsClientHandshaker. Protected by disabled QUIC versions.

Also add a QUIC_BUG in QuicSession::SetDefaultEncryptionLevel to check this condition.

This fixes a DCHECK failure in chrome in which the handshake completes, but GetSSLInfo fails because the cipher suite is not yet set.

https://bugs.chromium.org/p/chromium/issues/detail?id=1032263

PiperOrigin-RevId: 286011546
Change-Id: Ie9e03fa5cf6e3c346181435d45f362de10dc7083

quic/core/http/quic_spdy_session_test.cc

diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc
index 2867f5e..ac5f707 100644
--- a/quic/core/http/quic_spdy_session_test.cc
+++ b/quic/core/http/quic_spdy_session_test.cc
@@ -74,7 +74,10 @@
         QuicCryptoHandshaker(this, session),
         encryption_established_(false),
         handshake_confirmed_(false),
-        params_(new QuicCryptoNegotiatedParameters) {}
+        params_(new QuicCryptoNegotiatedParameters) {
+    // Simulate a negotiated cipher_suite with a fake value.
+    params_->cipher_suite = 1;
+  }
 
   void OnHandshakeMessage(const CryptoHandshakeMessage& /*message*/) override {
     encryption_established_ = true;

quic/core/quic_session.cc

diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc
index 9a0bae6..74dc15d 100644
--- a/quic/core/quic_session.cc
+++ b/quic/core/quic_session.cc
@@ -1357,6 +1357,11 @@
     case ENCRYPTION_HANDSHAKE:
       break;
     case ENCRYPTION_FORWARD_SECURE:
+      if (connection_->version().handshake_protocol == PROTOCOL_TLS1_3) {
+        QUIC_BUG_IF(!GetCryptoStream()->crypto_negotiated_params().cipher_suite)
+            << ENDPOINT
+            << "Handshake confirmed without cipher suite negotiation.";
+      }
       QUIC_BUG_IF(!config_.negotiated())
           << ENDPOINT << "Handshake confirmed without parameter negotiation.";
       break;

quic/core/quic_session_test.cc

diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc
index 4c7e112..935ef0b 100644
--- a/quic/core/quic_session_test.cc
+++ b/quic/core/quic_session_test.cc
@@ -61,7 +61,10 @@
         QuicCryptoHandshaker(this, session),
         encryption_established_(false),
         handshake_confirmed_(false),
-        params_(new QuicCryptoNegotiatedParameters) {}
+        params_(new QuicCryptoNegotiatedParameters) {
+    // Simulate a negotiated cipher_suite with a fake value.
+    params_->cipher_suite = 1;
+  }
 
   void OnHandshakeMessage(const CryptoHandshakeMessage& /*message*/) override {
     encryption_established_ = true;

quic/core/tls_client_handshaker.cc

diff --git a/quic/core/tls_client_handshaker.cc b/quic/core/tls_client_handshaker.cc
index b027004..5d9ffd4 100644
--- a/quic/core/tls_client_handshaker.cc
+++ b/quic/core/tls_client_handshaker.cc
@@ -337,7 +337,6 @@
 
   encryption_established_ = true;
   handshake_confirmed_ = true;
-  delegate()->SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
 
   // Fill crypto_negotiated_params_:
   const SSL_CIPHER* cipher = SSL_get_current_cipher(ssl());
@@ -347,6 +346,8 @@
   crypto_negotiated_params_->key_exchange_group = SSL_get_curve_id(ssl());
   crypto_negotiated_params_->peer_signature_algorithm =
       SSL_get_peer_signature_algorithm(ssl());
+
+  delegate()->SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
   // TODO(fayang): Replace this with DiscardOldKeys(ENCRYPTION_HANDSHAKE) when
   // handshake key discarding settles down.
   delegate()->NeuterHandshakeData();

quic/core/tls_server_handshaker.cc

diff --git a/quic/core/tls_server_handshaker.cc b/quic/core/tls_server_handshaker.cc
index 2d8cd79..87c146f 100644
--- a/quic/core/tls_server_handshaker.cc
+++ b/quic/core/tls_server_handshaker.cc
@@ -265,7 +265,6 @@
 
   encryption_established_ = true;
   handshake_confirmed_ = true;
-  delegate()->SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
 
   // Fill crypto_negotiated_params_:
   const SSL_CIPHER* cipher = SSL_get_current_cipher(ssl());
@@ -273,6 +272,8 @@
     crypto_negotiated_params_->cipher_suite = SSL_CIPHER_get_value(cipher);
   }
   crypto_negotiated_params_->key_exchange_group = SSL_get_curve_id(ssl());
+
+  delegate()->SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
   // TODO(fayang): Replace this with DiscardOldKeys(ENCRYPTION_HANDSHAKE) when
   // handshake key discarding settles down.
   delegate()->NeuterHandshakeData();