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;
