Prefer established session properties mid renegotiation.
Among many many problems with renegotiation is it makes every API
ambiguous. Do we return the pending handshake's properties, or the most
recently completed handshake? Neither answer is unambiguously correct:
On the one hand, OpenSSL's API makes renegotiation transparent, so the
pending handshake breaks invariants. E.g., currently,
SSL_get_current_cipher and other functions can return NULL mid
renegotiation. See https://crbug.com/1010748.
On the other hand, OpenSSL's API is callback-heavy. During a handshake
callback, the application most likely wants to check the pending
parameters. Most notably, cert verify callbacks calling
SSL_get_peer_certificate.
Historically, only the pending state was available to return anyway.
We've since changed this
(https://boringssl-review.googlesource.com/8612), but we kept the public
APIs as-is. I was particularly worried about cert verify callbacks.
As of https://boringssl-review.googlesource.com/c/boringssl/+/14028/ and
https://boringssl-review.googlesource.com/c/boringssl/+/19665/, cert
verify is moot. We implement the 3-SHAKE mitigation in library, so the
peer cert cannot change, and we don't reverify the certificate at all.
With that, I think we should switch to returning the established
parameters. Chromium is the main consumer that enables renegotiation,
and it would be better off with this behavior. (Maybe we should try to
forbid other properties, like the cipher suite, from changing on
renegotiation. Unchangeable properties make this issue moot.)
This CL would break if the handshake internally used SSL_get_session,
but this is no longer true as of
https://boringssl-review.googlesource.com/c/boringssl/+/41865.
Update-Note: Some APIs will now behave differently mid-renegotation. I
think this is the safer option, but it is possible code was relying on
the other one.
Fixed: chromium:1010748
Change-Id: I42157ccd9704cde3eebf947136d47cda6754c36e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54165
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 633a15a..d0a8ad6 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4135,6 +4135,13 @@
// renegotiation attempts by a server. If |ssl| is a server, peer-initiated
// renegotiations are *always* rejected and this function does nothing.
//
+// WARNING: Renegotiation is error-prone, complicates TLS's security properties,
+// and increases its attack surface. When enabled, many common assumptions about
+// BoringSSL's behavior no longer hold, and the calling application must handle
+// more cases. Renegotiation is also incompatible with many application
+// protocols, e.g. section 9.2.1 of RFC 7540. Many functions behave in ambiguous
+// or undefined ways during a renegotiation.
+//
// The renegotiation mode defaults to |ssl_renegotiate_never|, but may be set
// at any point in a connection's lifetime. Set it to |ssl_renegotiate_once| to
// allow one renegotiation, |ssl_renegotiate_freely| to allow all
@@ -4156,6 +4163,20 @@
// e.g., ALPN must enable renegotiation before the handshake and conditionally
// disable it afterwards.
//
+// When enabled, renegotiation can cause properties of |ssl|, such as the cipher
+// suite, to change during the lifetime of the connection. More over, during a
+// renegotiation, not all properties of the new handshake are available or fully
+// established. In BoringSSL, most functions, such as |SSL_get_current_cipher|,
+// report information from the most recently completed handshake, not the
+// pending one. However, renegotiation may rerun handshake callbacks, such as
+// |SSL_CTX_set_cert_cb|. Such callbacks must ensure they are acting on the
+// desired versions of each property.
+//
+// BoringSSL does not reverify peer certificates on renegotiation and instead
+// requires they match between handshakes, so certificate verification callbacks
+// (see |SSL_CTX_set_custom_verify|) may assume |ssl| is in the initial
+// handshake and use |SSL_get0_peer_certificates|, etc.
+//
// There is no support in BoringSSL for initiating renegotiations as a client
// or server.
OPENSSL_EXPORT void SSL_set_renegotiate_mode(SSL *ssl,
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 3b06bf0..82acb65 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -1950,8 +1950,6 @@
}
uint16_t SSL_get_curve_id(const SSL *ssl) {
- // TODO(davidben): This checks the wrong session if there is a renegotiation
- // in progress.
SSL_SESSION *session = SSL_get_session(ssl);
if (session == NULL) {
return 0;
@@ -2835,8 +2833,6 @@
}
uint16_t SSL_get_peer_signature_algorithm(const SSL *ssl) {
- // TODO(davidben): This checks the wrong session if there is a renegotiation
- // in progress.
SSL_SESSION *session = SSL_get_session(ssl);
if (session == NULL) {
return 0;
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 76e6dc4..05bcf6f 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -1169,20 +1169,31 @@
}
SSL_SESSION *SSL_get_session(const SSL *ssl) {
- // Once the handshake completes we return the established session. Otherwise
- // we return the intermediate session, either |session| (for resumption) or
- // |new_session| if doing a full handshake.
- if (!SSL_in_init(ssl)) {
+ // Once the initially handshake completes, we return the most recently
+ // established session. In particular, if there is a pending renegotiation, we
+ // do not return information about it until it completes.
+ //
+ // Code in the handshake must either use |hs->new_session| (if updating a
+ // partial session) or |ssl_handshake_session| (if trying to query properties
+ // consistently across TLS 1.2 resumption and other handshakes).
+ if (ssl->s3->established_session != nullptr) {
return ssl->s3->established_session.get();
}
+
+ // Otherwise, we must be in the initial handshake.
SSL_HANDSHAKE *hs = ssl->s3->hs.get();
+ assert(hs != nullptr);
+ assert(!ssl->s3->initial_handshake_complete);
+
+ // Return the 0-RTT session, if in the 0-RTT state. While the handshake has
+ // not actually completed, the public accessors all report properties as if
+ // it has.
if (hs->early_session) {
return hs->early_session.get();
}
- if (hs->new_session) {
- return hs->new_session.get();
- }
- return ssl->session.get();
+
+ // Otherwise, return the partial session.
+ return (SSL_SESSION *)ssl_handshake_session(hs);
}
SSL_SESSION *SSL_get1_session(SSL *ssl) {
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 9f488d5..2627362 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -7468,33 +7468,11 @@
EXPECT_EQ(Bytes(SessionIDOf(client.get())), Bytes(SessionIDOf(server.get())));
}
-TEST(SSLTest, WriteWhileExplicitRenegotiate) {
- bssl::UniquePtr<SSL_CTX> ctx(CreateContextWithTestCertificate(TLS_method()));
- ASSERT_TRUE(ctx);
-
- ASSERT_TRUE(SSL_CTX_set_min_proto_version(ctx.get(), TLS1_2_VERSION));
- ASSERT_TRUE(SSL_CTX_set_max_proto_version(ctx.get(), TLS1_2_VERSION));
- ASSERT_TRUE(SSL_CTX_set_strict_cipher_list(
- ctx.get(), "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256"));
-
- bssl::UniquePtr<SSL> client, server;
- ASSERT_TRUE(CreateClientAndServer(&client, &server, ctx.get(), ctx.get()));
- SSL_set_renegotiate_mode(client.get(), ssl_renegotiate_explicit);
- ASSERT_TRUE(CompleteHandshakes(client.get(), server.get()));
-
- static const uint8_t kInput[] = {'h', 'e', 'l', 'l', 'o'};
-
- // Write "hello" until the buffer is full, so |client| has a pending write.
- size_t num_writes = 0;
- for (;;) {
- int ret = SSL_write(client.get(), kInput, sizeof(kInput));
- if (ret != int(sizeof(kInput))) {
- ASSERT_EQ(-1, ret);
- ASSERT_EQ(SSL_ERROR_WANT_WRITE, SSL_get_error(client.get(), ret));
- break;
- }
- num_writes++;
- }
+static void WriteHelloRequest(SSL *server) {
+ // This function assumes TLS 1.2 with ChaCha20-Poly1305.
+ ASSERT_EQ(SSL_version(server), TLS1_2_VERSION);
+ ASSERT_EQ(SSL_CIPHER_get_cipher_nid(SSL_get_current_cipher(server)),
+ NID_chacha20_poly1305);
// Encrypt a HelloRequest.
uint8_t in[] = {SSL3_MT_HELLO_REQUEST, 0, 0, 0};
@@ -7511,16 +7489,15 @@
// Extract key material from |server|.
static const size_t kKeyLen = 32;
static const size_t kNonceLen = 12;
- ASSERT_EQ(2u * (kKeyLen + kNonceLen), SSL_get_key_block_len(server.get()));
+ ASSERT_EQ(2u * (kKeyLen + kNonceLen), SSL_get_key_block_len(server));
uint8_t key_block[2u * (kKeyLen + kNonceLen)];
- ASSERT_TRUE(
- SSL_generate_key_block(server.get(), key_block, sizeof(key_block)));
+ ASSERT_TRUE(SSL_generate_key_block(server, key_block, sizeof(key_block)));
Span<uint8_t> key = MakeSpan(key_block + kKeyLen, kKeyLen);
Span<uint8_t> nonce =
MakeSpan(key_block + kKeyLen + kKeyLen + kNonceLen, kNonceLen);
uint8_t ad[13];
- uint64_t seq = SSL_get_write_sequence(server.get());
+ uint64_t seq = SSL_get_write_sequence(server);
for (size_t i = 0; i < 8; i++) {
// The nonce is XORed with the sequence number.
nonce[11 - i] ^= uint8_t(seq);
@@ -7553,7 +7530,38 @@
#endif // BORINGSSL_UNSAFE_FUZZER_MODE
ASSERT_EQ(int(sizeof(record)),
- BIO_write(SSL_get_wbio(server.get()), record, sizeof(record)));
+ BIO_write(SSL_get_wbio(server), record, sizeof(record)));
+}
+
+TEST(SSLTest, WriteWhileExplicitRenegotiate) {
+ bssl::UniquePtr<SSL_CTX> ctx(CreateContextWithTestCertificate(TLS_method()));
+ ASSERT_TRUE(ctx);
+
+ ASSERT_TRUE(SSL_CTX_set_min_proto_version(ctx.get(), TLS1_2_VERSION));
+ ASSERT_TRUE(SSL_CTX_set_max_proto_version(ctx.get(), TLS1_2_VERSION));
+ ASSERT_TRUE(SSL_CTX_set_strict_cipher_list(
+ ctx.get(), "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256"));
+
+ bssl::UniquePtr<SSL> client, server;
+ ASSERT_TRUE(CreateClientAndServer(&client, &server, ctx.get(), ctx.get()));
+ SSL_set_renegotiate_mode(client.get(), ssl_renegotiate_explicit);
+ ASSERT_TRUE(CompleteHandshakes(client.get(), server.get()));
+
+ static const uint8_t kInput[] = {'h', 'e', 'l', 'l', 'o'};
+
+ // Write "hello" until the buffer is full, so |client| has a pending write.
+ size_t num_writes = 0;
+ for (;;) {
+ int ret = SSL_write(client.get(), kInput, sizeof(kInput));
+ if (ret != int(sizeof(kInput))) {
+ ASSERT_EQ(-1, ret);
+ ASSERT_EQ(SSL_ERROR_WANT_WRITE, SSL_get_error(client.get(), ret));
+ break;
+ }
+ num_writes++;
+ }
+
+ ASSERT_NO_FATAL_FAILURE(WriteHelloRequest(server.get()));
// |SSL_read| should pick up the HelloRequest.
uint8_t byte;
@@ -7595,6 +7603,57 @@
EXPECT_EQ(SSL_R_NO_RENEGOTIATION, ERR_GET_REASON(err));
}
+TEST(SSLTest, ConnectionPropertiesDuringRenegotiate) {
+ // Configure known connection properties, so we can check against them.
+ bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
+ ASSERT_TRUE(ctx);
+ bssl::UniquePtr<X509> cert = GetTestCertificate();
+ ASSERT_TRUE(cert);
+ bssl::UniquePtr<EVP_PKEY> key = GetTestKey();
+ ASSERT_TRUE(key);
+ ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), cert.get()));
+ ASSERT_TRUE(SSL_CTX_use_PrivateKey(ctx.get(), key.get()));
+ ASSERT_TRUE(SSL_CTX_set_min_proto_version(ctx.get(), TLS1_2_VERSION));
+ ASSERT_TRUE(SSL_CTX_set_max_proto_version(ctx.get(), TLS1_2_VERSION));
+ ASSERT_TRUE(SSL_CTX_set_strict_cipher_list(
+ ctx.get(), "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256"));
+ ASSERT_TRUE(SSL_CTX_set1_curves_list(ctx.get(), "X25519"));
+ ASSERT_TRUE(SSL_CTX_set1_sigalgs_list(ctx.get(), "rsa_pkcs1_sha256"));
+
+ // Connect a client and server that accept renegotiation.
+ bssl::UniquePtr<SSL> client, server;
+ ASSERT_TRUE(CreateClientAndServer(&client, &server, ctx.get(), ctx.get()));
+ SSL_set_renegotiate_mode(client.get(), ssl_renegotiate_freely);
+ ASSERT_TRUE(CompleteHandshakes(client.get(), server.get()));
+
+ auto check_properties = [&] {
+ EXPECT_EQ(SSL_version(client.get()), TLS1_2_VERSION);
+ const SSL_CIPHER *cipher = SSL_get_current_cipher(client.get());
+ ASSERT_TRUE(cipher);
+ EXPECT_EQ(SSL_CIPHER_get_id(cipher),
+ uint32_t{TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256});
+ EXPECT_EQ(SSL_get_curve_id(client.get()), SSL_CURVE_X25519);
+ EXPECT_EQ(SSL_get_peer_signature_algorithm(client.get()),
+ SSL_SIGN_RSA_PKCS1_SHA256);
+ bssl::UniquePtr<X509> peer(SSL_get_peer_certificate(client.get()));
+ ASSERT_TRUE(peer);
+ EXPECT_EQ(X509_cmp(cert.get(), peer.get()), 0);
+ };
+ check_properties();
+
+ // The server sends a HelloRequest.
+ ASSERT_NO_FATAL_FAILURE(WriteHelloRequest(server.get()));
+
+ // Reading from the client will consume the HelloRequest, start a
+ // renegotiation, and then block on a ServerHello from the server.
+ uint8_t byte;
+ ASSERT_EQ(-1, SSL_read(client.get(), &byte, 1));
+ ASSERT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(client.get(), -1));
+
+ // Connection properties should continue to report values from the original
+ // handshake.
+ check_properties();
+}
TEST(SSLTest, CopyWithoutEarlyData) {
bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
