diff --git a/ssl/dtls_method.cc b/ssl/dtls_method.cc index ae26de7b1..d179baef5 100644 --- a/ssl/dtls_method.cc +++ b/ssl/dtls_method.cc @@ -78,7 +78,9 @@ static void dtls1_on_handshake_complete(SSL *ssl) { } static bool dtls1_set_read_state(SSL *ssl, ssl_encryption_level_t level, - UniquePtr aead_ctx) { + UniquePtr aead_ctx, + Span secret_for_quic) { + assert(secret_for_quic.empty()); // QUIC does not use DTLS. // Cipher changes are forbidden if the current epoch has leftover data. if (dtls_has_unprocessed_handshake_data(ssl)) { OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA); @@ -97,7 +99,9 @@ static bool dtls1_set_read_state(SSL *ssl, ssl_encryption_level_t level, } static bool dtls1_set_write_state(SSL *ssl, ssl_encryption_level_t level, - UniquePtr aead_ctx) { + UniquePtr aead_ctx, + Span secret_for_quic) { + assert(secret_for_quic.empty()); // QUIC does not use DTLS. ssl->d1->w_epoch++; OPENSSL_memcpy(ssl->d1->last_write_sequence, ssl->s3->write_sequence, sizeof(ssl->s3->write_sequence)); diff --git a/ssl/internal.h b/ssl/internal.h index 91036ae9b..ac3a8448c 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -2140,15 +2140,19 @@ struct SSL_PROTOCOL_METHOD { // on_handshake_complete is called when the handshake is complete. void (*on_handshake_complete)(SSL *ssl); // set_read_state sets |ssl|'s read cipher state and level to |aead_ctx| and - // |level|. It returns true on success and false if changing the read state is - // forbidden at this point. + // |level|. In QUIC, |aead_ctx| is a placeholder object and |secret_for_quic| + // is the original secret. This function returns true on success and false on + // error. bool (*set_read_state)(SSL *ssl, ssl_encryption_level_t level, - UniquePtr aead_ctx); + UniquePtr aead_ctx, + Span secret_for_quic); // set_write_state sets |ssl|'s write cipher state and level to |aead_ctx| and - // |level|. It returns true on success and false if changing the write state - // is forbidden at this point. + // |level|. In QUIC, |aead_ctx| is a placeholder object and |secret_for_quic| + // is the original secret. This function returns true on success and false on + // error. bool (*set_write_state)(SSL *ssl, ssl_encryption_level_t level, - UniquePtr aead_ctx); + UniquePtr aead_ctx, + Span secret_for_quic); }; // The following wrappers call |open_*| but handle |read_shutdown| correctly. diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 22934054c..cc6fdee1a 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -4850,6 +4850,8 @@ class MockQUICTransport { return !levels_[level].write_secret.empty(); } + void AllowOutOfOrderWrites() { allow_out_of_order_writes_ = true; } + bool SetReadSecret(ssl_encryption_level_t level, const SSL_CIPHER *cipher, Span secret) { if (HasReadSecret(level)) { @@ -4915,6 +4917,35 @@ class MockQUICTransport { << " write secret not yet configured"; return false; } + + // Although the levels are conceptually separate, BoringSSL finishes writing + // data from a previous level before installing keys for the next level. + if (!allow_out_of_order_writes_) { + switch (level) { + case ssl_encryption_early_data: + ADD_FAILURE() << "unexpected handshake data at early data level"; + return false; + case ssl_encryption_initial: + if (!levels_[ssl_encryption_handshake].write_secret.empty()) { + ADD_FAILURE() + << LevelToString(level) + << " handshake data written after handshake keys installed"; + return false; + } + OPENSSL_FALLTHROUGH; + case ssl_encryption_handshake: + if (!levels_[ssl_encryption_application].write_secret.empty()) { + ADD_FAILURE() + << LevelToString(level) + << " handshake data written after application keys installed"; + return false; + } + OPENSSL_FALLTHROUGH; + case ssl_encryption_application: + break; + } + } + levels_[level].write_data.insert(levels_[level].write_data.end(), data.begin(), data.end()); return true; @@ -4971,6 +5002,7 @@ class MockQUICTransport { Role role_; MockQUICTransport *peer_ = nullptr; + bool allow_out_of_order_writes_ = false; bool has_alert_ = false; ssl_encryption_level_t alert_level_ = ssl_encryption_initial; uint8_t alert_ = 0; @@ -5048,6 +5080,10 @@ class QUICMethodTest : public testing::Test { SSL_provide_quic_data(ssl, level, data.data(), data.size()); } + void AllowOutOfOrderWrites() { + allow_out_of_order_writes_ = true; + } + bool CreateClientAndServer() { client_.reset(SSL_new(client_ctx_.get())); server_.reset(SSL_new(server_ctx_.get())); @@ -5061,6 +5097,10 @@ class QUICMethodTest : public testing::Test { transport_.reset(new MockQUICTransportPair); ex_data_.Set(client_.get(), transport_->client()); ex_data_.Set(server_.get(), transport_->server()); + if (allow_out_of_order_writes_) { + transport_->client()->AllowOutOfOrderWrites(); + transport_->server()->AllowOutOfOrderWrites(); + } return true; } @@ -5183,6 +5223,8 @@ class QUICMethodTest : public testing::Test { bssl::UniquePtr client_; bssl::UniquePtr server_; + + bool allow_out_of_order_writes_ = false; }; UnownedSSLExData QUICMethodTest::ex_data_; @@ -5486,6 +5528,8 @@ TEST_F(QUICMethodTest, Async) { // Test buffering write data until explicit flushes. TEST_F(QUICMethodTest, Buffered) { + AllowOutOfOrderWrites(); + struct BufferedFlight { std::vector data[kNumQUICLevels]; }; @@ -5535,6 +5579,8 @@ TEST_F(QUICMethodTest, Buffered) { // EncryptedExtensions in a single chunk, BoringSSL notices and rejects this on // key change. TEST_F(QUICMethodTest, ExcessProvidedData) { + AllowOutOfOrderWrites(); + auto add_handshake_data = [](SSL *ssl, enum ssl_encryption_level_t level, const uint8_t *data, size_t len) -> int { // Switch everything to the initial level. @@ -5580,10 +5626,10 @@ TEST_F(QUICMethodTest, ExcessProvidedData) { EXPECT_EQ(transport_->client()->alert_level(), ssl_encryption_handshake); EXPECT_EQ(transport_->client()->alert(), SSL_AD_UNEXPECTED_MESSAGE); - // Sanity-check client did get far enough to process the ServerHello and - // install keys. - EXPECT_TRUE(transport_->client()->HasReadSecret(ssl_encryption_handshake)); + // Sanity-check handshake secrets. The error is discovered while setting the + // read secret, so only the write secret has been installed. EXPECT_TRUE(transport_->client()->HasWriteSecret(ssl_encryption_handshake)); + EXPECT_FALSE(transport_->client()->HasReadSecret(ssl_encryption_handshake)); } // Test that |SSL_provide_quic_data| will reject data at the wrong level. diff --git a/ssl/t1_enc.cc b/ssl/t1_enc.cc index 80910219a..73b6544a8 100644 --- a/ssl/t1_enc.cc +++ b/ssl/t1_enc.cc @@ -237,11 +237,13 @@ int tls1_configure_aead(SSL *ssl, evp_aead_direction_t direction, if (direction == evp_aead_open) { return ssl->method->set_read_state(ssl, ssl_encryption_application, - std::move(aead_ctx)); + std::move(aead_ctx), + /*secret_for_quic=*/{}); } return ssl->method->set_write_state(ssl, ssl_encryption_application, - std::move(aead_ctx)); + std::move(aead_ctx), + /*secret_for_quic=*/{}); } int tls1_change_cipher_state(SSL_HANDSHAKE *hs, diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index 722847172..b889ac283 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc @@ -79,8 +79,10 @@ static bool close_early_data(SSL_HANDSHAKE *hs, ssl_encryption_level_t level) { if (level == ssl_encryption_initial) { bssl::UniquePtr null_ctx = SSLAEADContext::CreateNullCipher(SSL_is_dtls(ssl)); - if (!null_ctx || !ssl->method->set_write_state(ssl, ssl_encryption_initial, - std::move(null_ctx))) { + if (!null_ctx || + !ssl->method->set_write_state(ssl, ssl_encryption_initial, + std::move(null_ctx), + /*secret_for_quic=*/{})) { return false; } ssl->s3->aead_write_ctx->SetVersionIfNullCipher(ssl->version); diff --git a/ssl/tls13_enc.cc b/ssl/tls13_enc.cc index 3a2e4e5d3..69a55788c 100644 --- a/ssl/tls13_enc.cc +++ b/ssl/tls13_enc.cc @@ -143,33 +143,13 @@ bool tls13_set_traffic_key(SSL *ssl, enum ssl_encryption_level_t level, Span traffic_secret) { uint16_t version = ssl_session_protocol_version(session); UniquePtr traffic_aead; + Span secret_for_quic; if (ssl->quic_method != nullptr) { - // Pass the traffic secrets to QUIC. - if (direction == evp_aead_open) { - if (!ssl->quic_method->set_read_secret(ssl, level, session->cipher, - traffic_secret.data(), - traffic_secret.size())) { - return false; - } - } else { - if (!ssl->quic_method->set_write_secret(ssl, level, session->cipher, - traffic_secret.data(), - traffic_secret.size())) { - return false; - } - } - - // QUIC only uses |ssl| for handshake messages, which never use early data - // keys, so we return installing anything. This avoids needing to have two - // secrets active at once in 0-RTT. - if (level == ssl_encryption_early_data) { - return true; - } - // Install a placeholder SSLAEADContext so that SSL accessors work. The // encryption itself will be handled by the SSL_QUIC_METHOD. traffic_aead = SSLAEADContext::CreatePlaceholderForQUIC(version, session->cipher); + secret_for_quic = traffic_secret; } else { // Look up cipher suite properties. const EVP_AEAD *aead; @@ -217,14 +197,16 @@ bool tls13_set_traffic_key(SSL *ssl, enum ssl_encryption_level_t level, } if (direction == evp_aead_open) { - if (!ssl->method->set_read_state(ssl, level, std::move(traffic_aead))) { + if (!ssl->method->set_read_state(ssl, level, std::move(traffic_aead), + secret_for_quic)) { return false; } OPENSSL_memmove(ssl->s3->read_traffic_secret, traffic_secret.data(), traffic_secret.size()); ssl->s3->read_traffic_secret_len = traffic_secret.size(); } else { - if (!ssl->method->set_write_state(ssl, level, std::move(traffic_aead))) { + if (!ssl->method->set_write_state(ssl, level, std::move(traffic_aead), + secret_for_quic)) { return false; } OPENSSL_memmove(ssl->s3->write_traffic_secret, traffic_secret.data(), diff --git a/ssl/tls_method.cc b/ssl/tls_method.cc index 3868852d1..8165d1c92 100644 --- a/ssl/tls_method.cc +++ b/ssl/tls_method.cc @@ -83,7 +83,8 @@ static void tls_on_handshake_complete(SSL *ssl) { } static bool tls_set_read_state(SSL *ssl, ssl_encryption_level_t level, - UniquePtr aead_ctx) { + UniquePtr aead_ctx, + Span secret_for_quic) { // Cipher changes are forbidden if the current epoch has leftover data. if (tls_has_unprocessed_handshake_data(ssl)) { OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA); @@ -91,6 +92,21 @@ static bool tls_set_read_state(SSL *ssl, ssl_encryption_level_t level, return false; } + if (ssl->quic_method != nullptr) { + if (!ssl->quic_method->set_read_secret(ssl, level, aead_ctx->cipher(), + secret_for_quic.data(), + secret_for_quic.size())) { + return false; + } + + // QUIC only uses |ssl| for handshake messages, which never use early data + // keys, so we return without installing anything. This avoids needing to + // have two secrets active at once in 0-RTT. + if (level == ssl_encryption_early_data) { + return true; + } + } + OPENSSL_memset(ssl->s3->read_sequence, 0, sizeof(ssl->s3->read_sequence)); ssl->s3->aead_read_ctx = std::move(aead_ctx); ssl->s3->read_level = level; @@ -98,11 +114,27 @@ static bool tls_set_read_state(SSL *ssl, ssl_encryption_level_t level, } static bool tls_set_write_state(SSL *ssl, ssl_encryption_level_t level, - UniquePtr aead_ctx) { + UniquePtr aead_ctx, + Span secret_for_quic) { if (!tls_flush_pending_hs_data(ssl)) { return false; } + if (ssl->quic_method != nullptr) { + if (!ssl->quic_method->set_write_secret(ssl, level, aead_ctx->cipher(), + secret_for_quic.data(), + secret_for_quic.size())) { + return false; + } + + // QUIC only uses |ssl| for handshake messages, which never use early data + // keys, so we return without installing anything. This avoids needing to + // have two secrets active at once in 0-RTT. + if (level == ssl_encryption_early_data) { + return true; + } + } + OPENSSL_memset(ssl->s3->write_sequence, 0, sizeof(ssl->s3->write_sequence)); ssl->s3->aead_write_ctx = std::move(aead_ctx); ssl->s3->write_level = level;