From 5298ef99bf2b2d77600b3bb74dd572027bf495be Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 13 Mar 2020 12:17:30 -0400 Subject: [PATCH] Configure QUIC secrets inside set_{read,write}_state. set_write_state flushes buffered handshake data, and we should finish writing to a level before moving on to the next one. I've moved the callback into set_{read,write}_state to ensure we still update read_level and write_level after installing secrets, since that's how we decide what level to write things and we should never write alerts with keys we don't have. (I believe the only way this can come up is if the QUIC callback itself fails, but it still seems prudent to defer updating the levels.) This does unfortunately mean a goofy secret_for_quic parameter, though it is arguably more "correct" in that QUIC would ideally be a third SSL_PROTOCOL_METHOD, rather than escape hatches over TLS. Probably a cleaner abstraction would be for set_read_state and set_write_state to take the secret and derive an SSLAEADContext internally. Update-Note: See b/151142920#comment9 Change-Id: I4bbb76e15b5d95615ea643bccf796db87fae4989 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40244 Commit-Queue: David Benjamin Reviewed-by: Steven Valdez --- ssl/dtls_method.cc | 8 +++++-- ssl/internal.h | 16 ++++++++------ ssl/ssl_test.cc | 52 ++++++++++++++++++++++++++++++++++++++++++--- ssl/t1_enc.cc | 6 ++++-- ssl/tls13_client.cc | 6 ++++-- ssl/tls13_enc.cc | 30 ++++++-------------------- ssl/tls_method.cc | 36 +++++++++++++++++++++++++++++-- 7 files changed, 113 insertions(+), 41 deletions(-) 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;