Enforce supported_versions in the second ServerHello.
We forgot to do this in our original implementation on general ecosystem
grounds. It's also mandated starting draft-26.
Just to avoid unnecessary turbulence, since draft-23 is doomed to die
anyway, condition this on our draft-28 implementation. (We don't support
24 through 27.)
We'd actually checked this already on the Go side, but the spec wants a
different alert.
Change-Id: I0014cda03d7129df0b48de077e45f8ae9fd16976
Reviewed-on: https://boringssl-review.googlesource.com/28124
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 7b63bc8..6cedfe2 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -133,6 +133,7 @@
SSL,204,RESUMED_EMS_SESSION_WITHOUT_EMS_EXTENSION
SSL,205,RESUMED_NON_EMS_SESSION_WITH_EMS_EXTENSION
SSL,206,SCSV_RECEIVED_WHEN_RENEGOTIATING
+SSL,288,SECOND_SERVERHELLO_VERSION_MISMATCH
SSL,207,SERVERHELLO_TLSEXT
SSL,273,SERVER_CERT_CHANGED
SSL,286,SERVER_ECHOED_INVALID_SESSION_ID
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 5fe394d..1ad7042 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4746,6 +4746,7 @@
#define SSL_R_NEGOTIATED_TB_WITHOUT_EMS_OR_RI 285
#define SSL_R_SERVER_ECHOED_INVALID_SESSION_ID 286
#define SSL_R_PRIVATE_KEY_OPERATION_FAILED 287
+#define SSL_R_SECOND_SERVERHELLO_VERSION_MISMATCH 288
#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/test/runner/common.go b/ssl/test/runner/common.go
index 7115b4d..f5ddaed 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1395,9 +1395,15 @@
// specified value in ServerHello version field.
SendServerHelloVersion uint16
- // SendServerSupportedExtensionVersion, if non-zero, causes the server to send
- // the specified value in supported_versions extension in the ServerHello.
- SendServerSupportedExtensionVersion uint16
+ // SendServerSupportedVersionExtension, if non-zero, causes the server to send
+ // the specified value in supported_versions extension in the ServerHello (but
+ // not the HelloRetryRequest).
+ SendServerSupportedVersionExtension uint16
+
+ // OmitServerSupportedVersionExtension, if true, causes the server to
+ // omit the supported_versions extension in the ServerHello (but not the
+ // HelloRetryRequest)
+ OmitServerSupportedVersionExtension bool
// SkipHelloRetryRequest, if true, causes the TLS 1.3 server to not send
// HelloRetryRequest.
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 4178dc1..6f6f440 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -590,7 +590,7 @@
}
if serverWireVersion != serverHello.vers {
- c.sendAlert(alertProtocolVersion)
+ c.sendAlert(alertIllegalParameter)
return fmt.Errorf("tls: server sent non-matching version %x vs %x", serverWireVersion, serverHello.vers)
}
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index 64936b4..745c8f3 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -919,6 +919,7 @@
vers uint16
versOverride uint16
supportedVersOverride uint16
+ omitSupportedVers bool
random []byte
sessionId []byte
cipherSuite uint16
@@ -979,12 +980,14 @@
extensions.addU16(2) // Length
extensions.addU16(m.pskIdentity)
}
- extensions.addU16(extensionSupportedVersions)
- extensions.addU16(2) // Length
- if m.supportedVersOverride != 0 {
- extensions.addU16(m.supportedVersOverride)
- } else {
- extensions.addU16(m.vers)
+ if !m.omitSupportedVers {
+ extensions.addU16(extensionSupportedVersions)
+ extensions.addU16(2) // Length
+ if m.supportedVersOverride != 0 {
+ extensions.addU16(m.supportedVersOverride)
+ } else {
+ extensions.addU16(m.vers)
+ }
}
if len(m.customExtension) > 0 {
extensions.addU16(extensionCustom)
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index e5f2944..da6fddc 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -375,7 +375,8 @@
sessionId: hs.clientHello.sessionId,
compressionMethod: config.Bugs.SendCompressionMethod,
versOverride: config.Bugs.SendServerHelloVersion,
- supportedVersOverride: config.Bugs.SendServerSupportedExtensionVersion,
+ supportedVersOverride: config.Bugs.SendServerSupportedVersionExtension,
+ omitSupportedVers: config.Bugs.OmitServerSupportedVersionExtension,
customExtension: config.Bugs.CustomUnencryptedExtension,
unencryptedALPN: config.Bugs.SendUnencryptedALPN,
}
@@ -1122,7 +1123,7 @@
versOverride: config.Bugs.SendServerHelloVersion,
compressionMethod: config.Bugs.SendCompressionMethod,
extensions: serverExtensions{
- supportedVersion: config.Bugs.SendServerSupportedExtensionVersion,
+ supportedVersion: config.Bugs.SendServerSupportedVersionExtension,
},
omitExtensions: config.Bugs.OmitExtensions,
emptyExtensions: config.Bugs.EmptyExtensions,
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 16c039f..0ad00c2 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -5592,7 +5592,7 @@
config: Config{
MaxVersion: VersionTLS12,
Bugs: ProtocolBugs{
- SendServerSupportedExtensionVersion: VersionTLS12,
+ SendServerSupportedVersionExtension: VersionTLS12,
},
},
shouldFail: true,
@@ -12757,6 +12757,39 @@
expectedError: ":WRONG_CURVE:",
})
+ // Test that the supported_versions extension is enforced in the
+ // second ServerHello. Note we only enforce this starting draft 28.
+ if isDraft28(version.versionWire) {
+ testCases = append(testCases, testCase{
+ name: "SecondServerHelloNoVersion-" + name,
+ config: Config{
+ MaxVersion: VersionTLS13,
+ // P-384 requires HelloRetryRequest in BoringSSL.
+ CurvePreferences: []CurveID{CurveP384},
+ Bugs: ProtocolBugs{
+ OmitServerSupportedVersionExtension: true,
+ },
+ },
+ tls13Variant: variant,
+ shouldFail: true,
+ expectedError: ":SECOND_SERVERHELLO_VERSION_MISMATCH:",
+ })
+ testCases = append(testCases, testCase{
+ name: "SecondServerHelloWrongVersion-" + name,
+ config: Config{
+ MaxVersion: VersionTLS13,
+ // P-384 requires HelloRetryRequest in BoringSSL.
+ CurvePreferences: []CurveID{CurveP384},
+ Bugs: ProtocolBugs{
+ SendServerSupportedVersionExtension: 0x1234,
+ },
+ },
+ tls13Variant: variant,
+ shouldFail: true,
+ expectedError: ":SECOND_SERVERHELLO_VERSION_MISMATCH:",
+ })
+ }
+
testCases = append(testCases, testCase{
name: "RequestContextInHandshake-" + name,
config: Config{
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index 579e6a6..49f528b 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -290,6 +290,18 @@
return ssl_hs_error;
}
+ if (ssl_is_draft28(ssl->version)) {
+ // Recheck supported_versions, in case this is the second ServerHello.
+ uint16_t version;
+ if (!have_supported_versions ||
+ !CBS_get_u16(&supported_versions, &version) ||
+ version != ssl->version) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_SECOND_SERVERHELLO_VERSION_MISMATCH);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return ssl_hs_error;
+ }
+ }
+
alert = SSL_AD_DECODE_ERROR;
if (have_pre_shared_key) {
if (ssl->session == NULL) {
