From d8434cf85691f32a17dcdfed6e81769a001074dd Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 19 Jul 2018 16:51:58 +0100 Subject: [PATCH] Validate legacy_version The spec says that a client MUST set legacy_version to TLSv1.2, and requires servers to verify that it isn't SSLv3. Fixes #6600 Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/6747) --- crypto/err/openssl.txt | 1 + include/openssl/sslerr.h | 1 + ssl/ssl_err.c | 1 + ssl/statem/statem_lib.c | 12 ++++++++++++ test/recipes/70-test_sslversions.t | 18 +++++++++++++----- 5 files changed, 28 insertions(+), 5 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 3e2bc6991d..a0dc3c5277 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -2581,6 +2581,7 @@ SSL_R_BAD_HELLO_REQUEST:105:bad hello request SSL_R_BAD_HRR_VERSION:263:bad hrr version SSL_R_BAD_KEY_SHARE:108:bad key share SSL_R_BAD_KEY_UPDATE:122:bad key update +SSL_R_BAD_LEGACY_VERSION:292:bad legacy version SSL_R_BAD_LENGTH:271:bad length SSL_R_BAD_PACKET:240:bad packet SSL_R_BAD_PACKET_LENGTH:115:bad packet length diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index 9eba6d8fd5..a5b2c55942 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -471,6 +471,7 @@ int ERR_load_SSL_strings(void); # define SSL_R_BAD_HRR_VERSION 263 # define SSL_R_BAD_KEY_SHARE 108 # define SSL_R_BAD_KEY_UPDATE 122 +# define SSL_R_BAD_LEGACY_VERSION 292 # define SSL_R_BAD_LENGTH 271 # define SSL_R_BAD_PACKET 240 # define SSL_R_BAD_PACKET_LENGTH 115 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 9ce643ae8e..d3e805636f 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -757,6 +757,7 @@ static const ERR_STRING_DATA SSL_str_reasons[] = { {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_HRR_VERSION), "bad hrr version"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_KEY_SHARE), "bad key share"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_KEY_UPDATE), "bad key update"}, + {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_LEGACY_VERSION), "bad legacy version"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_LENGTH), "bad length"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_PACKET), "bad packet"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_PACKET_LENGTH), "bad packet length"}, diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 6262a068c2..ebb21deb8b 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -1753,6 +1753,18 @@ int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello, DOWNGRADE *dgrd) return SSL_R_LENGTH_MISMATCH; } + /* + * The TLSv1.3 spec says the client MUST set this to TLS1_2_VERSION. + * The spec only requires servers to check that it isn't SSLv3: + * "Any endpoint receiving a Hello message with + * ClientHello.legacy_version or ServerHello.legacy_version set to + * 0x0300 MUST abort the handshake with a "protocol_version" alert." + * We are slightly stricter and require that it isn't SSLv3 or lower. + * We tolerate TLSv1 and TLSv1.1. + */ + if (client_version <= SSL3_VERSION) + return SSL_R_BAD_LEGACY_VERSION; + while (PACKET_get_net_2(&versionslist, &candidate_vers)) { /* TODO(TLS1.3): Remove this before release */ if (candidate_vers == TLS1_3_VERSION_DRAFT diff --git a/test/recipes/70-test_sslversions.t b/test/recipes/70-test_sslversions.t index 5c9ee6c67f..8ef85af7a9 100644 --- a/test/recipes/70-test_sslversions.t +++ b/test/recipes/70-test_sslversions.t @@ -18,7 +18,8 @@ use constant { NO_EXTENSION => 3, EMPTY_EXTENSION => 4, TLS1_1_AND_1_0_ONLY => 5, - WITH_TLS1_4 => 6 + WITH_TLS1_4 => 6, + BAD_LEGACY_VERSION => 7 }; my $testtype; @@ -55,7 +56,7 @@ my $proxy = TLSProxy::Proxy->new( $testtype = EMPTY_EXTENSION; $proxy->filter(\&modify_supported_versions_filter); $proxy->start() or plan skip_all => "Unable to start up Proxy for tests"; -plan tests => 7; +plan tests => 8; ok(TLSProxy::Message->fail(), "Empty supported versions"); #Test 2: supported_versions extension with no recognised versions should not @@ -111,6 +112,12 @@ ok(TLSProxy::Message->success() && TLSProxy::Proxy->is_tls13(), "TLS1.4 in supported versions extension"); +#Test 8: Set the legacy version to SSLv3 with supported versions. Should fail +$proxy->clear(); +$testtype = BAD_LEGACY_VERSION; +$proxy->start(); +ok(TLSProxy::Message->fail(), "Legacy version is SSLv3 with supported versions"); + sub modify_supported_versions_filter { my $proxy = shift; @@ -165,14 +172,15 @@ sub modify_supported_versions_filter } elsif ($testtype == EMPTY_EXTENSION) { $message->set_extension( TLSProxy::Message::EXT_SUPPORTED_VERSIONS, ""); - } else { + } elsif ($testtype == NO_EXTENSION) { $message->delete_extension( TLSProxy::Message::EXT_SUPPORTED_VERSIONS); + } else { + # BAD_LEGACY_VERSION + $message->client_version(TLSProxy::Record::VERS_SSL_3_0); } $message->repack(); } } } - - -- 2.25.1