Make PSK handshake fail with QUIC+TLS
QUIC+TLS currently does not support pre-shared keys (PSK). Before this CL, the code would simply ignore the PSK and continue the handshake. This could lead to traffic not being authenticated. To avoid this class of security issues, this CL ensures that the QUIC+TLS handshake fails if a PSK has been configured. We will implement support for PSK in QUIC+TLS in a subsequent CL.
This CL also refactors the constructor for TlsServerHandshaker to give it access to the QuicCryptoServerConfig.
gfe-relnote: make QUIC+TLS+PSK fail, not used in production, TLS versions protected by gfe2_reloadable_flag_quic_enable_t050_v2, gfe2_reloadable_flag_quic_enable_version_draft_25_v3 and gfe2_reloadable_flag_quic_enable_version_draft_27.
PiperOrigin-RevId: 306890388
Change-Id: Ic254e3f049bcdc5b980916072e516447f198e2f1
diff --git a/quic/core/crypto/quic_crypto_client_config.h b/quic/core/crypto/quic_crypto_client_config.h
index e83156e..3ae19dd 100644
--- a/quic/core/crypto/quic_crypto_client_config.h
+++ b/quic/core/crypto/quic_crypto_client_config.h
@@ -398,10 +398,14 @@
// Saves the |alpn| that will be passed in QUIC's CHLO message.
void set_alpn(const std::string& alpn) { alpn_ = alpn; }
+ // Saves the pre-shared key used during the handshake.
void set_pre_shared_key(quiche::QuicheStringPiece psk) {
pre_shared_key_ = std::string(psk);
}
+ // Returns the pre-shared key used during the handshake.
+ const std::string& pre_shared_key() const { return pre_shared_key_; }
+
bool pad_inchoate_hello() const { return pad_inchoate_hello_; }
void set_pad_inchoate_hello(bool new_value) {
pad_inchoate_hello_ = new_value;
diff --git a/quic/core/crypto/quic_crypto_server_config.h b/quic/core/crypto/quic_crypto_server_config.h
index 4a8cb73..4fab2cd 100644
--- a/quic/core/crypto/quic_crypto_server_config.h
+++ b/quic/core/crypto/quic_crypto_server_config.h
@@ -432,6 +432,8 @@
SSL_CTX* ssl_ctx() const;
+ // Pre-shared key used during the handshake.
+ const std::string& pre_shared_key() const { return pre_shared_key_; }
void set_pre_shared_key(quiche::QuicheStringPiece psk) {
pre_shared_key_ = std::string(psk);
}
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc
index cec0ceb..a4d6cd7 100644
--- a/quic/core/http/end_to_end_test.cc
+++ b/quic/core/http/end_to_end_test.cc
@@ -3661,13 +3661,23 @@
client_->WaitForDelayedAcks();
}
-TEST_P(EndToEndTest, PreSharedKey) {
+TEST_P(EndToEndTestWithTls, PreSharedKey) {
client_config_.set_max_time_before_crypto_handshake(
QuicTime::Delta::FromSeconds(5));
client_config_.set_max_idle_time_before_crypto_handshake(
QuicTime::Delta::FromSeconds(5));
pre_shared_key_client_ = "foobar";
pre_shared_key_server_ = "foobar";
+
+ if (GetParam().negotiated_version.handshake_protocol == PROTOCOL_TLS1_3) {
+ // TODO(b/154162689) add PSK support to QUIC+TLS.
+ bool ok;
+ EXPECT_QUIC_BUG(ok = Initialize(),
+ "QUIC client pre-shared keys not yet supported with TLS");
+ EXPECT_FALSE(ok);
+ return;
+ }
+
ASSERT_TRUE(Initialize());
ASSERT_EQ(kFooResponseBody, client_->SendSynchronousRequest("/foo"));
@@ -3675,13 +3685,24 @@
}
// TODO: reenable once we have a way to make this run faster.
-TEST_P(EndToEndTest, QUIC_TEST_DISABLED_IN_CHROME(PreSharedKeyMismatch)) {
+TEST_P(EndToEndTestWithTls,
+ QUIC_TEST_DISABLED_IN_CHROME(PreSharedKeyMismatch)) {
client_config_.set_max_time_before_crypto_handshake(
QuicTime::Delta::FromSeconds(1));
client_config_.set_max_idle_time_before_crypto_handshake(
QuicTime::Delta::FromSeconds(1));
pre_shared_key_client_ = "foo";
pre_shared_key_server_ = "bar";
+
+ if (GetParam().negotiated_version.handshake_protocol == PROTOCOL_TLS1_3) {
+ // TODO(b/154162689) add PSK support to QUIC+TLS.
+ bool ok;
+ EXPECT_QUIC_BUG(ok = Initialize(),
+ "QUIC client pre-shared keys not yet supported with TLS");
+ EXPECT_FALSE(ok);
+ return;
+ }
+
// One of two things happens when Initialize() returns:
// 1. Crypto handshake has completed, and it is unsuccessful. Initialize()
// returns false.
@@ -3694,24 +3715,46 @@
}
// TODO: reenable once we have a way to make this run faster.
-TEST_P(EndToEndTest, QUIC_TEST_DISABLED_IN_CHROME(PreSharedKeyNoClient)) {
+TEST_P(EndToEndTestWithTls,
+ QUIC_TEST_DISABLED_IN_CHROME(PreSharedKeyNoClient)) {
client_config_.set_max_time_before_crypto_handshake(
QuicTime::Delta::FromSeconds(1));
client_config_.set_max_idle_time_before_crypto_handshake(
QuicTime::Delta::FromSeconds(1));
pre_shared_key_server_ = "foobar";
+
+ if (GetParam().negotiated_version.handshake_protocol == PROTOCOL_TLS1_3) {
+ // TODO(b/154162689) add PSK support to QUIC+TLS.
+ bool ok;
+ EXPECT_QUIC_BUG(ok = Initialize(),
+ "QUIC server pre-shared keys not yet supported with TLS");
+ EXPECT_FALSE(ok);
+ return;
+ }
+
ASSERT_FALSE(Initialize() &&
client_->client()->WaitForCryptoHandshakeConfirmed());
EXPECT_THAT(client_->connection_error(), IsError(QUIC_HANDSHAKE_TIMEOUT));
}
// TODO: reenable once we have a way to make this run faster.
-TEST_P(EndToEndTest, QUIC_TEST_DISABLED_IN_CHROME(PreSharedKeyNoServer)) {
+TEST_P(EndToEndTestWithTls,
+ QUIC_TEST_DISABLED_IN_CHROME(PreSharedKeyNoServer)) {
client_config_.set_max_time_before_crypto_handshake(
QuicTime::Delta::FromSeconds(1));
client_config_.set_max_idle_time_before_crypto_handshake(
QuicTime::Delta::FromSeconds(1));
pre_shared_key_client_ = "foobar";
+
+ if (GetParam().negotiated_version.handshake_protocol == PROTOCOL_TLS1_3) {
+ // TODO(b/154162689) add PSK support to QUIC+TLS.
+ bool ok;
+ EXPECT_QUIC_BUG(ok = Initialize(),
+ "QUIC client pre-shared keys not yet supported with TLS");
+ EXPECT_FALSE(ok);
+ return;
+ }
+
ASSERT_FALSE(Initialize() &&
client_->client()->WaitForCryptoHandshakeConfirmed());
EXPECT_THAT(client_->connection_error(), IsError(QUIC_HANDSHAKE_TIMEOUT));
diff --git a/quic/core/http/quic_server_session_base_test.cc b/quic/core/http/quic_server_session_base_test.cc
index 98cdf49..606d3eb 100644
--- a/quic/core/http/quic_server_session_base_test.cc
+++ b/quic/core/http/quic_server_session_base_test.cc
@@ -484,9 +484,8 @@
class MockTlsServerHandshaker : public TlsServerHandshaker {
public:
explicit MockTlsServerHandshaker(QuicServerSessionBase* session,
- SSL_CTX* ssl_ctx,
- ProofSource* proof_source)
- : TlsServerHandshaker(session, ssl_ctx, proof_source) {}
+ const QuicCryptoServerConfig& crypto_config)
+ : TlsServerHandshaker(session, crypto_config) {}
MockTlsServerHandshaker(const MockTlsServerHandshaker&) = delete;
MockTlsServerHandshaker& operator=(const MockTlsServerHandshaker&) = delete;
~MockTlsServerHandshaker() override {}
@@ -532,8 +531,7 @@
quic_crypto_stream);
} else {
tls_server_stream =
- new MockTlsServerHandshaker(session_.get(), crypto_config_.ssl_ctx(),
- crypto_config_.proof_source());
+ new MockTlsServerHandshaker(session_.get(), crypto_config_);
QuicServerSessionBasePeer::SetCryptoStream(session_.get(),
tls_server_stream);
}
diff --git a/quic/core/quic_crypto_server_stream_base.cc b/quic/core/quic_crypto_server_stream_base.cc
index 0a77ca7..8ea63ba 100644
--- a/quic/core/quic_crypto_server_stream_base.cc
+++ b/quic/core/quic_crypto_server_stream_base.cc
@@ -37,8 +37,8 @@
return std::unique_ptr<QuicCryptoServerStream>(new QuicCryptoServerStream(
crypto_config, compressed_certs_cache, session, helper));
case PROTOCOL_TLS1_3:
- return std::unique_ptr<TlsServerHandshaker>(new TlsServerHandshaker(
- session, crypto_config->ssl_ctx(), crypto_config->proof_source()));
+ return std::unique_ptr<TlsServerHandshaker>(
+ new TlsServerHandshaker(session, *crypto_config));
case PROTOCOL_UNSUPPORTED:
break;
}
diff --git a/quic/core/tls_client_handshaker.cc b/quic/core/tls_client_handshaker.cc
index f3e1b13..baac324 100644
--- a/quic/core/tls_client_handshaker.cc
+++ b/quic/core/tls_client_handshaker.cc
@@ -59,6 +59,7 @@
proof_handler_(proof_handler),
session_cache_(crypto_config->session_cache()),
user_agent_id_(crypto_config->user_agent_id()),
+ pre_shared_key_(crypto_config->pre_shared_key()),
crypto_negotiated_params_(new QuicCryptoNegotiatedParameters),
tls_connection_(crypto_config->ssl_ctx(), this) {}
@@ -71,6 +72,15 @@
bool TlsClientHandshaker::CryptoConnect() {
state_ = STATE_HANDSHAKE_RUNNING;
+ if (!pre_shared_key_.empty()) {
+ // TODO(b/154162689) add PSK support to QUIC+TLS.
+ std::string error_details =
+ "QUIC client pre-shared keys not yet supported with TLS";
+ QUIC_BUG << error_details;
+ CloseConnection(QUIC_HANDSHAKE_FAILED, error_details);
+ return false;
+ }
+
// Set the SNI to send, if any.
SSL_set_connect_state(ssl());
if (!server_id_.host().empty() &&
diff --git a/quic/core/tls_client_handshaker.h b/quic/core/tls_client_handshaker.h
index 6e29544..0128c8b 100644
--- a/quic/core/tls_client_handshaker.h
+++ b/quic/core/tls_client_handshaker.h
@@ -147,6 +147,9 @@
std::string user_agent_id_;
+ // Pre-shared key used during the handshake.
+ std::string pre_shared_key_;
+
// ProofVerifierCallback used for async certificate verification. This object
// is owned by |proof_verifier_|.
ProofVerifierCallbackImpl* proof_verify_callback_ = nullptr;
diff --git a/quic/core/tls_handshaker_test.cc b/quic/core/tls_handshaker_test.cc
index fa94aec..de3a8a4 100644
--- a/quic/core/tls_handshaker_test.cc
+++ b/quic/core/tls_handshaker_test.cc
@@ -2,9 +2,11 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <memory>
#include <string>
#include <utility>
+#include "net/third_party/quiche/src/quic/core/crypto/quic_crypto_server_config.h"
#include "net/third_party/quiche/src/quic/core/crypto/tls_client_connection.h"
#include "net/third_party/quiche/src/quic/core/crypto/tls_server_connection.h"
#include "net/third_party/quiche/src/quic/core/quic_utils.h"
@@ -223,8 +225,10 @@
MockProofHandler() = default;
~MockProofHandler() override {}
- MOCK_METHOD1(OnProofValid, void(const QuicCryptoClientConfig::CachedState&));
- MOCK_METHOD1(OnProofVerifyDetailsAvailable, void(const ProofVerifyDetails&));
+ MOCK_METHOD1(OnProofValid, // NOLINT(build/deprecated)
+ void(const QuicCryptoClientConfig::CachedState&));
+ MOCK_METHOD1(OnProofVerifyDetailsAvailable, // NOLINT(build/deprecated)
+ void(const ProofVerifyDetails&));
};
class TestQuicCryptoClientStream : public TestQuicCryptoStream {
@@ -270,10 +274,9 @@
class TestTlsServerHandshaker : public TlsServerHandshaker {
public:
TestTlsServerHandshaker(QuicSession* session,
- SSL_CTX* ssl_ctx,
- ProofSource* proof_source,
+ const QuicCryptoServerConfig& crypto_config,
TestQuicCryptoStream* test_stream)
- : TlsServerHandshaker(session, ssl_ctx, proof_source),
+ : TlsServerHandshaker(session, crypto_config),
test_stream_(test_stream) {}
void WriteCryptoData(EncryptionLevel level,
@@ -287,15 +290,14 @@
class TestQuicCryptoServerStream : public TestQuicCryptoStream {
public:
- TestQuicCryptoServerStream(QuicSession* session,
- FakeProofSource* proof_source)
+ TestQuicCryptoServerStream(QuicSession* session)
: TestQuicCryptoStream(session),
- proof_source_(proof_source),
- ssl_ctx_(TlsServerConnection::CreateSslCtx()),
- handshaker_(new TestTlsServerHandshaker(session,
- ssl_ctx_.get(),
- proof_source_,
- this)) {}
+ crypto_config_(QuicCryptoServerConfig::TESTING,
+ QuicRandom::GetInstance(),
+ std::make_unique<FakeProofSource>(),
+ KeyExchangeSource::Default()),
+ handshaker_(
+ new TestTlsServerHandshaker(session, crypto_config_, this)) {}
~TestQuicCryptoServerStream() override = default;
@@ -310,11 +312,12 @@
TlsHandshaker* handshaker() const override { return handshaker_.get(); }
- FakeProofSource* GetFakeProofSource() const { return proof_source_; }
+ FakeProofSource* GetFakeProofSource() const {
+ return static_cast<FakeProofSource*>(crypto_config_.proof_source());
+ }
private:
- FakeProofSource* proof_source_;
- bssl::UniquePtr<SSL_CTX> ssl_ctx_;
+ QuicCryptoServerConfig crypto_config_;
std::unique_ptr<TlsServerHandshaker> handshaker_;
};
@@ -343,8 +346,7 @@
server_session_(server_conn_, /*create_mock_crypto_stream=*/false) {
client_stream_ = new TestQuicCryptoClientStream(&client_session_);
client_session_.SetCryptoStream(client_stream_);
- server_stream_ =
- new TestQuicCryptoServerStream(&server_session_, &proof_source_);
+ server_stream_ = new TestQuicCryptoServerStream(&server_session_);
server_session_.SetCryptoStream(server_stream_);
client_session_.Initialize();
server_session_.Initialize();
@@ -400,7 +402,6 @@
MockQuicSession client_session_;
MockQuicSession server_session_;
- FakeProofSource proof_source_;
TestQuicCryptoClientStream* client_stream_;
TestQuicCryptoServerStream* server_stream_;
};
diff --git a/quic/core/tls_server_handshaker.cc b/quic/core/tls_server_handshaker.cc
index f4ce22b..75ff75d 100644
--- a/quic/core/tls_server_handshaker.cc
+++ b/quic/core/tls_server_handshaker.cc
@@ -44,14 +44,15 @@
handshaker_ = nullptr;
}
-TlsServerHandshaker::TlsServerHandshaker(QuicSession* session,
- SSL_CTX* ssl_ctx,
- ProofSource* proof_source)
+TlsServerHandshaker::TlsServerHandshaker(
+ QuicSession* session,
+ const QuicCryptoServerConfig& crypto_config)
: TlsHandshaker(this, session),
QuicCryptoServerStreamBase(session),
- proof_source_(proof_source),
+ proof_source_(crypto_config.proof_source()),
+ pre_shared_key_(crypto_config.pre_shared_key()),
crypto_negotiated_params_(new QuicCryptoNegotiatedParameters),
- tls_connection_(ssl_ctx, this) {
+ tls_connection_(crypto_config.ssl_ctx(), this) {
DCHECK_EQ(PROTOCOL_TLS1_3,
session->connection()->version().handshake_protocol);
@@ -390,6 +391,12 @@
return SSL_TLSEXT_ERR_ALERT_FATAL;
}
+ if (!pre_shared_key_.empty()) {
+ // TODO(b/154162689) add PSK support to QUIC+TLS.
+ QUIC_BUG << "QUIC server pre-shared keys not yet supported with TLS";
+ return SSL_TLSEXT_ERR_ALERT_FATAL;
+ }
+
std::vector<CRYPTO_BUFFER*> certs;
certs.resize(chain->certs.size());
for (size_t i = 0; i < certs.size(); i++) {
diff --git a/quic/core/tls_server_handshaker.h b/quic/core/tls_server_handshaker.h
index 22ae45d..066c53b 100644
--- a/quic/core/tls_server_handshaker.h
+++ b/quic/core/tls_server_handshaker.h
@@ -9,6 +9,7 @@
#include "third_party/boringssl/src/include/openssl/pool.h"
#include "third_party/boringssl/src/include/openssl/ssl.h"
+#include "net/third_party/quiche/src/quic/core/crypto/quic_crypto_server_config.h"
#include "net/third_party/quiche/src/quic/core/crypto/tls_server_connection.h"
#include "net/third_party/quiche/src/quic/core/proto/cached_network_parameters_proto.h"
#include "net/third_party/quiche/src/quic/core/quic_crypto_server_stream_base.h"
@@ -27,8 +28,7 @@
public QuicCryptoServerStreamBase {
public:
TlsServerHandshaker(QuicSession* session,
- SSL_CTX* ssl_ctx,
- ProofSource* proof_source);
+ const QuicCryptoServerConfig& crypto_config);
TlsServerHandshaker(const TlsServerHandshaker&) = delete;
TlsServerHandshaker& operator=(const TlsServerHandshaker&) = delete;
@@ -150,6 +150,9 @@
std::string cert_verify_sig_;
std::unique_ptr<ProofSource::Details> proof_source_details_;
+ // Pre-shared key used during the handshake.
+ std::string pre_shared_key_;
+
// Used to hold the ENCRYPTION_FORWARD_SECURE read secret until the handshake
// is complete. This is temporary until
// https://bugs.chromium.org/p/boringssl/issues/detail?id=303 is resolved.
