From 2d98d49cf712ca7dc6f4b23b9c5f5542385d8dbe Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 8 Oct 2018 17:31:17 -0500 Subject: [PATCH] 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 Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- include/openssl/ssl.h | 4 ++ ssl/handshake_client.cc | 2 +- ssl/internal.h | 4 ++ ssl/ssl_lib.cc | 11 +++- ssl/test/runner/runner.go | 104 ++++++++++++++++++++------------------ 5 files changed, 74 insertions(+), 51 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 117321a4..c0d44ce2 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -3667,6 +3667,10 @@ OPENSSL_EXPORT void SSL_CTX_set_false_start_allowed_without_alpn(SSL_CTX *ctx, 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); diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index ae96bcf2..e46b39f9 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -600,7 +600,7 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { .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 0535b8de..561b5d9b 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -2456,6 +2456,10 @@ struct SSL_CONFIG { // 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 13b9cacc..9c16de49 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -693,6 +693,7 @@ SSL *SSL_new(SSL_CTX *ctx) { 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 @@ SSL_CONFIG::SSL_CONFIG(SSL *ssl_arg) 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 @@ void SSL_CTX_set_ignore_tls13_downgrade(SSL_CTX *ctx, int ignore) { 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 4bcf6037..9631e6e2 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -5891,59 +5891,65 @@ func addVersionNegotiationTests() { }) // 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}, + 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"}, + } + + 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: VersionTLS12, - shouldFail: true, - expectedLocalError: "tls: downgrade from TLS 1.3 detected", - }) + tls13Variant: TLS13RFC, + expectedVersion: test.version, + shouldFail: true, + expectedError: ":TLS13_DOWNGRADE:", + expectedLocalError: "remote error: illegal parameter", + }) - testCases = append(testCases, testCase{ - name: "Downgrade-TLS11-Client", - config: Config{ - Bugs: ProtocolBugs{ - NegotiateVersion: VersionTLS11, + // 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, - 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, + flags: []string{ + "-ignore-tls13-downgrade", + "-expect-tls13-downgrade", }, - }, - tls13Variant: TLS13RFC, - expectedVersion: VersionTLS11, - shouldFail: true, - expectedLocalError: "tls: downgrade from TLS 1.2 detected", - }) + }) + + // 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{