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 <[email protected]> Reviewed-by: Nick Harper <[email protected]>
google · Dec 6, 2024 · 4c647a5 · 4c647a5
1 parent bbf03ea
commit 4c647a5
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 46 deletions.
diff --git 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
@@ -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
@@ -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