Disable compatibility mode for DTLS 1.3.

Bug: 715
Change-Id: I25509b0783852164006a156de98b1ac89f734e52
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69947
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

ssl/d1_both.cc

@@ -584,6 +584,11 @@ bool dtls1_add_message(SSL *ssl, Array<uint8_t> data) {
 }
 
 bool dtls1_add_change_cipher_spec(SSL *ssl) {
+  // DTLS 1.3 disables compatibility mode, which means that DTLS 1.3 never sends
+  // a ChangeCipherSpec message.
+  if (ssl_protocol_version(ssl) > TLS1_2_VERSION) {
+    return true;
+  }
   return add_outgoing(ssl, true /* ChangeCipherSpec */, Array<uint8_t>());
 }
 

ssl/handshake_client.cc

@@ -526,25 +526,28 @@ static enum ssl_hs_wait_t do_start_connect(SSL_HANDSHAKE *hs) {
     return ssl_hs_error;
   }
 
-  // Never send a session ID in QUIC. QUIC uses TLS 1.3 at a minimum and
-  // disables TLS 1.3 middlebox compatibility mode.
-  if (ssl->quic_method == nullptr) {
-    const bool has_id_session = ssl->session != nullptr &&
-                                ssl->session->session_id_length > 0 &&
-                                ssl->session->ticket.empty();
-    const bool has_ticket_session =
-        ssl->session != nullptr && !ssl->session->ticket.empty();
-    if (has_id_session) {
-      hs->session_id_len = ssl->session->session_id_length;
-      OPENSSL_memcpy(hs->session_id, ssl->session->session_id,
-                     hs->session_id_len);
-    } else if (has_ticket_session || hs->max_version >= TLS1_3_VERSION) {
-      // Send a random session ID. TLS 1.3 always sends one, and TLS 1.2 session
-      // tickets require a placeholder value to signal resumption.
-      hs->session_id_len = sizeof(hs->session_id);
-      if (!RAND_bytes(hs->session_id, hs->session_id_len)) {
-        return ssl_hs_error;
-      }
+  const bool has_id_session = ssl->session != nullptr &&
+                              ssl->session->session_id_length > 0 &&
+                              ssl->session->ticket.empty();
+  const bool has_ticket_session =
+      ssl->session != nullptr && !ssl->session->ticket.empty();
+  // TLS 1.2 session tickets require a placeholder value to signal resumption.
+  const bool ticket_session_requires_random_id =
+      has_ticket_session &&
+      ssl_session_protocol_version(ssl->session.get()) < TLS1_3_VERSION;
+  // Compatibility mode sends a random session ID. Compatibility mode is
+  // enabled for TLS 1.3, but not when it's run over QUIC or DTLS.
+  const bool enable_compatibility_mode = hs->max_version >= TLS1_3_VERSION &&
+                                         ssl->quic_method == nullptr &&
+                                         !SSL_is_dtls(hs->ssl);
+  if (has_id_session) {
+    hs->session_id_len = ssl->session->session_id_length;
+    OPENSSL_memcpy(hs->session_id, ssl->session->session_id,
+                   hs->session_id_len);
+  } else if (ticket_session_requires_random_id || enable_compatibility_mode) {
+    hs->session_id_len = sizeof(hs->session_id);
+    if (!RAND_bytes(hs->session_id, hs->session_id_len)) {
+      return ssl_hs_error;
     }
   }
 

ssl/test/runner/common.go

@@ -1965,6 +1965,10 @@ type ProtocolBugs struct {
 	// when running over QUIC.
 	CompatModeWithQUIC bool
 
+	// DTLS13EchoSessionID, if true, has DTLS 1.3 servers echo the client's
+	// session ID in the ServerHello.
+	DTLS13EchoSessionID bool
+
 	// EncryptSessionTicketKey, if non-nil, is the ticket key to use when
 	// encrypting tickets.
 	EncryptSessionTicketKey *[32]byte

ssl/test/runner/dtls.go

@@ -80,25 +80,17 @@ func (c *Conn) dtlsDoReadRecord(want recordType) (recordType, *block, error) {
 	}
 	epoch := b.data[3:5]
 	seq := b.data[5:11]
-	if c.expectTLS13ChangeCipherSpec && typ == recordTypeChangeCipherSpec {
-		// CCS should only be at epoch 0
-		if !bytes.Equal(epoch, []byte{0, 0}) {
-			c.sendAlert(alertIllegalParameter)
-			return 0, nil, c.in.setErrorLocked(fmt.Errorf("dtls: bad epoch, expected CCS at epoch 0"))
-		}
-	} else {
-		// For test purposes, require the sequence number be monotonically
-		// increasing, so c.in includes the minimum next sequence number. Gaps
-		// may occur if packets failed to be sent out. A real implementation
-		// would maintain a replay window and such.
-		if !bytes.Equal(epoch, c.in.seq[:2]) {
-			c.sendAlert(alertIllegalParameter)
-			return 0, nil, c.in.setErrorLocked(fmt.Errorf("dtls: bad epoch, want %x, got %x", c.in.seq[:2], epoch))
-		}
-		if bytes.Compare(seq, c.in.seq[2:]) < 0 {
-			c.sendAlert(alertIllegalParameter)
-			return 0, nil, c.in.setErrorLocked(fmt.Errorf("dtls: bad sequence number"))
-		}
+	// For test purposes, require the sequence number be monotonically
+	// increasing, so c.in includes the minimum next sequence number. Gaps
+	// may occur if packets failed to be sent out. A real implementation
+	// would maintain a replay window and such.
+	if !bytes.Equal(epoch, c.in.seq[:2]) {
+		c.sendAlert(alertIllegalParameter)
+		return 0, nil, c.in.setErrorLocked(fmt.Errorf("dtls: bad epoch, want %x, got %x", c.in.seq[:2], epoch))
+	}
+	if bytes.Compare(seq, c.in.seq[2:]) < 0 {
+		c.sendAlert(alertIllegalParameter)
+		return 0, nil, c.in.setErrorLocked(fmt.Errorf("dtls: bad sequence number"))
 	}
 	copy(c.in.seq[2:], seq)
 	n := int(b.data[11])<<8 | int(b.data[12])
@@ -108,21 +100,6 @@ func (c *Conn) dtlsDoReadRecord(want recordType) (recordType, *block, error) {
 	}
 	b, c.rawInput = c.in.splitBlock(b, recordHeaderLen+n)
 
-	// Process CCS
-	if c.expectTLS13ChangeCipherSpec {
-		c.expectTLS13ChangeCipherSpec = false
-		ccsData := b.data[recordHeaderLen:]
-		if typ == recordTypeChangeCipherSpec {
-			if !bytes.Equal(ccsData, []byte{1}) {
-				return 0, nil, c.in.setErrorLocked(fmt.Errorf("dtls: ChangeCipherSpec of length %d did not contain single byte value 0x01", len(ccsData)))
-			}
-			return c.dtlsDoReadRecord(want)
-		}
-		if typ != recordTypeAlert {
-			return 0, nil, c.in.setErrorLocked(fmt.Errorf("dtls: Expected ChangeCipherSpec but got record type %d", typ))
-		}
-	}
-
 	// Process message.
 	ok, off, _, alertValue := c.in.decrypt(b)
 	if !ok {

ssl/test/runner/handshake_client.go

@@ -1003,12 +1003,16 @@ func (hs *clientHandshakeState) doTLS13Handshake(msg any) error {
 	hs.finishedHash.discardHandshakeBuffer()
 
 	// The first server message must be followed by a ChangeCipherSpec.
-	c.expectTLS13ChangeCipherSpec = true
+	c.expectTLS13ChangeCipherSpec = !c.isDTLS
 
+	expectedSessionID := hs.hello.sessionID
+	if c.isDTLS {
+		expectedSessionID = nil
+	}
 	if haveHelloRetryRequest {
 		hs.writeServerHash(helloRetryRequest.marshal())
 
-		if !bytes.Equal(hs.hello.sessionID, helloRetryRequest.sessionID) {
+		if !bytes.Equal(expectedSessionID, helloRetryRequest.sessionID) {
 			return errors.New("tls: ClientHello and HelloRetryRequest session IDs did not match.")
 		}
 
@@ -1111,7 +1115,7 @@ func (hs *clientHandshakeState) doTLS13Handshake(msg any) error {
 		}
 	}
 
-	if !bytes.Equal(hs.hello.sessionID, hs.serverHello.sessionID) {
+	if !bytes.Equal(expectedSessionID, hs.serverHello.sessionID) {
 		return errors.New("tls: ClientHello and ServerHello session IDs did not match.")
 	}
 

ssl/test/runner/handshake_server.go

@@ -521,11 +521,15 @@ func (hs *serverHandshakeState) doTLS13Handshake() error {
 
 	// We've read the ClientHello, so the next record must be preceded with ChangeCipherSpec.
 	c.expectTLS13ChangeCipherSpec = true
+	sessionID := hs.clientHello.sessionID
+	if c.isDTLS && !config.Bugs.DTLS13EchoSessionID {
+		sessionID = nil
+	}
 
 	hs.hello = &serverHelloMsg{
 		isDTLS:                c.isDTLS,
 		vers:                  c.wireVersion,
-		sessionID:             hs.clientHello.sessionID,
+		sessionID:             sessionID,
 		compressionMethod:     config.Bugs.SendCompressionMethod,
 		versOverride:          config.Bugs.SendServerHelloVersion,
 		supportedVersOverride: config.Bugs.SendServerSupportedVersionExtension,

ssl/test/runner/runner.go

@@ -3710,6 +3710,25 @@ read alert 1 0
 		shouldFail:    true,
 		expectedError: ":UNEXPECTED_COMPATIBILITY_MODE:",
 	})
+
+	// Clients should reject DTLS 1.3 ServerHellos that echo the legacy
+	// session ID.
+	testCases = append(testCases, testCase{
+		protocol:      dtls,
+		name:          "DTLS13CompatibilityMode-EchoSessionID",
+		resumeSession: true,
+		config: Config{
+			MaxVersion: VersionTLS12,
+		},
+		resumeConfig: &Config{
+			MinVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				DTLS13EchoSessionID: true,
+			},
+		},
+		shouldFail:    true,
+		expectedError: ":DECODE_ERROR:",
+	})
 }
 
 func addTestForCipherSuite(suite testCipherSuite, ver tlsVersion, protocol protocol) {
@@ -4884,6 +4903,7 @@ func addStateMachineCoverageTests(config stateMachineTestConfig) {
 	}
 
 	// TLS 1.3 basic handshake shapes. DTLS 1.3 isn't supported yet.
+	// TODO(crbug.com/boringssl/715): Enable these tests.
 	if config.protocol != dtls {
 		tests = append(tests, testCase{
 			name: "TLS13-1RTT-Client",
@@ -6154,8 +6174,8 @@ read alert 1 0
 		}
 	}
 	if config.protocol == dtls {
-		// TODO(davidben): DTLS 1.3 will want a similar thing for
-		// HelloRetryRequest.
+		// TODO(crbug.com/boringssl/715): DTLS 1.3 will want a similar
+		// thing for HelloRetryRequest.
 		tests = append(tests, testCase{
 			name: "SkipHelloVerifyRequest",
 			config: Config{

ssl/tls13_client.cc

@@ -111,11 +111,20 @@ static bool parse_server_hello_tls13(const SSL_HANDSHAKE *hs,
   if (SSL_is_dtls(hs->ssl)) {
     server_hello_version = DTLS1_2_VERSION;
   }
+  // DTLS 1.3 disables "compatibility mode" (RFC 8446, appendix D.4). When
+  // disabled, servers MUST NOT echo the legacy_session_id (RFC 9147, section
+  // 5). The client could have sent a session ID indicating its willingness to
+  // resume a DTLS 1.2 session, so just checking that the session IDs match is
+  // incorrect.
+  bool session_id_match =
+      (SSL_is_dtls(hs->ssl) && CBS_len(&out->session_id) == 0) ||
+      (!SSL_is_dtls(hs->ssl) &&
+       CBS_mem_equal(&out->session_id, hs->session_id, hs->session_id_len));
+
   // The RFC8446 version of the structure fixes some legacy values.
   // Additionally, the session ID must echo the original one.
   if (out->legacy_version != server_hello_version ||
-      out->compression_method != 0 ||
-      !CBS_mem_equal(&out->session_id, hs->session_id, hs->session_id_len) ||
+      out->compression_method != 0 || !session_id_match ||
       CBS_len(&out->extensions) == 0) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
     *out_alert = SSL_AD_DECODE_ERROR;

ssl/tls13_server.cc

@@ -244,9 +244,15 @@ static enum ssl_hs_wait_t do_select_parameters(SSL_HANDSHAKE *hs) {
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
     return ssl_hs_error;
   }
-  OPENSSL_memcpy(hs->session_id, client_hello.session_id,
-                 client_hello.session_id_len);
-  hs->session_id_len = client_hello.session_id_len;
+  // DTLS 1.3 disables compatibility mode, and even if the client advertised a
+  // session ID (for resumption in DTLS 1.2), the server "MUST NOT echo the
+  // 'legacy_session_id' value from the client" (RFC 9147, section 5) as it
+  // would in a TLS 1.3 handshake.
+  if (!SSL_is_dtls(ssl)) {
+    OPENSSL_memcpy(hs->session_id, client_hello.session_id,
+                   client_hello.session_id_len);
+    hs->session_id_len = client_hello.session_id_len;
+  }
 
   Array<SSL_CREDENTIAL *> creds;
   if (!ssl_get_credential_list(hs, &creds)) {