From 4c647a5623c3da9bfa8f9cd7d4f60c992b0685c3 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 27 Nov 2024 18:16:51 -0500 Subject: [PATCH] Implement the downgrade protection signal in DTLS 1.3 This was originally looking for the client to specifically support the final TLS 1.3 version 0x0304. This has a side effect of not picking up DTLS 1.3, which has a different codepoint. We did it this way because, early in TLS 1.3's development, we had draft versions of TLS 1.3 flying around, and only the final TLS 1.3 can safely ship enforcing this check. Those draft versions are now gone, and this check is now getting in the way of DTLS 1.3. Switch it to checking hs->max_version. Bug: 42290594 Change-Id: Ic2d143af965b4b8bafef524f3f0e85cc3efa42fe Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73728 Commit-Queue: David Benjamin Reviewed-by: Nick Harper --- ssl/handshake_client.cc | 2 +- ssl/handshake_server.cc | 2 +- ssl/test/runner/runner.go | 91 ++++++++++++++++++++------------------- 3 files changed, 49 insertions(+), 46 deletions(-) diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index b904e4f3cb..afc99be95c 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -796,7 +796,7 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { // Enforce the TLS 1.3 anti-downgrade feature. if (!ssl->s3->initial_handshake_complete && - ssl_supports_version(hs, TLS1_3_VERSION)) { + hs->max_version >= TLS1_3_VERSION) { static_assert( sizeof(kTLS12DowngradeRandom) == sizeof(kTLS13DowngradeRandom), "downgrade signals have different size"); diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index e41b23354c..843e2badfd 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc @@ -1057,7 +1057,7 @@ static enum ssl_hs_wait_t do_send_server_hello(SSL_HANDSHAKE *hs) { } // Implement the TLS 1.3 anti-downgrade feature. - if (ssl_supports_version(hs, TLS1_3_VERSION)) { + if (hs->max_version >= TLS1_3_VERSION) { if (ssl_protocol_version(ssl) == TLS1_2_VERSION) { if (hs->apply_jdk11_workaround) { // JDK 11 implements the TLS 1.3 downgrade signal, so we cannot send it diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 19e3740ed2..41fa4541a4 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -6891,8 +6891,9 @@ func addVersionNegotiationTests() { name: "MinorVersionTolerance-DTLS", config: Config{ Bugs: ProtocolBugs{ - SendClientVersion: 0xfe00, - OmitSupportedVersions: true, + SendClientVersion: 0xfe00, + OmitSupportedVersions: true, + IgnoreTLS13DowngradeRandom: true, }, }, expectations: connectionExpectations{ @@ -6905,8 +6906,9 @@ func addVersionNegotiationTests() { name: "MajorVersionTolerance-DTLS", config: Config{ Bugs: ProtocolBugs{ - SendClientVersion: 0xfdff, - OmitSupportedVersions: true, + SendClientVersion: 0xfdff, + OmitSupportedVersions: true, + IgnoreTLS13DowngradeRandom: true, }, }, expectations: connectionExpectations{ @@ -6953,49 +6955,50 @@ func addVersionNegotiationTests() { }) // Test TLS 1.3's downgrade signal. - 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, + for _, protocol := range []protocol{tls, dtls} { + for _, vers := range allVersions(protocol) { + if vers.version >= VersionTLS13 { + continue + } + clientShimError := "tls: downgrade from TLS 1.3 detected" + if vers.version < VersionTLS12 { + clientShimError = "tls: downgrade from TLS 1.2 detected" + } + // for _, test := range downgradeTests { + // The client should enforce the downgrade sentinel. + testCases = append(testCases, testCase{ + protocol: protocol, + name: "Downgrade-" + vers.name + "-Client-" + protocol.String(), + config: Config{ + Bugs: ProtocolBugs{ + NegotiateVersion: vers.wire(protocol), + }, }, - }, - expectations: connectionExpectations{ - version: test.version, - }, - shouldFail: true, - expectedError: ":TLS13_DOWNGRADE:", - expectedLocalError: "remote error: illegal parameter", - }) + expectations: connectionExpectations{ + version: vers.version, + }, + shouldFail: true, + expectedError: ":TLS13_DOWNGRADE:", + expectedLocalError: "remote error: illegal parameter", + }) - // 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}, + // The server should emit the downgrade signal. + testCases = append(testCases, testCase{ + protocol: protocol, + testType: serverTest, + name: "Downgrade-" + vers.name + "-Server-" + protocol.String(), + config: Config{ + Bugs: ProtocolBugs{ + SendSupportedVersions: []uint16{vers.wire(protocol)}, + }, }, - }, - expectations: connectionExpectations{ - version: test.version, - }, - shouldFail: true, - expectedLocalError: test.clientShimError, - }) + expectations: connectionExpectations{ + version: vers.version, + }, + shouldFail: true, + expectedLocalError: clientShimError, + }) + } } // SSL 3.0 support has been removed. Test that the shim does not