Always check the TLS 1.3 downgrade signal. These APIs were used by Chromium to control the carve-out for the TLS 1.3 downgrade signal. As of https://chromium-review.googlesource.com/c/chromium/src/+/2324170, Chromium no longer uses them. Update-Note: SSL_CTX_set_ignore_tls13_downgrade, SSL_set_ignore_tls13_downgrade, and SSL_is_tls13_downgrade now do nothing. Calls sites should be removed. (There are some copies of older Chromium lying around, so I haven't removed the functions yet.) The enforcement was already on by default, so this CL does not affect callers that don't use those functions. Change-Id: I016af8291cd92051472d239c4650602fe2a68f5b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44124 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index c12aa0e..933c04a 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -4087,19 +4087,6 @@ OPENSSL_EXPORT void SSL_CTX_set_false_start_allowed_without_alpn(SSL_CTX *ctx, int allowed); -// SSL_CTX_set_ignore_tls13_downgrade configures whether connections on |ctx| -// ignore the downgrade signal in the server's random value. -OPENSSL_EXPORT void SSL_CTX_set_ignore_tls13_downgrade(SSL_CTX *ctx, - int ignore); - -// SSL_set_ignore_tls13_downgrade configures whether |ssl| ignores the downgrade -// signal in the server's random value. -OPENSSL_EXPORT void SSL_set_ignore_tls13_downgrade(SSL *ssl, int ignore); - -// SSL_is_tls13_downgrade returns one if the TLS 1.3 anti-downgrade -// mechanism would have aborted |ssl|'s handshake and zero otherwise. -OPENSSL_EXPORT int SSL_is_tls13_downgrade(const SSL *ssl); - // SSL_used_hello_retry_request returns one if the TLS 1.3 HelloRetryRequest // message has been either sent by the server or received by the client. It // returns zero otherwise. @@ -4776,6 +4763,18 @@ // name and remove this one. OPENSSL_EXPORT uint16_t SSL_CIPHER_get_value(const SSL_CIPHER *cipher); +// SSL_CTX_set_ignore_tls13_downgrade does nothing. +OPENSSL_EXPORT void SSL_CTX_set_ignore_tls13_downgrade(SSL_CTX *ctx, + int ignore); + +// SSL_set_ignore_tls13_downgrade does nothing. +OPENSSL_EXPORT void SSL_set_ignore_tls13_downgrade(SSL *ssl, int ignore); + +// SSL_is_tls13_downgrade returns zero. Historically, this function returned +// whether the TLS 1.3 downgrade signal would have been enforced if not +// disabled. The TLS 1.3 downgrade signal is now always enforced. +OPENSSL_EXPORT int SSL_is_tls13_downgrade(const SSL *ssl); + // Nodejs compatibility section (hidden). //
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 5c800fb..c36b192 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc
@@ -636,12 +636,9 @@ .subspan(SSL3_RANDOM_SIZE - sizeof(kTLS13DowngradeRandom)); if (suffix == kTLS12DowngradeRandom || suffix == kTLS13DowngradeRandom || suffix == kJDK11DowngradeRandom) { - ssl->s3->tls13_downgrade = true; - if (!hs->config->ignore_tls13_downgrade) { - OPENSSL_PUT_ERROR(SSL, SSL_R_TLS13_DOWNGRADE); - ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); - return ssl_hs_error; - } + OPENSSL_PUT_ERROR(SSL, SSL_R_TLS13_DOWNGRADE); + ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); + return ssl_hs_error; } } @@ -1550,18 +1547,12 @@ // // Now that TLS 1.3 exists, we would like to avoid similar attacks between // TLS 1.2 and TLS 1.3, but there are too many TLS 1.2 deployments to - // sacrifice False Start on them. TLS 1.3's downgrade signal fixes this, but - // |SSL_CTX_set_ignore_tls13_downgrade| can disable it due to compatibility - // issues. - // - // |SSL_CTX_set_ignore_tls13_downgrade| normally still retains Finished-based - // downgrade protection, but False Start bypasses that. Thus, we disable False - // Start based on the TLS 1.3 downgrade signal, even if otherwise unenforced. + // sacrifice False Start on them. Instead, we rely on the ServerHello.random + // downgrade signal, which we unconditionally enforce. if (SSL_is_dtls(ssl) || SSL_version(ssl) != TLS1_2_VERSION || hs->new_cipher->algorithm_mkey != SSL_kECDHE || - hs->new_cipher->algorithm_mac != SSL_AEAD || - ssl->s3->tls13_downgrade) { + hs->new_cipher->algorithm_mac != SSL_AEAD) { return false; }
diff --git a/ssl/internal.h b/ssl/internal.h index 7420f65..1e54c98 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -2413,9 +2413,6 @@ // early_data_accepted is true if early data was accepted by the server. bool early_data_accepted : 1; - // tls13_downgrade is whether the TLS 1.3 anti-downgrade logic fired. - bool tls13_downgrade : 1; - // token_binding_negotiated is set if Token Binding was negotiated. bool token_binding_negotiated : 1; @@ -2764,10 +2761,6 @@ // should be freed after the handshake completes. bool shed_handshake_config : 1; - // ignore_tls13_downgrade is whether the connection should continue when the - // server random signals a downgrade. - bool ignore_tls13_downgrade : 1; - // jdk11_workaround is whether to disable TLS 1.3 for JDK 11 clients, as a // workaround for https://bugs.openjdk.java.net/browse/JDK-8211806. bool jdk11_workaround : 1; @@ -3353,10 +3346,6 @@ // |SSL_MODE_ENABLE_FALSE_START| is enabled) is allowed without ALPN. bool false_start_allowed_without_alpn : 1; - // ignore_tls13_downgrade is whether a connection should continue when the - // server random signals a downgrade. - bool ignore_tls13_downgrade:1; - // handoff indicates that a server should stop after receiving the // ClientHello and pause the handshake in such a way that |SSL_get_error| // returns |SSL_ERROR_HANDOFF|.
diff --git a/ssl/s3_lib.cc b/ssl/s3_lib.cc index ee35604..3e12492 100644 --- a/ssl/s3_lib.cc +++ b/ssl/s3_lib.cc
@@ -177,7 +177,6 @@ key_update_pending(false), wpend_pending(false), early_data_accepted(false), - tls13_downgrade(false), token_binding_negotiated(false), alert_dispatch(false), renegotiate_pending(false),
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index a52f1fa..79eaacb 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc
@@ -565,7 +565,6 @@ grease_enabled(false), allow_unknown_alpn_protos(false), false_start_allowed_without_alpn(false), - ignore_tls13_downgrade(false), handoff(false), enable_early_data(false) { CRYPTO_MUTEX_init(&lock); @@ -711,7 +710,6 @@ ctx->signed_cert_timestamps_enabled; ssl->config->ocsp_stapling_enabled = ctx->ocsp_stapling_enabled; ssl->config->handoff = ctx->handoff; - ssl->config->ignore_tls13_downgrade = ctx->ignore_tls13_downgrade; ssl->quic_method = ctx->quic_method; if (!ssl->method->ssl_new(ssl.get()) || @@ -731,7 +729,6 @@ retain_only_sha256_of_client_certs(false), handoff(false), shed_handshake_config(false), - ignore_tls13_downgrade(false), jdk11_workaround(false) { assert(ssl); } @@ -2929,22 +2926,15 @@ ctx->false_start_allowed_without_alpn = !!allowed; } -int SSL_is_tls13_downgrade(const SSL *ssl) { return ssl->s3->tls13_downgrade; } +int SSL_is_tls13_downgrade(const SSL *ssl) { return 0; } int SSL_used_hello_retry_request(const SSL *ssl) { return ssl->s3->used_hello_retry_request; } -void SSL_CTX_set_ignore_tls13_downgrade(SSL_CTX *ctx, int ignore) { - ctx->ignore_tls13_downgrade = !!ignore; -} +void SSL_CTX_set_ignore_tls13_downgrade(SSL_CTX *ctx, int ignore) {} -void SSL_set_ignore_tls13_downgrade(SSL *ssl, int ignore) { - if (!ssl->config) { - return; - } - ssl->config->ignore_tls13_downgrade = !!ignore; -} +void SSL_set_ignore_tls13_downgrade(SSL *ssl, int ignore) {} void SSL_set_shed_handshake_config(SSL *ssl, int enable) { if (!ssl->config) {
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index b04f089..08482fb 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc
@@ -661,12 +661,6 @@ return false; } - if (config->expect_tls13_downgrade != !!SSL_is_tls13_downgrade(ssl)) { - fprintf(stderr, "Got %s downgrade signal, but wanted the opposite.\n", - SSL_is_tls13_downgrade(ssl) ? "" : "no "); - return false; - } - if (config->expect_delegated_credential_used != !!SSL_delegated_credential_used(ssl)) { fprintf(stderr,
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 631756f..1bbec86 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -6377,24 +6377,6 @@ expectedLocalError: "remote error: illegal parameter", }) - // The client should ignore the downgrade sentinel if - // configured. - testCases = append(testCases, testCase{ - name: "Downgrade-" + test.name + "-Client-Ignore", - config: Config{ - Bugs: ProtocolBugs{ - NegotiateVersion: test.version, - }, - }, - expectations: connectionExpectations{ - version: test.version, - }, - flags: []string{ - "-ignore-tls13-downgrade", - "-expect-tls13-downgrade", - }, - }) - // The server should emit the downgrade signal. testCases = append(testCases, testCase{ testType: serverTest, @@ -6412,31 +6394,6 @@ }) } - // Test that False Start is disabled when the downgrade logic triggers. - testCases = append(testCases, testCase{ - name: "Downgrade-FalseStart", - config: Config{ - NextProtos: []string{"foo"}, - Bugs: ProtocolBugs{ - NegotiateVersion: VersionTLS12, - ExpectFalseStart: true, - AlertBeforeFalseStartTest: alertAccessDenied, - }, - }, - expectations: connectionExpectations{ - version: VersionTLS12, - }, - flags: []string{ - "-false-start", - "-advertise-alpn", "\x03foo", - "-ignore-tls13-downgrade", - }, - shimWritesFirst: true, - shouldFail: true, - expectedError: ":TLSV1_ALERT_ACCESS_DENIED:", - expectedLocalError: "tls: peer did not false start: EOF", - }) - // SSL 3.0 support has been removed. Test that the shim does not // support it. testCases = append(testCases, testCase{
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index ca87c44..3c49b94 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc
@@ -131,8 +131,6 @@ {"-use-custom-verify-callback", &TestConfig::use_custom_verify_callback}, {"-allow-false-start-without-alpn", &TestConfig::allow_false_start_without_alpn}, - {"-ignore-tls13-downgrade", &TestConfig::ignore_tls13_downgrade}, - {"-expect-tls13-downgrade", &TestConfig::expect_tls13_downgrade}, {"-handoff", &TestConfig::handoff}, {"-use-ocsp-callback", &TestConfig::use_ocsp_callback}, {"-set-ocsp-in-callback", &TestConfig::set_ocsp_in_callback}, @@ -1328,10 +1326,6 @@ SSL_CTX_set_false_start_allowed_without_alpn(ssl_ctx.get(), 1); } - if (ignore_tls13_downgrade) { - SSL_CTX_set_ignore_tls13_downgrade(ssl_ctx.get(), 1); - } - if (use_ocsp_callback) { SSL_CTX_set_tlsext_status_cb(ssl_ctx.get(), LegacyOCSPCallback); }
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 318c733..35007b6 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h
@@ -159,8 +159,6 @@ bool use_custom_verify_callback = false; std::string expect_msg_callback; bool allow_false_start_without_alpn = false; - bool ignore_tls13_downgrade = false; - bool expect_tls13_downgrade = false; bool handoff = false; bool use_ocsp_callback = false; bool set_ocsp_in_callback = false;