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>
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc index a4499a7..ac47189 100644 --- a/ssl/d1_both.cc +++ b/ssl/d1_both.cc
@@ -584,6 +584,11 @@ } 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>()); }
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 9788b46..3bfc7ae 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc
@@ -526,25 +526,28 @@ 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; } }
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index ab36e56..9260af0 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go
@@ -1965,6 +1965,10 @@ // 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
diff --git a/ssl/test/runner/dtls.go b/ssl/test/runner/dtls.go index b55f347..d407867 100644 --- a/ssl/test/runner/dtls.go +++ b/ssl/test/runner/dtls.go
@@ -80,25 +80,17 @@ } 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 @@ } 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 {
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index a7ab0e2..9362911 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go
@@ -1003,12 +1003,16 @@ 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 @@ } } - 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.") }
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index deb9ca5..9e647df 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go
@@ -521,11 +521,15 @@ // 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,
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 1476b38..623f277 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -3710,6 +3710,25 @@ 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 @@ } // 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 @@ } } 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{
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index 73e307c..0862a00 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc
@@ -111,11 +111,20 @@ 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;
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index d3147ea..f7804b2 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc
@@ -244,9 +244,15 @@ 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)) {