Notice earlier if a server echoes the TLS 1.3 compatibility session ID.
Mono's legacy TLS 1.0 stack, as a server, does not implement any form of
resumption, but blindly echos the ClientHello session ID in the
ServerHello for no particularly good reason.
This is invalid, but due to quirks of how our client checked session ID
equality, we only noticed on the second connection, rather than the
first. Flaky failures do no one any good, so break deterministically on
the first connection, when we realize something strange is going on.
Bug: chromium:796910
Change-Id: I1f255e915fcdffeafb80be481f6c0acb3c628846
Reviewed-on: https://boringssl-review.googlesource.com/25424
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index eadb25d..4450958 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -134,6 +134,7 @@
SSL,206,SCSV_RECEIVED_WHEN_RENEGOTIATING
SSL,207,SERVERHELLO_TLSEXT
SSL,273,SERVER_CERT_CHANGED
+SSL,286,SERVER_ECHOED_INVALID_SESSION_ID
SSL,208,SESSION_ID_CONTEXT_UNINITIALIZED
SSL,209,SESSION_MAY_NOT_BE_CREATED
SSL,250,SHUTDOWN_WHILE_IN_INIT
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 7ae8276..247a145 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4616,6 +4616,7 @@
#define SSL_R_EARLY_DATA_NOT_IN_USE 283
#define SSL_R_HANDSHAKE_NOT_COMPLETE 284
#define SSL_R_NEGOTIATED_TB_WITHOUT_EMS_OR_RI 285
+#define SSL_R_SERVER_ECHOED_INVALID_SESSION_ID 286
#define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
#define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
#define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 3961ca1..1f91adb 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -653,6 +653,18 @@
ssl->session->session_id_length)) {
ssl->s3->session_reused = true;
} else {
+ // The server may also have echoed back the TLS 1.3 compatibility mode
+ // session ID. As we know this is not a session the server knows about, any
+ // server resuming it is in error. Reject the first connection
+ // deterministicly, rather than installing an invalid session into the
+ // session cache. https://crbug.com/796910
+ if (hs->session_id_len != 0 &&
+ CBS_mem_equal(&session_id, hs->session_id, hs->session_id_len)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_SERVER_ECHOED_INVALID_SESSION_ID);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return ssl_hs_error;
+ }
+
// The session wasn't resumed. Create a fresh SSL_SESSION to
// fill out.
ssl_set_session(ssl, NULL);
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index a543e81..d89f7fb 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -780,9 +780,13 @@
SendClientHelloSessionID []byte
// ExpectClientHelloSessionID, if true, causes the server to fail the
- // connection if there is not a SessionID in the ClientHello.
+ // connection if there is not a session ID in the ClientHello.
ExpectClientHelloSessionID bool
+ // EchoSessionIDInFullHandshake, if true, causes the server to echo the
+ // ClientHello session ID, even in TLS 1.2 full handshakes.
+ EchoSessionIDInFullHandshake bool
+
// ExpectNoTLS12Session, if true, causes the server to fail the
// connection if either a session ID or TLS 1.2 ticket is offered.
ExpectNoTLS12Session bool
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index dc74259..123ab16 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -1585,6 +1585,9 @@
return errors.New("tls: short read from Rand: " + err.Error())
}
}
+ if config.Bugs.EchoSessionIDInFullHandshake {
+ hs.hello.sessionId = hs.clientHello.sessionId
+ }
hs.finishedHash = newFinishedHash(c.wireVersion, c.isDTLS, hs.suite)
hs.writeClientHash(hs.clientHello.marshal())
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index e17f5dd..02ea539 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -3044,6 +3044,21 @@
shouldFail: true,
expectedError: ":EXCESSIVE_MESSAGE_SIZE:",
})
+
+ // Servers echoing the TLS 1.3 compatibility mode session ID should be
+ // rejected.
+ testCases = append(testCases, testCase{
+ name: "EchoTLS13CompatibilitySessionID",
+ config: Config{
+ MaxVersion: VersionTLS12,
+ Bugs: ProtocolBugs{
+ EchoSessionIDInFullHandshake: true,
+ },
+ },
+ shouldFail: true,
+ expectedError: ":SERVER_ECHOED_INVALID_SESSION_ID:",
+ expectedLocalError: "remote error: illegal parameter",
+ })
}
func addTestForCipherSuite(suite testCipherSuite, ver tlsVersion, protocol protocol) {
@@ -6184,14 +6199,14 @@
name: "TokenBinding-Server-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{0, 1, 2},
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{0, 1, 2},
TokenBindingVersion: maxTokenBindingVersion,
},
- expectTokenBinding: true,
+ expectTokenBinding: true,
expectedTokenBindingParam: 2,
- tls13Variant: ver.tls13Variant,
+ tls13Variant: ver.tls13Variant,
flags: []string{
"-token-binding-params",
base64.StdEncoding.EncodeToString([]byte{2, 1, 0}),
@@ -6204,9 +6219,9 @@
name: "TokenBinding-Server-UnsupportedParam-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{3},
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{3},
TokenBindingVersion: maxTokenBindingVersion,
},
tls13Variant: ver.tls13Variant,
@@ -6220,9 +6235,9 @@
name: "TokenBinding-Server-OldVersion-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{0, 1, 2},
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{0, 1, 2},
TokenBindingVersion: minTokenBindingVersion - 1,
},
tls13Variant: ver.tls13Variant,
@@ -6236,14 +6251,14 @@
name: "TokenBinding-Server-NewVersion-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{0, 1, 2},
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{0, 1, 2},
TokenBindingVersion: maxTokenBindingVersion + 1,
},
- expectTokenBinding: true,
+ expectTokenBinding: true,
expectedTokenBindingParam: 2,
- tls13Variant: ver.tls13Variant,
+ tls13Variant: ver.tls13Variant,
flags: []string{
"-token-binding-params",
base64.StdEncoding.EncodeToString([]byte{2, 1, 0}),
@@ -6256,9 +6271,9 @@
name: "TokenBinding-Server-NoParams-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{},
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{},
TokenBindingVersion: maxTokenBindingVersion,
},
tls13Variant: ver.tls13Variant,
@@ -6266,7 +6281,7 @@
"-token-binding-params",
base64.StdEncoding.EncodeToString([]byte{2, 1, 0}),
},
- shouldFail: true,
+ shouldFail: true,
expectedError: ":ERROR_PARSING_EXTENSION:",
})
testCases = append(testCases, testCase{
@@ -6274,14 +6289,14 @@
name: "TokenBinding-Server-RepeatedParam" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{0, 1, 2, 2},
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{0, 1, 2, 2},
TokenBindingVersion: maxTokenBindingVersion,
},
- expectTokenBinding: true,
+ expectTokenBinding: true,
expectedTokenBindingParam: 2,
- tls13Variant: ver.tls13Variant,
+ tls13Variant: ver.tls13Variant,
flags: []string{
"-token-binding-params",
base64.StdEncoding.EncodeToString([]byte{2, 1, 0}),
@@ -6294,10 +6309,10 @@
name: "TokenBinding-Client-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{2},
- TokenBindingVersion: maxTokenBindingVersion,
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{2},
+ TokenBindingVersion: maxTokenBindingVersion,
ExpectTokenBindingParams: []byte{0, 1, 2},
},
tls13Variant: ver.tls13Variant,
@@ -6313,13 +6328,13 @@
name: "TokenBinding-Client-Unexpected-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{2},
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{2},
TokenBindingVersion: maxTokenBindingVersion,
},
- tls13Variant: ver.tls13Variant,
- shouldFail: true,
+ tls13Variant: ver.tls13Variant,
+ shouldFail: true,
expectedError: ":UNEXPECTED_EXTENSION:",
})
testCases = append(testCases, testCase{
@@ -6327,10 +6342,10 @@
name: "TokenBinding-Client-ExtraParams-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{2, 1},
- TokenBindingVersion: maxTokenBindingVersion,
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{2, 1},
+ TokenBindingVersion: maxTokenBindingVersion,
ExpectTokenBindingParams: []byte{0, 1, 2},
},
flags: []string{
@@ -6339,8 +6354,8 @@
"-expected-token-binding-param",
"2",
},
- tls13Variant: ver.tls13Variant,
- shouldFail: true,
+ tls13Variant: ver.tls13Variant,
+ shouldFail: true,
expectedError: ":ERROR_PARSING_EXTENSION:",
})
testCases = append(testCases, testCase{
@@ -6348,10 +6363,10 @@
name: "TokenBinding-Client-NoParams-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{},
- TokenBindingVersion: maxTokenBindingVersion,
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{},
+ TokenBindingVersion: maxTokenBindingVersion,
ExpectTokenBindingParams: []byte{0, 1, 2},
},
flags: []string{
@@ -6360,8 +6375,8 @@
"-expected-token-binding-param",
"2",
},
- tls13Variant: ver.tls13Variant,
- shouldFail: true,
+ tls13Variant: ver.tls13Variant,
+ shouldFail: true,
expectedError: ":ERROR_PARSING_EXTENSION:",
})
testCases = append(testCases, testCase{
@@ -6369,10 +6384,10 @@
name: "TokenBinding-Client-WrongParam-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{3},
- TokenBindingVersion: maxTokenBindingVersion,
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{3},
+ TokenBindingVersion: maxTokenBindingVersion,
ExpectTokenBindingParams: []byte{0, 1, 2},
},
flags: []string{
@@ -6381,8 +6396,8 @@
"-expected-token-binding-param",
"2",
},
- tls13Variant: ver.tls13Variant,
- shouldFail: true,
+ tls13Variant: ver.tls13Variant,
+ shouldFail: true,
expectedError: ":ERROR_PARSING_EXTENSION:",
})
testCases = append(testCases, testCase{
@@ -6390,10 +6405,10 @@
name: "TokenBinding-Client-OldVersion-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{2},
- TokenBindingVersion: minTokenBindingVersion - 1,
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{2},
+ TokenBindingVersion: minTokenBindingVersion - 1,
ExpectTokenBindingParams: []byte{0, 1, 2},
},
flags: []string{
@@ -6407,10 +6422,10 @@
name: "TokenBinding-Client-MinVersion-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{2},
- TokenBindingVersion: minTokenBindingVersion,
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{2},
+ TokenBindingVersion: minTokenBindingVersion,
ExpectTokenBindingParams: []byte{0, 1, 2},
},
flags: []string{
@@ -6426,18 +6441,18 @@
name: "TokenBinding-Client-VersionTooNew-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{2},
- TokenBindingVersion: maxTokenBindingVersion + 1,
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{2},
+ TokenBindingVersion: maxTokenBindingVersion + 1,
ExpectTokenBindingParams: []byte{0, 1, 2},
},
flags: []string{
"-token-binding-params",
base64.StdEncoding.EncodeToString([]byte{0, 1, 2}),
},
- tls13Variant: ver.tls13Variant,
- shouldFail: true,
+ tls13Variant: ver.tls13Variant,
+ shouldFail: true,
expectedError: "ERROR_PARSING_EXTENSION",
})
if ver.version < VersionTLS13 {
@@ -6446,10 +6461,10 @@
name: "TokenBinding-Client-NoEMS-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{2},
- TokenBindingVersion: maxTokenBindingVersion,
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{2},
+ TokenBindingVersion: maxTokenBindingVersion,
ExpectTokenBindingParams: []byte{2, 1, 0},
Bugs: ProtocolBugs{
NoExtendedMasterSecret: true,
@@ -6460,7 +6475,7 @@
"-token-binding-params",
base64.StdEncoding.EncodeToString([]byte{2, 1, 0}),
},
- shouldFail: true,
+ shouldFail: true,
expectedError: ":NEGOTIATED_TB_WITHOUT_EMS_OR_RI:",
})
testCases = append(testCases, testCase{
@@ -6468,9 +6483,9 @@
name: "TokenBinding-Server-NoEMS-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{0, 1, 2},
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{0, 1, 2},
TokenBindingVersion: maxTokenBindingVersion,
Bugs: ProtocolBugs{
NoExtendedMasterSecret: true,
@@ -6481,7 +6496,7 @@
"-token-binding-params",
base64.StdEncoding.EncodeToString([]byte{2, 1, 0}),
},
- shouldFail: true,
+ shouldFail: true,
expectedError: ":NEGOTIATED_TB_WITHOUT_EMS_OR_RI:",
})
testCases = append(testCases, testCase{
@@ -6489,10 +6504,10 @@
name: "TokenBinding-Client-NoRI-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{2},
- TokenBindingVersion: maxTokenBindingVersion,
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{2},
+ TokenBindingVersion: maxTokenBindingVersion,
ExpectTokenBindingParams: []byte{2, 1, 0},
Bugs: ProtocolBugs{
NoRenegotiationInfo: true,
@@ -6503,7 +6518,7 @@
"-token-binding-params",
base64.StdEncoding.EncodeToString([]byte{2, 1, 0}),
},
- shouldFail: true,
+ shouldFail: true,
expectedError: ":NEGOTIATED_TB_WITHOUT_EMS_OR_RI:",
})
testCases = append(testCases, testCase{
@@ -6511,9 +6526,9 @@
name: "TokenBinding-Server-NoRI-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{0, 1, 2},
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{0, 1, 2},
TokenBindingVersion: maxTokenBindingVersion,
Bugs: ProtocolBugs{
NoRenegotiationInfo: true,
@@ -6532,38 +6547,38 @@
testType: clientTest,
name: "TokenBinding-WithEarlyDataFails-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{2},
- TokenBindingVersion: maxTokenBindingVersion,
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{2},
+ TokenBindingVersion: maxTokenBindingVersion,
ExpectTokenBindingParams: []byte{2, 1, 0},
- MaxEarlyDataSize: 16384,
+ MaxEarlyDataSize: 16384,
},
resumeSession: true,
- tls13Variant: ver.tls13Variant,
+ tls13Variant: ver.tls13Variant,
flags: []string{
"-enable-early-data",
"-expect-ticket-supports-early-data",
"-token-binding-params",
base64.StdEncoding.EncodeToString([]byte{2, 1, 0}),
},
- shouldFail: true,
+ shouldFail: true,
expectedError: ":UNEXPECTED_EXTENSION_ON_EARLY_DATA:",
})
testCases = append(testCases, testCase{
testType: serverTest,
name: "TokenBinding-EarlyDataRejected-" + ver.name,
config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- TokenBindingParams: []byte{0, 1, 2},
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ TokenBindingParams: []byte{0, 1, 2},
TokenBindingVersion: maxTokenBindingVersion,
- MaxEarlyDataSize: 16384,
+ MaxEarlyDataSize: 16384,
},
- resumeSession: true,
- expectTokenBinding: true,
+ resumeSession: true,
+ expectTokenBinding: true,
expectedTokenBindingParam: 2,
- tls13Variant: ver.tls13Variant,
+ tls13Variant: ver.tls13Variant,
flags: []string{
"-enable-early-data",
"-expect-ticket-supports-early-data",
@@ -7272,13 +7287,14 @@
})
} else {
error := ":OLD_SESSION_VERSION_NOT_RETURNED:"
-
- // Offering a TLS 1.3 session sends an empty session ID, so
- // there is no way to convince a non-lookahead client the
- // session was resumed. It will appear to the client that a
- // stray ChangeCipherSpec was sent.
+ // Clients offering TLS 1.3 will send a fake session ID
+ // unrelated to the session being offer. This session ID is
+ // invalid for the server to echo, so the handshake fails at
+ // a different point. It's not syntactically possible for a
+ // server to convince our client that it's accepted a TLS
+ // 1.3 session at an older version.
if resumeVers.version < VersionTLS13 && sessionVers.version >= VersionTLS13 {
- error = ":UNEXPECTED_RECORD:"
+ error = ":SERVER_ECHOED_INVALID_SESSION_ID:"
}
testCases = append(testCases, testCase{
