gfe-relnote: In QUIC, close connection if decryption key is available before encryption key when TLS handshaker is used. Protected by disabled v99 flag. Also route the error up to BoringSSL layer. PiperOrigin-RevId: 294710276 Change-Id: I2af93903e76a81a0578e9791c1ccc25d35f9b5c5
diff --git a/quic/core/crypto/tls_connection.cc b/quic/core/crypto/tls_connection.cc index 4c59d3e..ec6a226 100644 --- a/quic/core/crypto/tls_connection.cc +++ b/quic/core/crypto/tls_connection.cc
@@ -125,8 +125,10 @@ std::vector<uint8_t> read_secret(key_length), write_secret(key_length); memcpy(read_secret.data(), read_key, key_length); memcpy(write_secret.data(), write_key, key_length); - ConnectionFromSsl(ssl)->delegate_->SetEncryptionSecret( - QuicEncryptionLevel(level), read_secret, write_secret); + if (!ConnectionFromSsl(ssl)->delegate_->SetEncryptionSecret( + QuicEncryptionLevel(level), read_secret, write_secret)) { + return 0; + } return 1; }
diff --git a/quic/core/crypto/tls_connection.h b/quic/core/crypto/tls_connection.h index cd9cec0..f037743 100644 --- a/quic/core/crypto/tls_connection.h +++ b/quic/core/crypto/tls_connection.h
@@ -37,7 +37,7 @@ // the handshake traffic secrets and application traffic secrets. For a // given level |level|, |read_secret| is the secret used for reading data, // and |write_secret| is the secret used for writing data. - virtual void SetEncryptionSecret( + virtual bool SetEncryptionSecret( EncryptionLevel level, const std::vector<uint8_t>& read_secret, const std::vector<uint8_t>& write_secret) = 0;
diff --git a/quic/core/handshaker_delegate_interface.h b/quic/core/handshaker_delegate_interface.h index 7e4b125..03b3af9 100644 --- a/quic/core/handshaker_delegate_interface.h +++ b/quic/core/handshaker_delegate_interface.h
@@ -17,8 +17,9 @@ public: virtual ~HandshakerDelegateInterface() {} - // Called when new decryption key of |level| is available. - virtual void OnNewDecryptionKeyAvailable( + // Called when new decryption key of |level| is available. Returns true if + // decrypter is set successfully, otherwise, returns false. + virtual bool OnNewDecryptionKeyAvailable( EncryptionLevel level, std::unique_ptr<QuicDecrypter> decrypter, bool set_alternative_decrypter,
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 6bdb6b0..fe0ea7b 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -1465,29 +1465,13 @@ current_effective_peer_migration_type_ = NO_CHANGE; - // Some encryption levels share a packet number space, it is therefore - // possible for us to want to ack some packets even though we do not yet - // have the appropriate keys to encrypt the acks. In this scenario we - // do not update the ACK timeout. This can happen for example with - // IETF QUIC on the server when we receive 0-RTT packets and do not yet - // have 1-RTT keys (0-RTT packets are acked at the 1-RTT level). - // Note that this could cause slight performance degradations in the edge - // case where one packet is received, then the encrypter is installed, - // then a second packet is received; as that could cause the ACK for the - // second packet to be delayed instead of immediate. This is currently - // considered to be small enough of an edge case to not be optimized for. - if (!SupportsMultiplePacketNumberSpaces() || - framer_.HasEncrypterOfEncryptionLevel(QuicUtils::GetEncryptionLevel( - QuicUtils::GetPacketNumberSpace(last_decrypted_packet_level_)))) { - uber_received_packet_manager_.MaybeUpdateAckTimeout( - should_last_packet_instigate_acks_, last_decrypted_packet_level_, - last_header_.packet_number, time_of_last_received_packet_, - clock_->ApproximateNow(), sent_packet_manager_.GetRttStats()); - } else { - QUIC_DLOG(INFO) << ENDPOINT << "Not updating ACK timeout for " - << EncryptionLevelToString(last_decrypted_packet_level_) - << " as we do not have the corresponding encrypter"; - } + // For IETF QUIC, it is guaranteed that TLS will give connection the + // corresponding write key before read key. In other words, connection should + // never process a packet while an ACK for it cannot be encrypted. + uber_received_packet_manager_.MaybeUpdateAckTimeout( + should_last_packet_instigate_acks_, last_decrypted_packet_level_, + last_header_.packet_number, time_of_last_received_packet_, + clock_->ApproximateNow(), sent_packet_manager_.GetRttStats()); ClearLastFrames(); CloseIfTooManyOutstandingSentPackets();
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc index f9d186d..ebd0380 100644 --- a/quic/core/quic_session.cc +++ b/quic/core/quic_session.cc
@@ -1306,24 +1306,30 @@ flow_controller_.UpdateSendWindowOffset(new_window); } -void QuicSession::OnNewDecryptionKeyAvailable( +bool QuicSession::OnNewDecryptionKeyAvailable( EncryptionLevel level, std::unique_ptr<QuicDecrypter> decrypter, bool set_alternative_decrypter, bool latch_once_used) { - // TODO(fayang): Close connection if corresponding encryption key is not - // available. - DCHECK(connection()->framer().HasEncrypterOfEncryptionLevel(level)); + if (connection_->version().handshake_protocol == PROTOCOL_TLS1_3 && + !connection()->framer().HasEncrypterOfEncryptionLevel( + QuicUtils::GetEncryptionLevel( + QuicUtils::GetPacketNumberSpace(level)))) { + // This should never happen because connection should never decrypt a packet + // while an ACK for it cannot be encrypted. + return false; + } if (connection()->version().KnowsWhichDecrypterToUse()) { connection()->InstallDecrypter(level, std::move(decrypter)); - return; + return true; } if (set_alternative_decrypter) { connection()->SetAlternativeDecrypter(level, std::move(decrypter), latch_once_used); - return; + return true; } connection()->SetDecrypter(level, std::move(decrypter)); + return true; } void QuicSession::OnNewEncryptionKeyAvailable(
diff --git a/quic/core/quic_session.h b/quic/core/quic_session.h index 57d7605..138f6c1 100644 --- a/quic/core/quic_session.h +++ b/quic/core/quic_session.h
@@ -243,7 +243,7 @@ virtual void OnConfigNegotiated(); // From HandshakerDelegateInterface - void OnNewDecryptionKeyAvailable(EncryptionLevel level, + bool OnNewDecryptionKeyAvailable(EncryptionLevel level, std::unique_ptr<QuicDecrypter> decrypter, bool set_alternative_decrypter, bool latch_once_used) override;
diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc index 153c787..e374432 100644 --- a/quic/core/quic_session_test.cc +++ b/quic/core/quic_session_test.cc
@@ -2822,6 +2822,17 @@ session_.SendRstStream(bidirectional, QUIC_STREAM_CANCELLED, 0); } +TEST_P(QuicSessionTestServer, DecryptionKeyAvailableBeforeEncryptionKey) { + if (connection_->version().handshake_protocol != PROTOCOL_TLS1_3) { + return; + } + ASSERT_FALSE(connection_->framer().HasEncrypterOfEncryptionLevel( + ENCRYPTION_HANDSHAKE)); + EXPECT_FALSE(session_.OnNewDecryptionKeyAvailable( + ENCRYPTION_HANDSHAKE, /*decrypter=*/nullptr, + /*set_alternative_decrypter=*/false, /*latch_once_used=*/false)); +} + // A client test class that can be used when the automatic configuration is not // desired. class QuicSessionTestClientUnconfigured : public QuicSessionTestBase {
diff --git a/quic/core/tls_handshaker.cc b/quic/core/tls_handshaker.cc index aee7133..4558928 100644 --- a/quic/core/tls_handshaker.cc +++ b/quic/core/tls_handshaker.cc
@@ -61,7 +61,7 @@ SSL_CIPHER_get_prf_nid(SSL_get_pending_cipher(ssl()))); } -void TlsHandshaker::SetEncryptionSecret( +bool TlsHandshaker::SetEncryptionSecret( EncryptionLevel level, const std::vector<uint8_t>& read_secret, const std::vector<uint8_t>& write_secret) { @@ -74,9 +74,10 @@ SSL_CIPHER_get_id(SSL_get_pending_cipher(ssl()))); CryptoUtils::SetKeyAndIV(Prf(), read_secret, decrypter.get()); delegate_->OnNewEncryptionKeyAvailable(level, std::move(encrypter)); - delegate_->OnNewDecryptionKeyAvailable(level, std::move(decrypter), - /*set_alternative_decrypter=*/false, - /*latch_once_used=*/false); + return delegate_->OnNewDecryptionKeyAvailable( + level, std::move(decrypter), + /*set_alternative_decrypter=*/false, + /*latch_once_used=*/false); } void TlsHandshaker::WriteMessage(EncryptionLevel level,
diff --git a/quic/core/tls_handshaker.h b/quic/core/tls_handshaker.h index 6fa22d0..d6df69b 100644 --- a/quic/core/tls_handshaker.h +++ b/quic/core/tls_handshaker.h
@@ -75,7 +75,8 @@ // secrets and application traffic secrets. For a given secret |secret|, // |level| indicates which EncryptionLevel it is to be used at, and |is_write| // indicates whether it is used for encryption or decryption. - void SetEncryptionSecret(EncryptionLevel level, + // Returns true if secret is sucessfully set, otherwise, returns false. + bool SetEncryptionSecret(EncryptionLevel level, const std::vector<uint8_t>& read_secret, const std::vector<uint8_t>& write_secret) override;