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 <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 2293405..cc6fdee 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -4850,6 +4850,8 @@
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<const uint8_t> secret) {
if (HasReadSecret(level)) {
@@ -4915,6 +4917,35 @@
<< " 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 @@
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 @@
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 @@
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 @@
bssl::UniquePtr<SSL> client_;
bssl::UniquePtr<SSL> server_;
+
+ bool allow_out_of_order_writes_ = false;
};
UnownedSSLExData<MockQUICTransport> QUICMethodTest::ex_data_;
@@ -5486,6 +5528,8 @@
// Test buffering write data until explicit flushes.
TEST_F(QUICMethodTest, Buffered) {
+ AllowOutOfOrderWrites();
+
struct BufferedFlight {
std::vector<uint8_t> data[kNumQUICLevels];
};
@@ -5535,6 +5579,8 @@
// 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 @@
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.
