From 038da9b939ded45b7624ba31c38a2ea55d53d12a Mon Sep 17 00:00:00 2001 From: Steven Valdez Date: Mon, 10 Jul 2017 12:57:25 -0400 Subject: [PATCH] Move the version to an extension in the experimental TLS 1.3 encoding. Change-Id: I0726e11006235db9309a8370a11e00ede0216279 Reviewed-on: https://boringssl-review.googlesource.com/17704 Reviewed-by: David Benjamin --- ssl/handshake_client.c | 41 ++++++++++++++ ssl/test/runner/common.go | 4 ++ ssl/test/runner/handshake_messages.go | 82 ++++++++++++++++++++++----- ssl/test/runner/handshake_server.go | 16 ++++-- ssl/test/runner/runner.go | 31 +++++++++- ssl/tls13_client.c | 26 ++++++--- ssl/tls13_server.c | 22 ++++++- 7 files changed, 192 insertions(+), 30 deletions(-) diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c index 51f7cc8d..0738e42e 100644 --- a/ssl/handshake_client.c +++ b/ssl/handshake_client.c @@ -846,6 +846,47 @@ static int ssl3_get_server_hello(SSL_HANDSHAKE *hs) { return -1; } + /* Parse out server version from supported_versions if available. */ + if (ssl->s3->tmp.message_type == SSL3_MT_SERVER_HELLO && + server_version == TLS1_2_VERSION) { + CBS copy = server_hello; + CBS extensions; + uint8_t sid_length; + if (!CBS_skip(©, SSL3_RANDOM_SIZE) || + !CBS_get_u8(©, &sid_length) || + !CBS_skip(©, sid_length + 2 /* cipher_suite */ + + 1 /* compression_method */) || + !CBS_get_u16_length_prefixed(©, &extensions) || + CBS_len(©) != 0) { + OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + return -1; + } + + int have_supported_versions; + CBS supported_versions; + const SSL_EXTENSION_TYPE ext_types[] = { + {TLSEXT_TYPE_supported_versions, &have_supported_versions, + &supported_versions}, + }; + + uint8_t alert = SSL_AD_DECODE_ERROR; + if (!ssl_parse_extensions(&extensions, &alert, ext_types, + OPENSSL_ARRAY_SIZE(ext_types), + 1 /* ignore unknown */)) { + ssl3_send_alert(ssl, SSL3_AL_FATAL, alert); + return -1; + } + + if (have_supported_versions) { + if (!CBS_get_u16(&supported_versions, &server_version) || + CBS_len(&supported_versions) != 0) { + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + return -1; + } + } + } + if (!ssl_supports_version(hs, server_version)) { OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_PROTOCOL); ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_PROTOCOL_VERSION); diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 2be474ca..b40f222f 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -1262,6 +1262,10 @@ 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 + // SkipHelloRetryRequest, if true, causes the TLS 1.3 server to not send // HelloRetryRequest. SkipHelloRetryRequest bool diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go index 92bbdca5..1e36f081 100644 --- a/ssl/test/runner/handshake_messages.go +++ b/ssl/test/runner/handshake_messages.go @@ -813,21 +813,22 @@ func (m *clientHelloMsg) unmarshal(data []byte) bool { } type serverHelloMsg struct { - raw []byte - isDTLS bool - vers uint16 - versOverride uint16 - random []byte - sessionId []byte - cipherSuite uint16 - hasKeyShare bool - keyShare keyShareEntry - hasPSKIdentity bool - pskIdentity uint16 - compressionMethod uint8 - customExtension string - unencryptedALPN string - extensions serverExtensions + raw []byte + isDTLS bool + vers uint16 + versOverride uint16 + supportedVersOverride uint16 + random []byte + sessionId []byte + cipherSuite uint16 + hasKeyShare bool + keyShare keyShareEntry + hasPSKIdentity bool + pskIdentity uint16 + compressionMethod uint8 + customExtension string + unencryptedALPN string + extensions serverExtensions } func (m *serverHelloMsg) marshal() []byte { @@ -848,6 +849,8 @@ func (m *serverHelloMsg) marshal() []byte { } if m.versOverride != 0 { hello.addU16(m.versOverride) + } else if m.vers == tls13ExperimentVersion { + hello.addU16(VersionTLS12) } else { hello.addU16(m.vers) } @@ -877,6 +880,15 @@ func (m *serverHelloMsg) marshal() []byte { extensions.addU16(2) // Length extensions.addU16(m.pskIdentity) } + if m.vers == tls13ExperimentVersion || m.supportedVersOverride != 0 { + 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) customExt := extensions.addU16LengthPrefixed() @@ -949,6 +961,36 @@ func (m *serverHelloMsg) unmarshal(data []byte) bool { return false } + // Parse out the version from supported_versions if available. + if m.vers == VersionTLS12 { + vdata := data + for len(vdata) != 0 { + if len(vdata) < 4 { + return false + } + extension := uint16(vdata[0])<<8 | uint16(vdata[1]) + length := int(vdata[2])<<8 | int(vdata[3]) + vdata = vdata[4:] + + if len(vdata) < length { + return false + } + d := vdata[:length] + vdata = vdata[length:] + + if extension == extensionSupportedVersions { + if len(d) < 2 { + return false + } + m.vers = uint16(d[0])<<8 | uint16(d[1]) + vers, ok = wireToVersion(m.vers, m.isDTLS) + if !ok { + return false + } + } + } + } + if vers >= VersionTLS13 { for len(data) != 0 { if len(data) < 4 { @@ -983,6 +1025,10 @@ func (m *serverHelloMsg) unmarshal(data []byte) bool { } m.pskIdentity = uint16(d[0])<<8 | uint16(d[1]) m.hasPSKIdentity = true + case extensionSupportedVersions: + if m.vers != tls13ExperimentVersion { + return false + } default: // Only allow the 3 extensions that are sent in // the clear in TLS 1.3. @@ -1059,6 +1105,7 @@ type serverExtensions struct { hasKeyShare bool hasEarlyData bool keyShare keyShareEntry + supportedVersion uint16 supportedPoints []uint8 serverNameAck bool } @@ -1155,6 +1202,11 @@ func (m *serverExtensions) marshal(extensions *byteBuilder) { keyExchange := keyShare.addU16LengthPrefixed() keyExchange.addBytes(m.keyShare.keyExchange) } + if m.supportedVersion != 0 { + extensions.addU16(extensionSupportedVersions) + extensions.addU16(2) // Length + extensions.addU16(m.supportedVersion) + } if len(m.supportedPoints) > 0 { // http://tools.ietf.org/html/rfc4492#section-5.1.2 extensions.addU16(extensionSupportedPoints) diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index f6692e18..25230a4e 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -359,12 +359,13 @@ func (hs *serverHandshakeState) doTLS13Handshake() error { config := c.config hs.hello = &serverHelloMsg{ - isDTLS: c.isDTLS, - vers: c.wireVersion, - sessionId: hs.clientHello.sessionId, - versOverride: config.Bugs.SendServerHelloVersion, - customExtension: config.Bugs.CustomUnencryptedExtension, - unencryptedALPN: config.Bugs.SendUnencryptedALPN, + isDTLS: c.isDTLS, + vers: c.wireVersion, + sessionId: hs.clientHello.sessionId, + versOverride: config.Bugs.SendServerHelloVersion, + supportedVersOverride: config.Bugs.SendServerSupportedExtensionVersion, + customExtension: config.Bugs.CustomUnencryptedExtension, + unencryptedALPN: config.Bugs.SendUnencryptedALPN, } hs.hello.random = make([]byte, 32) @@ -1056,6 +1057,9 @@ func (hs *serverHandshakeState) processClientHello() (isResume bool, err error) vers: c.wireVersion, versOverride: config.Bugs.SendServerHelloVersion, compressionMethod: config.Bugs.SendCompressionMethod, + extensions: serverExtensions{ + supportedVersion: config.Bugs.SendServerSupportedExtensionVersion, + }, } hs.hello.random = make([]byte, 32) diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index acf1fa8d..be74ffe7 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -5032,7 +5032,6 @@ func addVersionNegotiationTests() { flags: []string{"-tls13-variant", strconv.Itoa(vers.tls13Variant)}, }) } - } // If all versions are unknown, negotiation fails. @@ -5112,6 +5111,36 @@ func addVersionNegotiationTests() { expectedVersion: VersionTLS12, }) + // Test that TLS 1.2 isn't negotiated by the supported_versions extension in + // the ServerHello. + testCases = append(testCases, testCase{ + testType: clientTest, + name: "SupportedVersionSelection-TLS12", + config: Config{ + MaxVersion: VersionTLS12, + Bugs: ProtocolBugs{ + SendServerSupportedExtensionVersion: VersionTLS12, + }, + }, + shouldFail: true, + expectedError: ":UNEXPECTED_EXTENSION:", + }) + + // Test that the non-experimental TLS 1.3 isn't negotiated by the + // supported_versions extension in the ServerHello. + testCases = append(testCases, testCase{ + testType: clientTest, + name: "SupportedVersionSelection-TLS13", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendServerSupportedExtensionVersion: tls13DraftVersion, + }, + }, + shouldFail: true, + expectedError: ":UNEXPECTED_EXTENSION:", + }) + // Test that the maximum version is selected regardless of the // client-sent order. testCases = append(testCases, testCase{ diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c index 0010ccb0..c92b5398 100644 --- a/ssl/tls13_client.c +++ b/ssl/tls13_client.c @@ -56,9 +56,9 @@ static enum ssl_hs_wait_t do_process_hello_retry_request(SSL_HANDSHAKE *hs) { } CBS cbs, extensions; - uint16_t server_wire_version; + uint16_t server_version; CBS_init(&cbs, ssl->init_msg, ssl->init_num); - if (!CBS_get_u16(&cbs, &server_wire_version) || + if (!CBS_get_u16(&cbs, &server_version) || !CBS_get_u16_length_prefixed(&cbs, &extensions) || /* HelloRetryRequest may not be empty. */ CBS_len(&extensions) == 0 || @@ -167,11 +167,11 @@ static enum ssl_hs_wait_t do_process_server_hello(SSL_HANDSHAKE *hs) { } CBS cbs, server_random, session_id, extensions; - uint16_t server_wire_version; + uint16_t server_version; uint16_t cipher_suite; uint8_t compression_method; CBS_init(&cbs, ssl->init_msg, ssl->init_num); - if (!CBS_get_u16(&cbs, &server_wire_version) || + if (!CBS_get_u16(&cbs, &server_version) || !CBS_get_bytes(&cbs, &server_random, SSL3_RANDOM_SIZE) || (ssl->version == TLS1_3_EXPERIMENT_VERSION && !CBS_get_u8_length_prefixed(&cbs, &session_id)) || @@ -185,7 +185,9 @@ static enum ssl_hs_wait_t do_process_server_hello(SSL_HANDSHAKE *hs) { return ssl_hs_error; } - if (server_wire_version != ssl->version) { + uint16_t expected_version = + ssl->version == TLS1_3_EXPERIMENT_VERSION ? TLS1_2_VERSION : ssl->version; + if (server_version != expected_version) { ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_VERSION_NUMBER); return ssl_hs_error; @@ -211,11 +213,13 @@ static enum ssl_hs_wait_t do_process_server_hello(SSL_HANDSHAKE *hs) { } /* Parse out the extensions. */ - int have_key_share = 0, have_pre_shared_key = 0; - CBS key_share, pre_shared_key; + int have_key_share = 0, have_pre_shared_key = 0, have_supported_versions = 0; + CBS key_share, pre_shared_key, supported_versions; const SSL_EXTENSION_TYPE ext_types[] = { {TLSEXT_TYPE_key_share, &have_key_share, &key_share}, {TLSEXT_TYPE_pre_shared_key, &have_pre_shared_key, &pre_shared_key}, + {TLSEXT_TYPE_supported_versions, &have_supported_versions, + &supported_versions}, }; uint8_t alert = SSL_AD_DECODE_ERROR; @@ -226,6 +230,14 @@ static enum ssl_hs_wait_t do_process_server_hello(SSL_HANDSHAKE *hs) { return ssl_hs_error; } + /* supported_versions is parsed in handshake_client to select the experimental + * TLS 1.3 version. */ + if (have_supported_versions && ssl->version != TLS1_3_EXPERIMENT_VERSION) { + OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION); + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNSUPPORTED_EXTENSION); + return ssl_hs_error; + } + alert = SSL_AD_DECODE_ERROR; if (have_pre_shared_key) { if (ssl->session == NULL) { diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c index 58e062dc..25a7c2cd 100644 --- a/ssl/tls13_server.c +++ b/ssl/tls13_server.c @@ -85,6 +85,19 @@ static int resolve_ecdhe_secret(SSL_HANDSHAKE *hs, int *out_need_retry, return ok; } +static int ssl_ext_supported_versions_add_serverhello(SSL_HANDSHAKE *hs, + CBB *out) { + CBB contents; + if (!CBB_add_u16(out, TLSEXT_TYPE_supported_versions) || + !CBB_add_u16_length_prefixed(out, &contents) || + !CBB_add_u16(&contents, hs->ssl->version) || + !CBB_flush(out)) { + return 0; + } + + return 1; +} + static const SSL_CIPHER *choose_tls13_cipher( const SSL *ssl, const SSL_CLIENT_HELLO *client_hello) { if (client_hello->cipher_suites_len % 2 != 0) { @@ -514,10 +527,15 @@ static enum ssl_hs_wait_t do_process_second_client_hello(SSL_HANDSHAKE *hs) { static enum ssl_hs_wait_t do_send_server_hello(SSL_HANDSHAKE *hs) { SSL *const ssl = hs->ssl; + uint16_t version = ssl->version; + if (ssl->version == TLS1_3_EXPERIMENT_VERSION) { + version = TLS1_2_VERSION; + } + /* Send a ServerHello. */ CBB cbb, body, extensions, session_id; if (!ssl->method->init_message(ssl, &cbb, &body, SSL3_MT_SERVER_HELLO) || - !CBB_add_u16(&body, ssl->version) || + !CBB_add_u16(&body, version) || !RAND_bytes(ssl->s3->server_random, sizeof(ssl->s3->server_random)) || !CBB_add_bytes(&body, ssl->s3->server_random, SSL3_RANDOM_SIZE) || (ssl->version == TLS1_3_EXPERIMENT_VERSION && @@ -528,6 +546,8 @@ static enum ssl_hs_wait_t do_send_server_hello(SSL_HANDSHAKE *hs) { !CBB_add_u16_length_prefixed(&body, &extensions) || !ssl_ext_pre_shared_key_add_serverhello(hs, &extensions) || !ssl_ext_key_share_add_serverhello(hs, &extensions) || + (ssl->version == TLS1_3_EXPERIMENT_VERSION && + !ssl_ext_supported_versions_add_serverhello(hs, &extensions)) || !ssl_add_message_cbb(ssl, &cbb)) { goto err; }