Add a per-SSL TLS 1.3 downgrade enforcement option and improve tests. Due to non-compliant middleboxes, it is possible we'll need to do some surgery to this mechanism. Making it per-SSL is a little more flexible and also eases some tests in Chromium until we get its SSL_CTX usage fixed up. Also fix up BoringSSL tests. We forgot to test it at TLS 1.0 and use the -expect-tls13-downgrade flag. Bug: 226 Change-Id: Ib39227e74e2d6f5e1fbc1ebcc091e751471b3cdc Reviewed-on: https://boringssl-review.googlesource.com/c/32424 Reviewed-by: Steven Valdez <svaldez@google.com> Commit-Queue: David Benjamin <davidben@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index ae96bcf..e46b39f 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc
@@ -600,7 +600,7 @@ .subspan(SSL3_RANDOM_SIZE - sizeof(kTLS13DowngradeRandom)); if (suffix == kTLS12DowngradeRandom || suffix == kTLS13DowngradeRandom) { ssl->s3->tls13_downgrade = true; - if (!ssl->ctx->ignore_tls13_downgrade) { + 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;
diff --git a/ssl/internal.h b/ssl/internal.h index 0535b8d..561b5d9 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -2456,6 +2456,10 @@ // shed_handshake_config indicates that the handshake config (this object!) // 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; }; // From RFC 8446, used in determining PSK modes.
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 13b9cac..9c16de4 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc
@@ -693,6 +693,7 @@ 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; if (!ssl->method->ssl_new(ssl.get()) || !ssl->ctx->x509_method->ssl_new(ssl->s3->hs.get())) { @@ -709,7 +710,8 @@ channel_id_enabled(false), retain_only_sha256_of_client_certs(false), handoff(false), - shed_handshake_config(false) { + shed_handshake_config(false), + ignore_tls13_downgrade(false) { assert(ssl); } @@ -2642,6 +2644,13 @@ ctx->ignore_tls13_downgrade = !!ignore; } +void SSL_set_ignore_tls13_downgrade(SSL *ssl, int ignore) { + if (!ssl->config) { + return; + } + ssl->config->ignore_tls13_downgrade = !!ignore; +} + void SSL_set_shed_handshake_config(SSL *ssl, int enable) { if (!ssl->config) { return;
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 4bcf603..9631e6e 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -5891,59 +5891,65 @@ }) // Test TLS 1.3's downgrade signal. - testCases = append(testCases, testCase{ - name: "Downgrade-TLS12-Client", - config: Config{ - Bugs: ProtocolBugs{ - NegotiateVersion: VersionTLS12, - }, - }, - tls13Variant: TLS13RFC, - expectedVersion: VersionTLS12, - shouldFail: true, - expectedError: ":TLS13_DOWNGRADE:", - expectedLocalError: "remote error: illegal parameter", - }) - testCases = append(testCases, testCase{ - testType: serverTest, - name: "Downgrade-TLS12-Server", - config: Config{ - Bugs: ProtocolBugs{ - SendSupportedVersions: []uint16{VersionTLS12}, - }, - }, - tls13Variant: TLS13RFC, - expectedVersion: VersionTLS12, - shouldFail: true, - expectedLocalError: "tls: downgrade from TLS 1.3 detected", - }) + var downgradeTests = []struct { + name string + version uint16 + clientShimError string + }{ + {"TLS12", VersionTLS12, "tls: downgrade from TLS 1.3 detected"}, + {"TLS11", VersionTLS11, "tls: downgrade from TLS 1.2 detected"}, + // TLS 1.0 does not have a dedicated value. + {"TLS10", VersionTLS10, "tls: downgrade from TLS 1.2 detected"}, + } - testCases = append(testCases, testCase{ - name: "Downgrade-TLS11-Client", - config: Config{ - Bugs: ProtocolBugs{ - NegotiateVersion: VersionTLS11, + for _, test := range downgradeTests { + // The client should enforce the downgrade sentinel. + testCases = append(testCases, testCase{ + name: "Downgrade-" + test.name + "-Client", + config: Config{ + Bugs: ProtocolBugs{ + NegotiateVersion: test.version, + }, }, - }, - tls13Variant: TLS13RFC, - expectedVersion: VersionTLS11, - shouldFail: true, - expectedError: ":TLS13_DOWNGRADE:", - expectedLocalError: "remote error: illegal parameter", - }) - testCases = append(testCases, testCase{ - testType: serverTest, - name: "Downgrade-TLS11-Server", - config: Config{ - Bugs: ProtocolBugs{ - SendSupportedVersions: []uint16{VersionTLS11}, + tls13Variant: TLS13RFC, + expectedVersion: test.version, + shouldFail: true, + expectedError: ":TLS13_DOWNGRADE:", + 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, + }, }, - }, - tls13Variant: TLS13RFC, - expectedVersion: VersionTLS11, - shouldFail: true, - expectedLocalError: "tls: downgrade from TLS 1.2 detected", - }) + tls13Variant: TLS13RFC, + expectedVersion: test.version, + flags: []string{ + "-ignore-tls13-downgrade", + "-expect-tls13-downgrade", + }, + }) + + // The server should emit the downgrade signal. + testCases = append(testCases, testCase{ + testType: serverTest, + name: "Downgrade-" + test.name + "-Server", + config: Config{ + Bugs: ProtocolBugs{ + SendSupportedVersions: []uint16{test.version}, + }, + }, + tls13Variant: TLS13RFC, + expectedVersion: test.version, + shouldFail: true, + expectedLocalError: test.clientShimError, + }) + } // Test that the draft TLS 1.3 variants don't trigger the downgrade logic. testCases = append(testCases, testCase{