Enforce compression_method in TLS 1.3 draft 22.
Change-Id: Ic99a949258e62cad168c2c39507ca63100a8ffe5
Reviewed-on: https://boringssl-review.googlesource.com/23264
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 583aceb..7b1c097 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -295,33 +295,14 @@
return 0;
}
- // Renegotiations do not participate in session resumption.
- int has_session_id = ssl->session != NULL &&
- !ssl->s3->initial_handshake_complete &&
- ssl->session->session_id_length > 0;
-
CBB child;
if (!CBB_add_u16(&body, hs->client_version) ||
!CBB_add_bytes(&body, ssl->s3->client_random, SSL3_RANDOM_SIZE) ||
- !CBB_add_u8_length_prefixed(&body, &child)) {
+ !CBB_add_u8_length_prefixed(&body, &child) ||
+ !CBB_add_bytes(&child, hs->session_id, hs->session_id_len)) {
return 0;
}
- if (has_session_id) {
- if (!CBB_add_bytes(&child, ssl->session->session_id,
- ssl->session->session_id_length)) {
- return 0;
- }
- } else {
- // In TLS 1.3 experimental encodings, send a fake placeholder session ID
- // when we do not otherwise have one to send.
- if (hs->max_version >= TLS1_3_VERSION &&
- ssl_is_resumption_variant(ssl->tls13_variant) &&
- !CBB_add_bytes(&child, hs->session_id, hs->session_id_len)) {
- return 0;
- }
- }
-
if (SSL_is_dtls(ssl)) {
if (!CBB_add_u8_length_prefixed(&body, &child) ||
!CBB_add_bytes(&child, ssl->d1->cookie, ssl->d1->cookie_len)) {
@@ -472,7 +453,13 @@
// Initialize a random session ID for the experimental TLS 1.3 variant
// requiring a session id.
- if (ssl_is_resumption_variant(ssl->tls13_variant)) {
+ if (ssl->session != nullptr &&
+ !ssl->s3->initial_handshake_complete &&
+ ssl->session->session_id_length > 0) {
+ hs->session_id_len = ssl->session->session_id_length;
+ OPENSSL_memcpy(hs->session_id, ssl->session->session_id,
+ hs->session_id_len);
+ } else if (ssl_is_resumption_variant(ssl->tls13_variant)) {
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/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index 22eb013..1fd84cb 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -1356,6 +1356,7 @@
isServerHello bool
sessionId []byte
cipherSuite uint16
+ compressionMethod uint8
hasSelectedGroup bool
selectedGroup CurveID
cookie []byte
@@ -1382,7 +1383,7 @@
sessionId := retryRequest.addU8LengthPrefixed()
sessionId.addBytes(m.sessionId)
retryRequest.addU16(m.cipherSuite)
- retryRequest.addU8(0)
+ retryRequest.addU8(m.compressionMethod)
} else {
retryRequest.addU16(m.vers)
if isDraft21(m.vers) {
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 4286911..595f8da 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -368,6 +368,7 @@
isDTLS: c.isDTLS,
vers: c.wireVersion,
sessionId: hs.clientHello.sessionId,
+ compressionMethod: config.Bugs.SendCompressionMethod,
versOverride: config.Bugs.SendServerHelloVersion,
supportedVersOverride: config.Bugs.SendServerSupportedExtensionVersion,
customExtension: config.Bugs.CustomUnencryptedExtension,
@@ -534,6 +535,7 @@
vers: c.wireVersion,
sessionId: hs.clientHello.sessionId,
cipherSuite: cipherSuite,
+ compressionMethod: config.Bugs.SendCompressionMethod,
duplicateExtensions: config.Bugs.DuplicateHelloRetryRequestExtensions,
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 8700af2..4f6fd13 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2785,6 +2785,34 @@
expectedLocalError: "remote error: illegal parameter",
},
{
+ testType: clientTest,
+ name: "TLS13Draft22-InvalidCompressionMethod",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ SendCompressionMethod: 1,
+ },
+ },
+ tls13Variant: TLS13Draft22,
+ shouldFail: true,
+ expectedError: ":DECODE_ERROR:",
+ },
+ {
+ testType: clientTest,
+ name: "TLS13Draft22-HRR-InvalidCompressionMethod",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ CurvePreferences: []CurveID{CurveP384},
+ Bugs: ProtocolBugs{
+ SendCompressionMethod: 1,
+ },
+ },
+ tls13Variant: TLS13Draft22,
+ shouldFail: true,
+ expectedError: ":DECODE_ERROR:",
+ expectedLocalError: "remote error: error decoding message",
+ },
+ {
name: "GREASE-Client-TLS12",
config: Config{
MaxVersion: VersionTLS12,
@@ -11017,6 +11045,23 @@
resumeSession: true,
})
+ // Test that the client correctly handles a TLS 1.3 ServerHello which echoes
+ // a TLS 1.2 session ID.
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ name: "TLS12SessionID-" + name,
+ config: Config{
+ MaxVersion: VersionTLS12,
+ SessionTicketsDisabled: true,
+ },
+ resumeConfig: &Config{
+ MaxVersion: VersionTLS13,
+ },
+ tls13Variant: variant,
+ resumeSession: true,
+ expectResumeRejected: true,
+ })
+
// Test that the server correctly echoes back session IDs of
// various lengths.
testCases = append(testCases, testCase{
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index 688fa06..f471a4e 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -75,11 +75,14 @@
CBS body = msg.body, server_random, session_id;
uint16_t server_version;
+ uint8_t compression_method;
if (!CBS_get_u16(&body, &server_version) ||
!CBS_get_bytes(&body, &server_random, SSL3_RANDOM_SIZE) ||
!CBS_get_u8_length_prefixed(&body, &session_id) ||
+ !CBS_mem_equal(&session_id, hs->session_id, hs->session_id_len) ||
!CBS_get_u16(&body, &cipher_suite) ||
- !CBS_skip(&body, 1) ||
+ !CBS_get_u8(&body, &compression_method) ||
+ compression_method != 0 ||
!CBS_get_u16_length_prefixed(&body, &extensions) ||
CBS_len(&extensions) == 0 ||
CBS_len(&body) != 0) {
@@ -251,7 +254,8 @@
if (!CBS_get_u16(&body, &server_version) ||
!CBS_get_bytes(&body, &server_random, SSL3_RANDOM_SIZE) ||
(ssl_is_resumption_experiment(ssl->version) &&
- !CBS_get_u8_length_prefixed(&body, &session_id)) ||
+ (!CBS_get_u8_length_prefixed(&body, &session_id) ||
+ !CBS_mem_equal(&session_id, hs->session_id, hs->session_id_len))) ||
!CBS_get_u16(&body, &cipher_suite) ||
(ssl_is_resumption_experiment(ssl->version) &&
(!CBS_get_u8(&body, &compression_method) || compression_method != 0)) ||
