From 426dfc9ff7c1afaf1ed5981a9c7846e310c7ae3e Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 5 Dec 2017 10:16:25 +0000 Subject: [PATCH] Send supported_versions in an HRR Reviewed-by: Ben Kaduk (Merged from https://github.com/openssl/openssl/pull/4701) --- crypto/err/openssl.txt | 1 + include/openssl/sslerr.h | 1 + ssl/ssl_err.c | 1 + ssl/statem/extensions_clnt.c | 15 +++++++++++++++ ssl/statem/statem_srvr.c | 2 +- test/recipes/70-test_tls13kexmodes.t | 2 ++ test/recipes/70-test_tls13messages.t | 2 ++ util/perl/TLSProxy/ServerHello.pm | 7 ++++--- 8 files changed, 27 insertions(+), 4 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index af826cd10f..872016f350 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -2361,6 +2361,7 @@ SSL_R_BAD_EXTENSION:110:bad extension SSL_R_BAD_HANDSHAKE_LENGTH:332:bad handshake length SSL_R_BAD_HANDSHAKE_STATE:236:bad handshake state 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_LENGTH:271:bad length diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index 9564023044..2986be84ad 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -444,6 +444,7 @@ int ERR_load_SSL_strings(void); # define SSL_R_BAD_HANDSHAKE_LENGTH 332 # define SSL_R_BAD_HANDSHAKE_STATE 236 # define SSL_R_BAD_HELLO_REQUEST 105 +# 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_LENGTH 271 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 08f6696163..1e3eb2cc72 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -706,6 +706,7 @@ static const ERR_STRING_DATA SSL_str_reasons[] = { {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_HANDSHAKE_STATE), "bad handshake state"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_BAD_HELLO_REQUEST), "bad hello request"}, + {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_LENGTH), "bad length"}, diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index 1fbf9f6e0e..f357396d81 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -1657,6 +1657,21 @@ int tls_parse_stoc_supported_versions(SSL *s, PACKET *pkt, unsigned int context, if (version == TLS1_3_VERSION_DRAFT) version = TLS1_3_VERSION; + /* We ignore this extension for HRRs except to sanity check it */ + if (context == SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST) { + /* + * The only protocol version we support which has an HRR message is + * TLSv1.3, therefore we shouldn't be getting an HRR for anything else. + */ + if (version != TLS1_3_VERSION) { + *al = SSL_AD_PROTOCOL_VERSION; + SSLerr(SSL_F_TLS_PARSE_STOC_SUPPORTED_VERSIONS, + SSL_R_BAD_HRR_VERSION); + return 0; + } + return 1; + } + /* We just set it here. We validate it in ssl_choose_client_version */ s->version = version; diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 7d1d15dcc1..4f0487cc0f 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -2274,7 +2274,7 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt) compm = s->s3->tmp.new_compression->id; #endif - if (!WPACKET_sub_memcpy_u8(pkt, session_id, sl) + if (!WPACKET_sub_memcpy_u8(pkt, session_id, sl) || !s->method->put_cipher_by_char(s->s3->tmp.new_cipher, pkt, &len) || !WPACKET_put_bytes_u8(pkt, compm) || !tls_construct_extensions(s, pkt, diff --git a/test/recipes/70-test_tls13kexmodes.t b/test/recipes/70-test_tls13kexmodes.t index 908ca4a21d..7afb56092b 100644 --- a/test/recipes/70-test_tls13kexmodes.t +++ b/test/recipes/70-test_tls13kexmodes.t @@ -90,6 +90,8 @@ $ENV{CTLOG_FILE} = srctop_file("test", "ct", "log_list.conf"); [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_PSK, checkhandshake::PSK_CLI_EXTENSION], + [TLSProxy::Message::MT_SERVER_HELLO, TLSProxy::Message::EXT_SUPPORTED_VERSIONS, + checkhandshake::DEFAULT_EXTENSIONS], [TLSProxy::Message::MT_SERVER_HELLO, TLSProxy::Message::EXT_KEY_SHARE, checkhandshake::KEY_SHARE_HRR_EXTENSION], diff --git a/test/recipes/70-test_tls13messages.t b/test/recipes/70-test_tls13messages.t index 4b0552c5ca..2cf822aca0 100644 --- a/test/recipes/70-test_tls13messages.t +++ b/test/recipes/70-test_tls13messages.t @@ -90,6 +90,8 @@ $ENV{CTLOG_FILE} = srctop_file("test", "ct", "log_list.conf"); [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_PSK, checkhandshake::PSK_CLI_EXTENSION], + [TLSProxy::Message::MT_SERVER_HELLO, TLSProxy::Message::EXT_SUPPORTED_VERSIONS, + checkhandshake::DEFAULT_EXTENSIONS], [TLSProxy::Message::MT_SERVER_HELLO, TLSProxy::Message::EXT_KEY_SHARE, checkhandshake::KEY_SHARE_HRR_EXTENSION], diff --git a/util/perl/TLSProxy/ServerHello.pm b/util/perl/TLSProxy/ServerHello.pm index 693a652b6c..934eaf4dea 100644 --- a/util/perl/TLSProxy/ServerHello.pm +++ b/util/perl/TLSProxy/ServerHello.pm @@ -50,6 +50,7 @@ sub parse my $self = shift; my $ptr = 2; my ($server_version) = unpack('n', $self->data); + my $neg_version = $server_version; my $random = substr($self->data, $ptr, 32); $ptr += 32; @@ -94,15 +95,15 @@ sub parse $extension_data = substr($extension_data, 4 + $size); $extensions{$type} = $extdata; if ($type == TLSProxy::Message::EXT_SUPPORTED_VERSIONS) { - $server_version = unpack('n', $extdata); + $neg_version = unpack('n', $extdata); } } if ($random eq $hrrrandom) { TLSProxy::Proxy->is_tls13(1); # TODO(TLS1.3): Replace this reference to draft version before release - } elsif ($server_version == TLSProxy::Record::VERS_TLS_1_3_DRAFT) { - $server_version = TLSProxy::Record::VERS_TLS_1_3; + } elsif ($neg_version == TLSProxy::Record::VERS_TLS_1_3_DRAFT) { + $neg_version = TLSProxy::Record::VERS_TLS_1_3; TLSProxy::Proxy->is_tls13(1); TLSProxy::Record->server_encrypting(1); -- 2.25.1