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)) {
