From ed188fd8ef7ab62ebdddaa5f22e2f3d4d901e94b Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 4 May 2018 22:00:33 -0400 Subject: [PATCH] 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 CQ-Verified: CQ bot account: commit-bot@chromium.org Reviewed-by: Steven Valdez --- crypto/err/ssl.errordata | 1 + include/openssl/ssl.h | 1 + ssl/test/runner/common.go | 12 ++++++--- ssl/test/runner/handshake_client.go | 2 +- ssl/test/runner/handshake_messages.go | 15 +++++++----- ssl/test/runner/handshake_server.go | 5 ++-- ssl/test/runner/runner.go | 35 ++++++++++++++++++++++++++- ssl/tls13_client.cc | 12 +++++++++ 8 files changed, 70 insertions(+), 13 deletions(-) diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index 7b63bc8e..6cedfe2b 100644 --- a/crypto/err/ssl.errordata +++ b/crypto/err/ssl.errordata @@ -133,6 +133,7 @@ SSL,203,REQUIRED_CIPHER_MISSING 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 5fe394de..1ad70420 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -4746,6 +4746,7 @@ OPENSSL_EXPORT bool SSL_apply_handback(SSL *ssl, Span handback); #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 7115b4d7..f5ddaed5 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -1395,9 +1395,15 @@ type ProtocolBugs struct { // 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 4178dc17..6f6f440a 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go @@ -590,7 +590,7 @@ NextCipherSuite: } 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 64936b48..745c8f39 100644 --- a/ssl/test/runner/handshake_messages.go +++ b/ssl/test/runner/handshake_messages.go @@ -919,6 +919,7 @@ type serverHelloMsg struct { vers uint16 versOverride uint16 supportedVersOverride uint16 + omitSupportedVers bool random []byte sessionId []byte cipherSuite uint16 @@ -979,12 +980,14 @@ func (m *serverHelloMsg) marshal() []byte { 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 e5f2944f..da6fddc3 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -375,7 +375,8 @@ func (hs *serverHandshakeState) doTLS13Handshake() error { 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 @@ func (hs *serverHandshakeState) processClientHello() (isResume bool, err error) 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 16c039f4..0ad00c2d 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -5592,7 +5592,7 @@ func addVersionNegotiationTests() { config: Config{ MaxVersion: VersionTLS12, Bugs: ProtocolBugs{ - SendServerSupportedExtensionVersion: VersionTLS12, + SendServerSupportedVersionExtension: VersionTLS12, }, }, shouldFail: true, @@ -12757,6 +12757,39 @@ func addTLS13HandshakeTests() { 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 579e6a6a..49f528b3 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc @@ -290,6 +290,18 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { 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) {