Allow renego and config shedding to coexist more smoothly.
Chrome needs to support renegotiation at TLS 1.2 + HTTP/1.1, but we're
free to shed the handshake configuration at TLS 1.3 or HTTP/2.
Rather than making config shedding implicitly disable renegotiation,
make the actual shedding dependent on a combination of the two settings.
If config shedding is enabled, but so is renegotiation (including
whether we are a client, etc.), leave the config around. If the
renegotiation setting gets disabled again after the handshake,
re-evaluate and shed the config then.
Bug: 123
Change-Id: Ie833f413b3f15b8f0ede617991e3fef239d4a323
Reviewed-on: https://boringssl-review.googlesource.com/27904
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Matt Braithwaite <mab@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 2b87914..ee879c8 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -3304,10 +3304,17 @@
SSL_CTX *ctx, void (*cb)(const SSL *ssl, struct timeval *out_clock));
// SSL_set_shed_handshake_config allows some of the configuration of |ssl| to be
-// freed after its handshake completes. When configuration shedding is enabled,
+// freed after its handshake completes. When configuration shedding is enabled,
// it is an error to call APIs that query the state that was shed, and it is an
-// error to call |SSL_clear|. Additionally, if renegotiation is enabled,
-// renegotiation attempts will be rejected.
+// error to call |SSL_clear|.
+//
+// Note that configuration shedding as a client additionally depends on
+// renegotiation being disabled (see |SSL_set_renegotiate_mode|). If
+// renegotiation is possible, the configuration will be retained. If
+// configuration shedding is enabled and renegotiation later disabled after the
+// handshake, |SSL_set_renegotiate_mode| will shed configuration then. This may
+// be useful for clients which support renegotiation with some ALPN protocols,
+// such as HTTP/1.1, and not others, such as HTTP/2.
OPENSSL_EXPORT void SSL_set_shed_handshake_config(SSL *ssl, int enable);
enum ssl_renegotiate_mode_t {
@@ -3328,6 +3335,13 @@
// Note that ignoring HelloRequest messages may cause the connection to stall
// if the server waits for the renegotiation to complete.
//
+// 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
+// enabling it on a given connection. Callers that condition renegotiation on,
+// e.g., ALPN must enable renegotiation before the handshake and conditionally
+// disable it afterwards.
+//
// 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 1e5b3d1..7b80a03 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -459,6 +459,50 @@
ctx->handoff = on;
}
+static bool ssl_can_renegotiate(const SSL *ssl) {
+ if (ssl->server || SSL_is_dtls(ssl)) {
+ return false;
+ }
+
+ // We do not accept at SSL 3.0. SSL 3.0 will be removed entirely in the future
+ // and requires retaining more data for renegotiation_info.
+ uint16_t version = ssl_protocol_version(ssl);
+ if (version == SSL3_VERSION || version >= TLS1_3_VERSION) {
+ return false;
+ }
+
+ // The config has already been shed.
+ if (!ssl->config) {
+ return false;
+ }
+
+ switch (ssl->renegotiate_mode) {
+ case ssl_renegotiate_ignore:
+ case ssl_renegotiate_never:
+ return false;
+
+ case ssl_renegotiate_freely:
+ return true;
+ case ssl_renegotiate_once:
+ return ssl->s3->total_renegotiations == 0;
+ }
+
+ assert(0);
+ return false;
+}
+
+static void ssl_maybe_shed_handshake_config(SSL *ssl) {
+ if (ssl->s3->hs != nullptr ||
+ ssl->config == nullptr ||
+ !ssl->config->shed_handshake_config ||
+ ssl_can_renegotiate(ssl)) {
+ return;
+ }
+
+ Delete(ssl->config);
+ ssl->config = nullptr;
+}
+
} // namespace bssl
using namespace bssl;
@@ -881,13 +925,8 @@
// Destroy the handshake object if the handshake has completely finished.
if (!early_return) {
- if (hs->config->shed_handshake_config) {
- assert(ssl->config == hs->config);
- Delete(ssl->config);
- ssl->config = nullptr;
- hs->config = nullptr;
- }
ssl->s3->hs.reset();
+ ssl_maybe_shed_handshake_config(ssl);
}
return 1;
@@ -916,11 +955,12 @@
return tls13_post_handshake(ssl, msg);
}
- // We do not accept renegotiations as a server or SSL 3.0. SSL 3.0 will be
- // removed entirely in the future and requires retaining more data for
- // renegotiation_info.
- if (ssl->server || ssl->version == SSL3_VERSION) {
- goto no_renegotiation;
+ // Check for renegotiation on the server before parsing to use the correct
+ // error. Renegotiation is triggered by a different message for servers.
+ if (ssl->server) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_NO_RENEGOTIATION);
+ return 0;
}
if (msg.type != SSL3_MT_HELLO_REQUEST || CBS_len(&msg.body) != 0) {
@@ -929,36 +969,20 @@
return 0;
}
- switch (ssl->renegotiate_mode) {
- case ssl_renegotiate_ignore:
- // Ignore the HelloRequest.
- return 1;
-
- case ssl_renegotiate_once:
- if (ssl->s3->total_renegotiations != 0) {
- goto no_renegotiation;
- }
- break;
-
- case ssl_renegotiate_never:
- goto no_renegotiation;
-
- case ssl_renegotiate_freely:
- break;
+ if (ssl->renegotiate_mode == ssl_renegotiate_ignore) {
+ return 1; // Ignore the HelloRequest.
}
- // Reject renegotiation when the handshake config has been shed.
- if (!ssl->config) {
- goto no_renegotiation;
- }
-
- // 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() ||
+ 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) {
- goto no_renegotiation;
+ OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_NO_RENEGOTIATION);
+ return 0;
}
// Begin a new handshake.
@@ -973,11 +997,6 @@
ssl->s3->total_renegotiations++;
return 1;
-
-no_renegotiation:
- OPENSSL_PUT_ERROR(SSL, SSL_R_NO_RENEGOTIATION);
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_NO_RENEGOTIATION);
- return 0;
}
static int ssl_read_impl(SSL *ssl) {
@@ -2643,6 +2662,11 @@
void SSL_set_renegotiate_mode(SSL *ssl, enum ssl_renegotiate_mode_t mode) {
ssl->renegotiate_mode = mode;
+
+ // Check if |ssl_can_renegotiate| has changed and the configuration may now be
+ // shed. HTTP clients may initially allow renegotiation for HTTP/1.1, and then
+ // disable after the handshake once the ALPN protocol is known to be HTTP/2.
+ ssl_maybe_shed_handshake_config(ssl);
}
int SSL_get_ivs(const SSL *ssl, const uint8_t **out_read_iv,
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index dae759c..1b37a8d 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -2069,11 +2069,11 @@
SSL_set_shed_handshake_config(ssl.get(), true);
if (config->renegotiate_once) {
SSL_set_renegotiate_mode(ssl.get(), ssl_renegotiate_once);
- SSL_set_shed_handshake_config(ssl.get(), config->shed_despite_renegotiate);
}
- if (config->renegotiate_freely) {
+ if (config->renegotiate_freely ||
+ config->forbid_renegotiation_after_handshake) {
+ // |forbid_renegotiation_after_handshake| will disable renegotiation later.
SSL_set_renegotiate_mode(ssl.get(), ssl_renegotiate_freely);
- SSL_set_shed_handshake_config(ssl.get(), config->shed_despite_renegotiate);
}
if (config->renegotiate_ignore) {
SSL_set_renegotiate_mode(ssl.get(), ssl_renegotiate_ignore);
@@ -2378,6 +2378,10 @@
} while (config->async && RetryAsync(ssl, ret));
}
+ if (config->forbid_renegotiation_after_handshake) {
+ SSL_set_renegotiate_mode(ssl, ssl_renegotiate_never);
+ }
+
if (ret != 1 || !CheckHandshakeProperties(ssl, is_resume, config)) {
return false;
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index d85efca..38607d2 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -8209,14 +8209,15 @@
},
})
- // Renegotiation should be rejected if the handshake config has been shed.
+ // Renegotiation may be enabled and then disabled immediately after the
+ // handshake.
testCases = append(testCases, testCase{
- name: "Renegotiate-HandshakeConfigShed",
+ name: "Renegotiate-ForbidAfterHandshake",
config: Config{
MaxVersion: VersionTLS12,
},
renegotiate: 1,
- flags: []string{"-renegotiate-freely", "-shed-despite-renegotiate"},
+ flags: []string{"-forbid-renegotiation-after-handshake"},
shouldFail: true,
expectedError: ":NO_RENEGOTIATION:",
expectedLocalError: "remote error: no renegotiation",
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 9433e4c..2ab5bfe 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -98,10 +98,11 @@
{ "-verify-peer", &TestConfig::verify_peer },
{ "-verify-peer-if-no-obc", &TestConfig::verify_peer_if_no_obc },
{ "-expect-verify-result", &TestConfig::expect_verify_result },
- { "-shed-despite-renegotiate", &TestConfig::shed_despite_renegotiate },
{ "-renegotiate-once", &TestConfig::renegotiate_once },
{ "-renegotiate-freely", &TestConfig::renegotiate_freely },
{ "-renegotiate-ignore", &TestConfig::renegotiate_ignore },
+ { "-forbid-renegotiation-after-handshake",
+ &TestConfig::forbid_renegotiation_after_handshake },
{ "-p384-only", &TestConfig::p384_only },
{ "-enable-all-curves", &TestConfig::enable_all_curves },
{ "-use-old-client-cert-callback",
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index 4a03fba..9a026ce 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -115,7 +115,7 @@
bool renegotiate_once = false;
bool renegotiate_freely = false;
bool renegotiate_ignore = false;
- bool shed_despite_renegotiate = false;
+ bool forbid_renegotiation_after_handshake = false;
int expect_peer_signature_algorithm = 0;
bool p384_only = false;
bool enable_all_curves = false;
