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;
