kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Enforce supported_versions in the second ServerHello.

Browse Source 
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>
This commit is contained in:
David Benjamin 2018-05-04 22:00:33 -04:00 committed by CQ bot account: commit-bot@chromium.org
parent 3d9705d0a4
commit ed188fd8ef
8 changed files with 70 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
crypto/err/ssl.errordata
View File

@ -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

1
include/openssl/ssl.h
View File

@ -4746,6 +4746,7 @@ OPENSSL_EXPORT bool SSL_apply_handback(SSL *ssl, Span<const uint8_t> 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

12
ssl/test/runner/common.go
View File

@ -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.

2
ssl/test/runner/handshake_client.go
View File

@ -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)
}

15
ssl/test/runner/handshake_messages.go
View File

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

5
ssl/test/runner/handshake_server.go
View File

@ -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,

35
ssl/test/runner/runner.go
View File

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

12
ssl/tls13_client.cc
View File

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