diff options
author | David Benjamin <davidben@google.com> | 2016-09-19 19:57:37 -0400 |
---|---|---|
committer | CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> | 2016-09-21 19:51:45 +0000 |
commit | 2dc0204603f777597e2f97662e42887d1af5013f (patch) | |
tree | 51be5e0d7c4e766c6a5d08bccaa7c961338d0c15 /ssl | |
parent | c027999c28db2f448ea5795798080f6a5aaa01d6 (diff) | |
download | boringssl-2dc0204603f777597e2f97662e42887d1af5013f.zip boringssl-2dc0204603f777597e2f97662e42887d1af5013f.tar.gz boringssl-2dc0204603f777597e2f97662e42887d1af5013f.tar.bz2 |
Don't return invalid versions in version_from_wire.
This is in preparation for using the supported_versions extension to
experiment with draft TLS 1.3 versions, since we don't wish to restore
the fallback. With versions begin opaque values, we will want
version_from_wire to reject unknown values, not attempt to preserve
order in some way.
This means ClientHello.version processing needs to be separate code.
That's just written out fully in negotiate_version now. It also means
SSL_set_{min,max}_version will notice invalid inputs which aligns us
better with upstream's versions of those APIs.
This CL doesn't replace ssl->version with an internal-representation
version, though follow work should do it once a couple of changes land
in consumers.
BUG=90
Change-Id: Id2f5e1fa72847c823ee7f082e9e69f55e51ce9da
Reviewed-on: https://boringssl-review.googlesource.com/11122
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Diffstat (limited to 'ssl')
-rw-r--r-- | ssl/dtls_method.c | 42 | ||||
-rw-r--r-- | ssl/handshake_client.c | 20 | ||||
-rw-r--r-- | ssl/handshake_server.c | 29 | ||||
-rw-r--r-- | ssl/internal.h | 12 | ||||
-rw-r--r-- | ssl/ssl_lib.c | 26 | ||||
-rw-r--r-- | ssl/ssl_test.cc | 128 | ||||
-rw-r--r-- | ssl/t1_lib.c | 19 | ||||
-rw-r--r-- | ssl/test/bssl_shim.cc | 17 | ||||
-rw-r--r-- | ssl/test/runner/runner.go | 11 | ||||
-rw-r--r-- | ssl/tls_method.c | 31 |
10 files changed, 231 insertions, 104 deletions
diff --git a/ssl/dtls_method.c b/ssl/dtls_method.c index e2e1726..9d791b5 100644 --- a/ssl/dtls_method.c +++ b/ssl/dtls_method.c @@ -65,31 +65,33 @@ #include "internal.h" -static uint16_t dtls1_version_from_wire(uint16_t wire_version) { - uint16_t tls_version = ~wire_version; - uint16_t version = tls_version + 0x0201; - /* If either component overflowed, clamp it so comparisons still work. */ - if ((version >> 8) < (tls_version >> 8)) { - version = 0xff00 | (version & 0xff); +static int dtls1_version_from_wire(uint16_t *out_version, + uint16_t wire_version) { + switch (wire_version) { + case DTLS1_VERSION: + /* DTLS 1.0 maps to TLS 1.1, not TLS 1.0. */ + *out_version = TLS1_1_VERSION; + return 1; + case DTLS1_2_VERSION: + *out_version = TLS1_2_VERSION; + return 1; } - if ((version & 0xff) < (tls_version & 0xff)) { - version = (version & 0xff00) | 0xff; - } - /* DTLS 1.0 maps to TLS 1.1, not TLS 1.0. */ - if (version == TLS1_VERSION) { - version = TLS1_1_VERSION; - } - return version; + + return 0; } static uint16_t dtls1_version_to_wire(uint16_t version) { - assert(version >= TLS1_1_VERSION); - - /* DTLS 1.0 maps to TLS 1.1, not TLS 1.0. */ - if (version == TLS1_1_VERSION) { - return DTLS1_VERSION; + switch (version) { + case TLS1_1_VERSION: + /* DTLS 1.0 maps to TLS 1.1, not TLS 1.0. */ + return DTLS1_VERSION; + case TLS1_2_VERSION: + return DTLS1_2_VERSION; } - return ~(version - 0x0201); + + /* It is an error to use this function with an invalid version. */ + assert(0); + return 0; } static int dtls1_set_read_state(SSL *ssl, SSL_AEAD_CTX *aead_ctx) { diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c index 4e4cf5c..d78d0a4 100644 --- a/ssl/handshake_client.c +++ b/ssl/handshake_client.c @@ -609,9 +609,11 @@ static int ssl_write_client_cipher_list(SSL *ssl, CBB *out, return 0; } /* Add PSK ciphers for TLS 1.3 resumption. */ + uint16_t session_version; if (ssl->session != NULL && - ssl->method->version_from_wire(ssl->session->ssl_version) >= - TLS1_3_VERSION) { + ssl->method->version_from_wire(&session_version, + ssl->session->ssl_version) && + session_version >= TLS1_3_VERSION) { uint16_t resumption_cipher; if (ssl_cipher_get_ecdhe_psk_cipher(cipher, &resumption_cipher) && !CBB_add_u16(&child, resumption_cipher)) { @@ -716,9 +718,10 @@ static int ssl3_send_client_hello(SSL *ssl) { /* If the configured session has expired or was created at a disabled * version, drop it. */ if (ssl->session != NULL) { - uint16_t session_version = - ssl->method->version_from_wire(ssl->session->ssl_version); - if ((session_version < TLS1_3_VERSION && + uint16_t session_version; + if (!ssl->method->version_from_wire(&session_version, + ssl->session->ssl_version) || + (session_version < TLS1_3_VERSION && ssl->session->session_id_length == 0) || ssl->session->not_resumable || !ssl_session_is_time_valid(ssl, ssl->session) || @@ -797,7 +800,7 @@ static int ssl3_get_server_hello(SSL *ssl) { CERT *ct = ssl->cert; int al = SSL_AD_INTERNAL_ERROR; CBS server_hello, server_random, session_id; - uint16_t server_wire_version, server_version, cipher_suite; + uint16_t server_wire_version, cipher_suite; uint8_t compression_method; int ret = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message); @@ -831,10 +834,9 @@ static int ssl3_get_server_hello(SSL *ssl) { goto f_err; } - server_version = ssl->method->version_from_wire(server_wire_version); - - uint16_t min_version, max_version; + uint16_t min_version, max_version, server_version; if (!ssl_get_version_range(ssl, &min_version, &max_version) || + !ssl->method->version_from_wire(&server_version, server_wire_version) || server_version < min_version || server_version > max_version) { OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_PROTOCOL); al = SSL_AD_PROTOCOL_VERSION; diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c index 3790762..d57735a 100644 --- a/ssl/handshake_server.c +++ b/ssl/handshake_server.c @@ -564,12 +564,30 @@ static int negotiate_version( return 0; } - uint16_t client_version = - ssl->method->version_from_wire(client_hello->version); - ssl->client_version = client_hello->version; + /* For TLS versions which use ClientHello.version, convert it to a version we + * are aware of. */ + uint16_t version = 0; + if (SSL_is_dtls(ssl)) { + if (client_hello->version <= DTLS1_2_VERSION) { + version = TLS1_2_VERSION; + } else if (client_hello->version <= DTLS1_VERSION) { + version = TLS1_1_VERSION; + } + } else { + if (client_hello->version >= TLS1_3_VERSION) { + version = TLS1_3_VERSION; + } else if (client_hello->version >= TLS1_2_VERSION) { + version = TLS1_2_VERSION; + } else if (client_hello->version >= TLS1_1_VERSION) { + version = TLS1_1_VERSION; + } else if (client_hello->version >= TLS1_VERSION) { + version = TLS1_VERSION; + } else if (client_hello->version >= SSL3_VERSION) { + version = SSL3_VERSION; + } + } - /* Select the version to use. */ - uint16_t version = client_version; + /* Apply our minimum and maximum version. */ if (version > max_version) { version = max_version; } @@ -589,6 +607,7 @@ static int negotiate_version( return 0; } + ssl->client_version = client_hello->version; ssl->version = ssl->method->version_to_wire(version); ssl->s3->enc_method = ssl3_get_enc_method(version); assert(ssl->s3->enc_method != NULL); diff --git a/ssl/internal.h b/ssl/internal.h index bdb392c..eff5672 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1090,14 +1090,10 @@ struct ssl_protocol_method_st { uint16_t min_version; /* max_version is the maximum implemented version. */ uint16_t max_version; - /* version_from_wire maps |wire_version| to a protocol version. For - * SSLv3/TLS, the version is returned as-is. For DTLS, the corresponding TLS - * version is used. Note that this mapping is not injective but preserves - * comparisons. - * - * TODO(davidben): To normalize some DTLS-specific code, move away from using - * the wire version except at API boundaries. */ - uint16_t (*version_from_wire)(uint16_t wire_version); + /* version_from_wire maps |wire_version| to a protocol version. On success, it + * sets |*out_version| to the result and returns one. If the version is + * unknown, it returns zero. */ + int (*version_from_wire)(uint16_t *out_version, uint16_t wire_version); /* version_to_wire maps |version| to the wire representation. It is an error * to call it with an invalid version. */ uint16_t (*version_to_wire)(uint16_t version); diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 3e27f37..0e8b344 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -949,20 +949,20 @@ int SSL_get_error(const SSL *ssl, int ret_code) { return SSL_ERROR_SYSCALL; } -void SSL_CTX_set_min_version(SSL_CTX *ctx, uint16_t version) { - ctx->min_version = ctx->method->version_from_wire(version); +int SSL_CTX_set_min_version(SSL_CTX *ctx, uint16_t version) { + return ctx->method->version_from_wire(&ctx->min_version, version); } -void SSL_CTX_set_max_version(SSL_CTX *ctx, uint16_t version) { - ctx->max_version = ctx->method->version_from_wire(version); +int SSL_CTX_set_max_version(SSL_CTX *ctx, uint16_t version) { + return ctx->method->version_from_wire(&ctx->max_version, version); } -void SSL_set_min_version(SSL *ssl, uint16_t version) { - ssl->min_version = ssl->method->version_from_wire(version); +int SSL_set_min_version(SSL *ssl, uint16_t version) { + return ssl->method->version_from_wire(&ssl->min_version, version); } -void SSL_set_max_version(SSL *ssl, uint16_t version) { - ssl->max_version = ssl->method->version_from_wire(version); +int SSL_set_max_version(SSL *ssl, uint16_t version) { + return ssl->method->version_from_wire(&ssl->max_version, version); } uint32_t SSL_CTX_set_options(SSL_CTX *ctx, uint32_t options) { @@ -2750,7 +2750,15 @@ int ssl_get_version_range(const SSL *ssl, uint16_t *out_min_version, uint16_t ssl3_protocol_version(const SSL *ssl) { assert(ssl->s3->have_version); - return ssl->method->version_from_wire(ssl->version); + uint16_t version; + if (!ssl->method->version_from_wire(&version, ssl->version)) { + /* TODO(davidben): Use the internal version representation for ssl->version + * and map to the public API representation at API boundaries. */ + assert(0); + return 0; + } + + return version; } int SSL_is_server(const SSL *ssl) { return ssl->server; } diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 8d45ace..4c4c0f4 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -1523,11 +1523,11 @@ static bool TestGetPeerCertificate() { bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method())); if (!ctx || !SSL_CTX_use_certificate(ctx.get(), cert.get()) || - !SSL_CTX_use_PrivateKey(ctx.get(), key.get())) { + !SSL_CTX_use_PrivateKey(ctx.get(), key.get()) || + !SSL_CTX_set_min_version(ctx.get(), version) || + !SSL_CTX_set_max_version(ctx.get(), version)) { return false; } - SSL_CTX_set_min_version(ctx.get(), version); - SSL_CTX_set_max_version(ctx.get(), version); SSL_CTX_set_verify( ctx.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, nullptr); SSL_CTX_set_cert_verify_callback(ctx.get(), VerifySucceed, NULL); @@ -1590,11 +1590,11 @@ static bool TestRetainOnlySHA256OfCerts() { bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method())); if (!ctx || !SSL_CTX_use_certificate(ctx.get(), cert.get()) || - !SSL_CTX_use_PrivateKey(ctx.get(), key.get())) { + !SSL_CTX_use_PrivateKey(ctx.get(), key.get()) || + !SSL_CTX_set_min_version(ctx.get(), version) || + !SSL_CTX_set_max_version(ctx.get(), version)) { return false; } - SSL_CTX_set_min_version(ctx.get(), version); - SSL_CTX_set_max_version(ctx.get(), version); SSL_CTX_set_verify( ctx.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, nullptr); SSL_CTX_set_cert_verify_callback(ctx.get(), VerifySucceed, NULL); @@ -1631,15 +1631,14 @@ static bool TestRetainOnlySHA256OfCerts() { static bool ClientHelloMatches(uint16_t version, const uint8_t *expected, size_t expected_len) { bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method())); - if (!ctx) { - return false; - } - SSL_CTX_set_max_version(ctx.get(), version); - // Our default cipher list varies by CPU capabilities, so manually place the - // ChaCha20 ciphers in front. - if (!SSL_CTX_set_cipher_list(ctx.get(), "CHACHA20:ALL")) { + if (!ctx || + !SSL_CTX_set_max_version(ctx.get(), version) || + // Our default cipher list varies by CPU capabilities, so manually place + // the ChaCha20 ciphers in front. + !SSL_CTX_set_cipher_list(ctx.get(), "CHACHA20:ALL")) { return false; } + bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get())); if (!ssl) { return false; @@ -1872,16 +1871,15 @@ static bool TestSessionIDContext() { !SSL_CTX_use_certificate(server_ctx.get(), cert.get()) || !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get()) || !SSL_CTX_set_session_id_context(server_ctx.get(), kContext1, - sizeof(kContext1))) { + sizeof(kContext1)) || + !SSL_CTX_set_min_version(client_ctx.get(), version) || + !SSL_CTX_set_max_version(client_ctx.get(), version) || + !SSL_CTX_set_min_version(server_ctx.get(), version) || + !SSL_CTX_set_max_version(server_ctx.get(), version)) { return false; } - SSL_CTX_set_min_version(client_ctx.get(), version); - SSL_CTX_set_max_version(client_ctx.get(), version); SSL_CTX_set_session_cache_mode(client_ctx.get(), SSL_SESS_CACHE_BOTH); - - SSL_CTX_set_min_version(server_ctx.get(), version); - SSL_CTX_set_max_version(server_ctx.get(), version); SSL_CTX_set_session_cache_mode(server_ctx.get(), SSL_SESS_CACHE_BOTH); bssl::UniquePtr<SSL_SESSION> session = @@ -1933,16 +1931,16 @@ static bool TestSessionTimeout() { bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method())); if (!server_ctx || !client_ctx || !SSL_CTX_use_certificate(server_ctx.get(), cert.get()) || - !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get())) { + !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get()) || + !SSL_CTX_set_min_version(client_ctx.get(), version) || + !SSL_CTX_set_max_version(client_ctx.get(), version) || + !SSL_CTX_set_min_version(server_ctx.get(), version) || + !SSL_CTX_set_max_version(server_ctx.get(), version)) { return false; } - SSL_CTX_set_min_version(client_ctx.get(), version); - SSL_CTX_set_max_version(client_ctx.get(), version); SSL_CTX_set_session_cache_mode(client_ctx.get(), SSL_SESS_CACHE_BOTH); - SSL_CTX_set_min_version(server_ctx.get(), version); - SSL_CTX_set_max_version(server_ctx.get(), version); SSL_CTX_set_session_cache_mode(server_ctx.get(), SSL_SESS_CACHE_BOTH); SSL_CTX_set_current_time_cb(server_ctx.get(), CurrentTimeCallback); @@ -2012,17 +2010,16 @@ static bool TestSNICallback() { // this doesn't happen when |version| is TLS 1.2, configure the private // key to only sign SHA-256. !SSL_CTX_set_signing_algorithm_prefs(server_ctx2.get(), - &kECDSAWithSHA256, 1)) { + &kECDSAWithSHA256, 1) || + !SSL_CTX_set_min_version(client_ctx.get(), version) || + !SSL_CTX_set_max_version(client_ctx.get(), version) || + !SSL_CTX_set_min_version(server_ctx.get(), version) || + !SSL_CTX_set_max_version(server_ctx.get(), version) || + !SSL_CTX_set_min_version(server_ctx2.get(), version) || + !SSL_CTX_set_max_version(server_ctx2.get(), version)) { return false; } - SSL_CTX_set_min_version(client_ctx.get(), version); - SSL_CTX_set_max_version(client_ctx.get(), version); - SSL_CTX_set_min_version(server_ctx.get(), version); - SSL_CTX_set_max_version(server_ctx.get(), version); - SSL_CTX_set_min_version(server_ctx2.get(), version); - SSL_CTX_set_max_version(server_ctx2.get(), version); - SSL_CTX_set_tlsext_servername_callback(server_ctx.get(), SwitchContext); SSL_CTX_set_tlsext_servername_arg(server_ctx.get(), server_ctx2.get()); @@ -2047,7 +2044,10 @@ static bool TestSNICallback() { } static int SetMaxVersion(const struct ssl_early_callback_ctx *ctx) { - SSL_set_max_version(ctx->ssl, TLS1_2_VERSION); + if (!SSL_set_max_version(ctx->ssl, TLS1_2_VERSION)) { + return -1; + } + return 1; } @@ -2060,13 +2060,12 @@ static bool TestEarlyCallbackVersionSwitch() { bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method())); if (!cert || !key || !server_ctx || !client_ctx || !SSL_CTX_use_certificate(server_ctx.get(), cert.get()) || - !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get())) { + !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get()) || + !SSL_CTX_set_max_version(client_ctx.get(), TLS1_3_VERSION) || + !SSL_CTX_set_max_version(server_ctx.get(), TLS1_3_VERSION)) { return false; } - SSL_CTX_set_max_version(client_ctx.get(), TLS1_3_VERSION); - SSL_CTX_set_max_version(server_ctx.get(), TLS1_3_VERSION); - SSL_CTX_set_select_certificate_cb(server_ctx.get(), SetMaxVersion); bssl::UniquePtr<SSL> client, server; @@ -2083,6 +2082,58 @@ static bool TestEarlyCallbackVersionSwitch() { return true; } +static bool TestSetVersion() { + bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method())); + if (!ctx) { + return false; + } + + if (!SSL_CTX_set_max_version(ctx.get(), TLS1_VERSION) || + !SSL_CTX_set_max_version(ctx.get(), TLS1_1_VERSION) || + !SSL_CTX_set_min_version(ctx.get(), TLS1_VERSION) || + !SSL_CTX_set_min_version(ctx.get(), TLS1_1_VERSION)) { + fprintf(stderr, "Could not set valid TLS version.\n"); + return false; + } + + if (SSL_CTX_set_max_version(ctx.get(), DTLS1_VERSION) || + SSL_CTX_set_max_version(ctx.get(), 0x0200) || + SSL_CTX_set_max_version(ctx.get(), 0x1234) || + SSL_CTX_set_min_version(ctx.get(), DTLS1_VERSION) || + SSL_CTX_set_min_version(ctx.get(), 0x0200) || + SSL_CTX_set_min_version(ctx.get(), 0x1234)) { + fprintf(stderr, "Unexpectedly set invalid TLS version.\n"); + return false; + } + + ctx.reset(SSL_CTX_new(DTLS_method())); + if (!ctx) { + return false; + } + + if (!SSL_CTX_set_max_version(ctx.get(), DTLS1_VERSION) || + !SSL_CTX_set_max_version(ctx.get(), DTLS1_2_VERSION) || + !SSL_CTX_set_min_version(ctx.get(), DTLS1_VERSION) || + !SSL_CTX_set_min_version(ctx.get(), DTLS1_2_VERSION)) { + fprintf(stderr, "Could not set valid DTLS version.\n"); + return false; + } + + if (SSL_CTX_set_max_version(ctx.get(), TLS1_VERSION) || + SSL_CTX_set_max_version(ctx.get(), 0xfefe /* DTLS 1.1 */) || + SSL_CTX_set_max_version(ctx.get(), 0xfffe /* DTLS 0.1 */) || + SSL_CTX_set_max_version(ctx.get(), 0x1234) || + SSL_CTX_set_min_version(ctx.get(), TLS1_VERSION) || + SSL_CTX_set_min_version(ctx.get(), 0xfefe /* DTLS 1.1 */) || + SSL_CTX_set_min_version(ctx.get(), 0xfffe /* DTLS 0.1 */) || + SSL_CTX_set_min_version(ctx.get(), 0x1234)) { + fprintf(stderr, "Unexpectedly set invalid DTLS version.\n"); + return false; + } + + return true; +} + int main() { CRYPTO_library_init(); @@ -2118,7 +2169,8 @@ int main() { !TestSessionIDContext() || !TestSessionTimeout() || !TestSNICallback() || - !TestEarlyCallbackVersionSwitch()) { + !TestEarlyCallbackVersionSwitch() || + !TestSetVersion()) { ERR_print_errors_fp(stderr); return 1; } diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 5871be2..a89f5cf 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1038,12 +1038,14 @@ static int ext_ticket_add_clienthello(SSL *ssl, CBB *out) { * advertise the extension to avoid potentially breaking servers which carry * over the state from the previous handshake, such as OpenSSL servers * without upstream's 3c3f0259238594d77264a78944d409f2127642c4. */ + uint16_t session_version; if (!ssl->s3->initial_handshake_complete && ssl->session != NULL && ssl->session->tlsext_tick != NULL && /* Don't send TLS 1.3 session tickets in the ticket extension. */ - ssl->method->version_from_wire(ssl->session->ssl_version) < - TLS1_3_VERSION) { + ssl->method->version_from_wire(&session_version, + ssl->session->ssl_version) && + session_version < TLS1_3_VERSION) { ticket_data = ssl->session->tlsext_tick; ticket_len = ssl->session->tlsext_ticklen; } @@ -1107,7 +1109,12 @@ static int ext_ticket_add_serverhello(SSL *ssl, CBB *out) { * https://tools.ietf.org/html/rfc5246#section-7.4.1.4.1 */ static int ext_sigalgs_add_clienthello(SSL *ssl, CBB *out) { - if (ssl->method->version_from_wire(ssl->client_version) < TLS1_2_VERSION) { + uint16_t min_version, max_version; + if (!ssl_get_version_range(ssl, &min_version, &max_version)) { + return 0; + } + + if (max_version < TLS1_2_VERSION) { return 1; } @@ -1990,9 +1997,11 @@ static int ext_pre_shared_key_add_clienthello(SSL *ssl, CBB *out) { return 0; } + uint16_t session_version; if (max_version < TLS1_3_VERSION || ssl->session == NULL || - ssl->method->version_from_wire(ssl->session->ssl_version) < - TLS1_3_VERSION) { + !ssl->method->version_from_wire(&session_version, + ssl->session->ssl_version) || + session_version < TLS1_3_VERSION) { return 1; } diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 7196e49..55b6599 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -814,9 +814,10 @@ static bssl::UniquePtr<SSL_CTX> SetupCtx(const TestConfig *config) { return nullptr; } - if (!config->is_dtls) { - // Enable TLS 1.3 for tests. - SSL_CTX_set_max_version(ssl_ctx.get(), TLS1_3_VERSION); + // Enable TLS 1.3 for tests. + if (!config->is_dtls && + !SSL_CTX_set_max_version(ssl_ctx.get(), TLS1_3_VERSION)) { + return nullptr; } std::string cipher_list = "ALL"; @@ -1364,11 +1365,13 @@ static bool DoExchange(bssl::UniquePtr<SSL_SESSION> *out_session, !SSL_enable_signed_cert_timestamps(ssl.get())) { return false; } - if (config->min_version != 0) { - SSL_set_min_version(ssl.get(), (uint16_t)config->min_version); + if (config->min_version != 0 && + !SSL_set_min_version(ssl.get(), (uint16_t)config->min_version)) { + return false; } - if (config->max_version != 0) { - SSL_set_max_version(ssl.get(), (uint16_t)config->max_version); + if (config->max_version != 0 && + !SSL_set_max_version(ssl.get(), (uint16_t)config->max_version)) { + return false; } if (config->mtu != 0) { SSL_set_options(ssl.get(), SSL_OP_NO_QUERY_MTU); diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 039cd5c..99d7fac 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -4221,6 +4221,17 @@ func addVersionNegotiationTests() { expectedError: ":UNSUPPORTED_PROTOCOL:", }) + testCases = append(testCases, testCase{ + name: "ServerBogusVersion", + config: Config{ + Bugs: ProtocolBugs{ + SendServerHelloVersion: 0x1234, + }, + }, + shouldFail: true, + expectedError: ":UNSUPPORTED_PROTOCOL:", + }) + // Test TLS 1.3's downgrade signal. testCases = append(testCases, testCase{ name: "Downgrade-TLS12-Client", diff --git a/ssl/tls_method.c b/ssl/tls_method.c index 61dc9f6..155d09a 100644 --- a/ssl/tls_method.c +++ b/ssl/tls_method.c @@ -56,6 +56,7 @@ #include <openssl/ssl.h> +#include <assert.h> #include <string.h> #include <openssl/buf.h> @@ -63,11 +64,35 @@ #include "internal.h" -static uint16_t ssl3_version_from_wire(uint16_t wire_version) { - return wire_version; +static int ssl3_version_from_wire(uint16_t *out_version, + uint16_t wire_version) { + switch (wire_version) { + case SSL3_VERSION: + case TLS1_VERSION: + case TLS1_1_VERSION: + case TLS1_2_VERSION: + case TLS1_3_VERSION: + *out_version = wire_version; + return 1; + } + + return 0; } -static uint16_t ssl3_version_to_wire(uint16_t version) { return version; } +static uint16_t ssl3_version_to_wire(uint16_t version) { + switch (version) { + case SSL3_VERSION: + case TLS1_VERSION: + case TLS1_1_VERSION: + case TLS1_2_VERSION: + case TLS1_3_VERSION: + return version; + } + + /* It is an error to use this function with an invalid version. */ + assert(0); + return 0; +} static int ssl3_set_read_state(SSL *ssl, SSL_AEAD_CTX *aead_ctx) { if (ssl->s3->rrec.length != 0) { |