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/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;