Add an option for explicit renegotiations.
Chromium's renegotiation handling currently relies on reads being the only
thing that can discover a renegotiation. However, for a number of reasons, we
would like to eagerly drive the read loop after a handshake:
- 0-RTT + HTTP/1.1 will otherwise not pick up ServerHellos until after we send
a request. In particular, if we preconnect a 0-RTT socket sufficiently in
advance, such that the ServerHello comes in by the time we use it, we should
send 1-RTT data rather than 0-RTT.
- In TLS 1.2 False Start, if HTTP/1.1 or preconnect, we will not pick up the
server Finished and NewSessionTicket until later. This way we pick it up
sooner.
- If the server does not implement
https://boringssl-review.googlesource.com/c/boringssl/+/34948, this plugs the
theoretical deadlock on the client end. The False Start and 0-RTT scenarios
above also have theoretical deadlocks and cannot be mitigated on the server.
- TLS 1.3 client certificate alerts interact badly with TCP reset. Eagerly
reading from the socket makes it behave slightly better, though it's still
not reliable unless the server defers closing the socket.
So we can SSL_peek without triggering a renegotiation, add an
ssl_renegotiate_explicit mode to defer processing the renegotiation.
Bug: chromium:950706, chromium:958638
Change-Id: I78242d93d651b7a32a5c4c24ea9032ef63a027cf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/37944
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 6810a64..8cd03be 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -560,6 +560,13 @@
#define SSL_ERROR_HANDOFF 17
#define SSL_ERROR_HANDBACK 18
+// SSL_ERROR_WANT_RENEGOTIATE indicates the operation is pending a response to
+// a renegotiation request from the server. The caller may call
+// |SSL_renegotiate| to schedule a renegotiation and retry the operation.
+//
+// See also |ssl_renegotiate_explicit|.
+#define SSL_ERROR_WANT_RENEGOTIATE 19
+
// SSL_error_description returns a string representation of |err|, where |err|
// is one of the |SSL_ERROR_*| constants returned by |SSL_get_error|, or NULL
// if the value is unrecognized.
@@ -3605,6 +3612,7 @@
ssl_renegotiate_once,
ssl_renegotiate_freely,
ssl_renegotiate_ignore,
+ ssl_renegotiate_explicit,
};
// SSL_set_renegotiate_mode configures how |ssl|, a client, reacts to
@@ -3618,6 +3626,13 @@
// Note that ignoring HelloRequest messages may cause the connection to stall
// if the server waits for the renegotiation to complete.
//
+// If set to |ssl_renegotiate_explicit|, |SSL_read| and |SSL_peek| calls which
+// encounter a HelloRequest will pause with |SSL_ERROR_WANT_RENEGOTIATE|.
+// |SSL_write| will continue to work while paused. The caller may call
+// |SSL_renegotiate| to begin the renegotiation at a later point. This mode may
+// be used if callers wish to eagerly call |SSL_peek| without triggering a
+// renegotiation.
+//
// If configuration shedding is enabled (see |SSL_set_shed_handshake_config|),
// configuration is released if, at any point after the handshake, renegotiation
// is disabled. It is not possible to switch from disabling renegotiation to
@@ -3630,6 +3645,16 @@
OPENSSL_EXPORT void SSL_set_renegotiate_mode(SSL *ssl,
enum ssl_renegotiate_mode_t mode);
+// SSL_renegotiate starts a deferred renegotiation on |ssl| if it was configured
+// with |ssl_renegotiate_explicit| and has a pending HelloRequest. It returns
+// one on success and zero on error.
+//
+// This function does not do perform any I/O. On success, a subsequent
+// |SSL_do_handshake| call will run the handshake. |SSL_write| and
+// |SSL_read| will also complete the handshake before sending or receiving
+// application data.
+OPENSSL_EXPORT int SSL_renegotiate(SSL *ssl);
+
// SSL_renegotiate_pending returns one if |ssl| is in the middle of a
// renegotiation.
OPENSSL_EXPORT int SSL_renegotiate_pending(SSL *ssl);
@@ -4081,9 +4106,6 @@
// SSL_set_read_ahead returns one.
OPENSSL_EXPORT int SSL_set_read_ahead(SSL *ssl, int yes);
-// SSL_renegotiate put an error on the error queue and returns zero.
-OPENSSL_EXPORT int SSL_renegotiate(SSL *ssl);
-
// SSL_set_state does nothing.
OPENSSL_EXPORT void SSL_set_state(SSL *ssl, int state);
diff --git a/ssl/internal.h b/ssl/internal.h
index ec3594c..c635583 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -2294,6 +2294,10 @@
// alert_dispatch is true there is an alert in |send_alert| to be sent.
bool alert_dispatch : 1;
+ // renegotiate_pending is whether the read half of the channel is blocked on a
+ // HelloRequest.
+ bool renegotiate_pending : 1;
+
// hs_buf is the buffer of handshake data to process.
UniquePtr<BUF_MEM> hs_buf;
diff --git a/ssl/s3_lib.cc b/ssl/s3_lib.cc
index 41dd588..d7f8a85 100644
--- a/ssl/s3_lib.cc
+++ b/ssl/s3_lib.cc
@@ -181,7 +181,8 @@
tls13_downgrade(false),
token_binding_negotiated(false),
pq_experiment_signal_seen(false),
- alert_dispatch(false) {}
+ alert_dispatch(false),
+ renegotiate_pending(false) {}
SSL3_STATE::~SSL3_STATE() {}
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 1186312..3deac7d 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -478,6 +478,7 @@
return false;
case ssl_renegotiate_freely:
+ case ssl_renegotiate_explicit:
return true;
case ssl_renegotiate_once:
return ssl->s3->total_renegotiations == 0;
@@ -945,29 +946,16 @@
return 1; // Ignore the HelloRequest.
}
- if (!ssl_can_renegotiate(ssl) ||
- // Renegotiation is only supported at quiescent points in the application
- // protocol, namely in HTTPS, just before reading the HTTP response.
- // Require the record-layer be idle and avoid complexities of sending a
- // handshake record while an application_data record is being written.
- !ssl->s3->write_buffer.empty() ||
- ssl->s3->write_shutdown != ssl_shutdown_none) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
+ ssl->s3->renegotiate_pending = true;
+ if (ssl->renegotiate_mode == ssl_renegotiate_explicit) {
+ return 1; // Handle it later.
+ }
+
+ if (!SSL_renegotiate(ssl)) {
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_NO_RENEGOTIATION);
return 0;
}
- // Begin a new handshake.
- if (ssl->s3->hs != nullptr) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- return 0;
- }
- ssl->s3->hs = ssl_handshake_new(ssl);
- if (ssl->s3->hs == nullptr) {
- return 0;
- }
-
- ssl->s3->total_renegotiations++;
return 1;
}
@@ -1012,6 +1000,11 @@
}
while (ssl->s3->pending_app_data.empty()) {
+ if (ssl->s3->renegotiate_pending) {
+ ssl->s3->rwstate = SSL_ERROR_WANT_RENEGOTIATE;
+ return -1;
+ }
+
// Complete the current handshake, if any. False Start will cause
// |SSL_do_handshake| to return mid-handshake, so this may require multiple
// iterations.
@@ -1353,6 +1346,7 @@
case SSL_ERROR_PENDING_TICKET:
case SSL_ERROR_EARLY_DATA_REJECTED:
case SSL_ERROR_WANT_CERTIFICATE_VERIFY:
+ case SSL_ERROR_WANT_RENEGOTIATE:
return ssl->s3->rwstate;
case SSL_ERROR_WANT_READ: {
@@ -1743,8 +1737,39 @@
int SSL_renegotiate(SSL *ssl) {
// Caller-initiated renegotiation is not supported.
- OPENSSL_PUT_ERROR(SSL, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
- return 0;
+ if (!ssl->s3->renegotiate_pending) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
+ return 0;
+ }
+
+ if (!ssl_can_renegotiate(ssl)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
+ return 0;
+ }
+
+ // Renegotiation is only supported at quiescent points in the application
+ // protocol, namely in HTTPS, just before reading the HTTP response.
+ // Require the record-layer be idle and avoid complexities of sending a
+ // handshake record while an application_data record is being written.
+ if (!ssl->s3->write_buffer.empty() ||
+ ssl->s3->write_shutdown != ssl_shutdown_none) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
+ return 0;
+ }
+
+ // Begin a new handshake.
+ if (ssl->s3->hs != nullptr) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+ return 0;
+ }
+ ssl->s3->hs = ssl_handshake_new(ssl);
+ if (ssl->s3->hs == nullptr) {
+ return 0;
+ }
+
+ ssl->s3->renegotiate_pending = false;
+ ssl->s3->total_renegotiations++;
+ return 1;
}
int SSL_renegotiate_pending(SSL *ssl) {
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 2df005a..e702762 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -24,6 +24,7 @@
#include <gtest/gtest.h>
+#include <openssl/aead.h>
#include <openssl/base64.h>
#include <openssl/bio.h>
#include <openssl/cipher.h>
@@ -5586,5 +5587,139 @@
}
}
+TEST(SSLTest, WriteWhileExplicitRenegotiate) {
+ bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
+ ASSERT_TRUE(ctx);
+
+ bssl::UniquePtr<X509> cert = GetTestCertificate();
+ bssl::UniquePtr<EVP_PKEY> pkey = GetTestKey();
+ ASSERT_TRUE(cert);
+ ASSERT_TRUE(pkey);
+ ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), cert.get()));
+ ASSERT_TRUE(SSL_CTX_use_PrivateKey(ctx.get(), pkey.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"));
+
+ bssl::UniquePtr<SSL> client, server;
+ ASSERT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get(),
+ ClientConfig(), true /* do_handshake */,
+ false /* don't shed handshake config */));
+ SSL_set_renegotiate_mode(client.get(), ssl_renegotiate_explicit);
+
+ 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++;
+ }
+
+ // Encrypt a HelloRequest.
+ uint8_t in[] = {SSL3_MT_HELLO_REQUEST, 0, 0, 0};
+#if defined(BORINGSSL_UNSAFE_FUZZER_MODE)
+ // Fuzzer-mode records are unencrypted.
+ uint8_t record[5 + sizeof(in)];
+ record[0] = SSL3_RT_HANDSHAKE;
+ record[1] = 3;
+ record[2] = 3; // TLS 1.2
+ record[3] = 0;
+ record[4] = sizeof(record) - 5;
+ memcpy(record + 5, in, sizeof(in));
+#else
+ // 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()));
+ uint8_t key_block[2u * (kKeyLen + kNonceLen)];
+ ASSERT_TRUE(
+ SSL_generate_key_block(server.get(), 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());
+ for (size_t i = 0; i < 8; i++) {
+ // The nonce is XORed with the sequence number.
+ nonce[11 - i] ^= uint8_t(seq);
+ ad[7 - i] = uint8_t(seq);
+ seq >>= 8;
+ }
+
+ ad[8] = SSL3_RT_HANDSHAKE;
+ ad[9] = 3;
+ ad[10] = 3; // TLS 1.2
+ ad[11] = 0;
+ ad[12] = sizeof(in);
+
+ uint8_t record[5 + sizeof(in) + 16];
+ record[0] = SSL3_RT_HANDSHAKE;
+ record[1] = 3;
+ record[2] = 3; // TLS 1.2
+ record[3] = 0;
+ record[4] = sizeof(record) - 5;
+
+ ScopedEVP_AEAD_CTX aead;
+ ASSERT_TRUE(EVP_AEAD_CTX_init(aead.get(), EVP_aead_chacha20_poly1305(),
+ key.data(), key.size(),
+ EVP_AEAD_DEFAULT_TAG_LENGTH, nullptr));
+ size_t len;
+ ASSERT_TRUE(EVP_AEAD_CTX_seal(aead.get(), record + 5, &len,
+ sizeof(record) - 5, nonce.data(), nonce.size(),
+ in, sizeof(in), ad, sizeof(ad)));
+ ASSERT_EQ(sizeof(record) - 5, len);
+#endif // BORINGSSL_UNSAFE_FUZZER_MODE
+
+ ASSERT_EQ(int(sizeof(record)),
+ BIO_write(SSL_get_wbio(server.get()), record, sizeof(record)));
+
+ // |SSL_read| should pick up the HelloRequest.
+ uint8_t byte;
+ ASSERT_EQ(-1, SSL_read(client.get(), &byte, 1));
+ ASSERT_EQ(SSL_ERROR_WANT_RENEGOTIATE, SSL_get_error(client.get(), -1));
+
+ // Drain the data from the |client|.
+ uint8_t buf[sizeof(kInput)];
+ for (size_t i = 0; i < num_writes; i++) {
+ ASSERT_EQ(int(sizeof(buf)), SSL_read(server.get(), buf, sizeof(buf)));
+ EXPECT_EQ(Bytes(buf), Bytes(kInput));
+ }
+
+ // |client| should be able to finish the pending write and continue to write,
+ // despite the paused HelloRequest.
+ ASSERT_EQ(int(sizeof(kInput)),
+ SSL_write(client.get(), kInput, sizeof(kInput)));
+ ASSERT_EQ(int(sizeof(buf)), SSL_read(server.get(), buf, sizeof(buf)));
+ EXPECT_EQ(Bytes(buf), Bytes(kInput));
+
+ ASSERT_EQ(int(sizeof(kInput)),
+ SSL_write(client.get(), kInput, sizeof(kInput)));
+ ASSERT_EQ(int(sizeof(buf)), SSL_read(server.get(), buf, sizeof(buf)));
+ EXPECT_EQ(Bytes(buf), Bytes(kInput));
+
+ // |SSL_read| is stuck until we acknowledge the HelloRequest.
+ ASSERT_EQ(-1, SSL_read(client.get(), &byte, 1));
+ ASSERT_EQ(SSL_ERROR_WANT_RENEGOTIATE, SSL_get_error(client.get(), -1));
+
+ ASSERT_TRUE(SSL_renegotiate(client.get()));
+ ASSERT_EQ(-1, SSL_read(client.get(), &byte, 1));
+ ASSERT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(client.get(), -1));
+
+ // We never renegotiate as a server.
+ ASSERT_EQ(-1, SSL_read(server.get(), buf, sizeof(buf)));
+ ASSERT_EQ(SSL_ERROR_SSL, SSL_get_error(server.get(), -1));
+ uint32_t err = ERR_get_error();
+ EXPECT_EQ(ERR_LIB_SSL, ERR_GET_LIB(err));
+ EXPECT_EQ(SSL_R_NO_RENEGOTIATION, ERR_GET_REASON(err));
+}
+
} // namespace
BSSL_NAMESPACE_END
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 261f6c6..9bd389b 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -1126,6 +1126,15 @@
return false;
}
+ if (config->renegotiate_explicit &&
+ SSL_total_renegotiations(ssl) !=
+ GetTestState(ssl)->explicit_renegotiates) {
+ fprintf(stderr, "Performed %d renegotiations, but triggered %d of them\n",
+ SSL_total_renegotiations(ssl),
+ GetTestState(ssl)->explicit_renegotiates);
+ return false;
+ }
+
return true;
}
diff --git a/ssl/test/handshake_util.cc b/ssl/test/handshake_util.cc
index 4b1dcc8..fe96751 100644
--- a/ssl/test/handshake_util.cc
+++ b/ssl/test/handshake_util.cc
@@ -40,8 +40,18 @@
bool RetryAsync(SSL *ssl, int ret) {
const TestConfig *config = GetTestConfig(ssl);
TestState *test_state = GetTestState(ssl);
- // No error or not async; don't retry.
- if (ret >= 0 || !config->async) {
+ if (ret >= 0) {
+ return false;
+ }
+
+ int ssl_err = SSL_get_error(ssl, ret);
+ if (ssl_err == SSL_ERROR_WANT_RENEGOTIATE && config->renegotiate_explicit) {
+ test_state->explicit_renegotiates++;
+ return SSL_renegotiate(ssl);
+ }
+
+ if (!config->async) {
+ // Only asynchronous tests should trigger other retries.
return false;
}
@@ -62,7 +72,7 @@
// See if we needed to read or write more. If so, allow one byte through on
// the appropriate end to maximally stress the state machine.
- switch (SSL_get_error(ssl, ret)) {
+ switch (ssl_err) {
case SSL_ERROR_WANT_READ:
AsyncBioAllowRead(test_state->async_bio, 1);
return true;
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 5a4b0cc..758566a 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -5268,6 +5268,18 @@
})
tests = append(tests, testCase{
+ name: "Renegotiate-Client-Explicit",
+ config: Config{
+ MaxVersion: VersionTLS12,
+ },
+ renegotiate: 1,
+ flags: []string{
+ "-renegotiate-explicit",
+ "-expect-total-renegotiations", "1",
+ },
+ })
+
+ tests = append(tests, testCase{
name: "SendHalfHelloRequest",
config: Config{
MaxVersion: VersionTLS12,
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index bd32ce9..8d8a068 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -102,6 +102,7 @@
{"-renegotiate-once", &TestConfig::renegotiate_once},
{"-renegotiate-freely", &TestConfig::renegotiate_freely},
{"-renegotiate-ignore", &TestConfig::renegotiate_ignore},
+ {"-renegotiate-explicit", &TestConfig::renegotiate_explicit},
{"-forbid-renegotiation-after-handshake",
&TestConfig::forbid_renegotiation_after_handshake},
{"-enable-all-curves", &TestConfig::enable_all_curves},
@@ -1577,6 +1578,9 @@
if (renegotiate_ignore) {
SSL_set_renegotiate_mode(ssl.get(), ssl_renegotiate_ignore);
}
+ if (renegotiate_explicit) {
+ SSL_set_renegotiate_mode(ssl.get(), ssl_renegotiate_explicit);
+ }
if (!check_close_notify) {
SSL_set_quiet_shutdown(ssl.get(), 1);
}
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index ce4b416..8c25ed2 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -119,6 +119,7 @@
bool renegotiate_once = false;
bool renegotiate_freely = false;
bool renegotiate_ignore = false;
+ bool renegotiate_explicit = false;
bool forbid_renegotiation_after_handshake = false;
int expect_peer_signature_algorithm = 0;
bool enable_all_curves = false;
diff --git a/ssl/test/test_state.h b/ssl/test/test_state.h
index 2364286..2aa9e30 100644
--- a/ssl/test/test_state.h
+++ b/ssl/test/test_state.h
@@ -61,6 +61,7 @@
// cert_verified is true if certificate verification has been driven to
// completion. This tests that the callback is not called again after this.
bool cert_verified = false;
+ int explicit_renegotiates = 0;
};
bool SetTestState(SSL *ssl, std::unique_ptr<TestState> state);
