From 972ee925b16fc3bc7ec71080c439e669754235ab Mon Sep 17 00:00:00 2001 From: Tim Perry Date: Tue, 16 Apr 2024 15:40:21 +0200 Subject: [PATCH] Use empty renegotiate extension instead of SCSV for TLS > 1.0 Reviewed-by: Tomas Mraz Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/24161) --- CHANGES.md | 6 ++ ssl/statem/extensions_clnt.c | 33 ++++++++- ssl/statem/statem_clnt.c | 5 +- test/recipes/70-test_renegotiation.t | 101 +++++++++++++++++++++++++-- test/recipes/70-test_sslextension.t | 1 + test/recipes/70-test_sslmessages.t | 2 +- test/recipes/70-test_tls13certcomp.t | 3 + test/recipes/70-test_tls13kexmodes.t | 6 ++ test/recipes/70-test_tls13messages.t | 6 ++ test/sslapitest.c | 6 +- 10 files changed, 153 insertions(+), 16 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 76801ac78c..aaa47976a2 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -41,6 +41,12 @@ OpenSSL 3.4 * Tomas Mraz* + * Use an empty renegotiate extension in TLS client hellos instead of + the empty renegotiation SCSV, for all connections with a minimum TLS + version > 1.0. + + *Tim Perry* + OpenSSL 3.3 ----------- diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index 381a6c9d7b..77fe629132 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -16,10 +16,37 @@ EXT_RETURN tls_construct_ctos_renegotiate(SSL_CONNECTION *s, WPACKET *pkt, unsigned int context, X509 *x, size_t chainidx) { - /* Add RI if renegotiating */ - if (!s->renegotiate) - return EXT_RETURN_NOT_SENT; + if (!s->renegotiate) { + /* If not renegotiating, send an empty RI extension to indicate support */ +#if DTLS_MAX_VERSION_INTERNAL != DTLS1_2_VERSION +# error Internal DTLS version error +#endif + + if (!SSL_CONNECTION_IS_DTLS(s) + && (s->min_proto_version >= TLS1_3_VERSION + || (ssl_security(s, SSL_SECOP_VERSION, 0, TLS1_VERSION, NULL) + && s->min_proto_version <= TLS1_VERSION))) { + /* + * For TLS <= 1.0 SCSV is used instead, and for TLS 1.3 this + * extension isn't used at all. + */ + return EXT_RETURN_NOT_SENT; + } + + + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_renegotiate) + || !WPACKET_start_sub_packet_u16(pkt) + || !WPACKET_put_bytes_u8(pkt, 0) + || !WPACKET_close(pkt)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + return EXT_RETURN_FAIL; + } + + return EXT_RETURN_SENT; + } + + /* Add a complete RI extension if renegotiating */ if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_renegotiate) || !WPACKET_start_sub_packet_u16(pkt) || !WPACKET_sub_memcpy_u8(pkt, s->s3.previous_client_finished, diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 7d8b140373..6f73d5f698 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -4064,8 +4064,9 @@ int ssl_cipher_list_to_bytes(SSL_CONNECTION *s, STACK_OF(SSL_CIPHER) *sk, int i; size_t totlen = 0, len, maxlen, maxverok = 0; int empty_reneg_info_scsv = !s->renegotiate - && (SSL_CONNECTION_IS_DTLS(s) - || s->min_proto_version < TLS1_3_VERSION); + && !SSL_CONNECTION_IS_DTLS(s) + && ssl_security(s, SSL_SECOP_VERSION, 0, TLS1_VERSION, NULL) + && s->min_proto_version <= TLS1_VERSION; SSL *ssl = SSL_CONNECTION_GET_SSL(s); /* Set disabled masks for this session */ diff --git a/test/recipes/70-test_renegotiation.t b/test/recipes/70-test_renegotiation.t index 37fbfd5854..445d447dc9 100644 --- a/test/recipes/70-test_renegotiation.t +++ b/test/recipes/70-test_renegotiation.t @@ -7,6 +7,7 @@ # https://www.openssl.org/source/license.html use strict; +use List::Util 'first'; use OpenSSL::Test qw/:DEFAULT cmdstr srctop_file bldtop_dir/; use OpenSSL::Test::Utils; use TLSProxy::Proxy; @@ -26,7 +27,7 @@ plan skip_all => "$test_name needs the sock feature enabled" plan skip_all => "$test_name needs TLS <= 1.2 enabled" if alldisabled(("ssl3", "tls1", "tls1_1", "tls1_2")); -plan tests => 5; +plan tests => 9; my $proxy = TLSProxy::Proxy->new( undef, @@ -42,19 +43,35 @@ $proxy->reneg(1); $proxy->start() or plan skip_all => "Unable to start up Proxy for tests"; ok(TLSProxy::Message->success(), "Basic renegotiation"); -#Test 2: Client does not send the Reneg SCSV. Reneg should fail +#Test 2: Seclevel 0 client does not send the Reneg SCSV. Reneg should fail $proxy->clear(); -$proxy->filter(\&reneg_filter); +$proxy->filter(\&reneg_scsv_filter); +$proxy->cipherc("DEFAULT:\@SECLEVEL=0"); $proxy->clientflags("-no_tls1_3"); $proxy->serverflags("-client_renegotiation"); $proxy->reneg(1); $proxy->start(); ok(TLSProxy::Message->fail(), "No client SCSV"); +SKIP: { + skip "TLSv1.2 disabled", 1 + if disabled("tls1_2"); + + #Test 3: TLS 1.2 client does not send the Reneg extension. Reneg should fail + + $proxy->clear(); + $proxy->filter(\&reneg_ext_filter); + $proxy->clientflags("-no_tls1_3"); + $proxy->serverflags("-client_renegotiation"); + $proxy->reneg(1); + $proxy->start(); + ok(TLSProxy::Message->fail(), "No client extension"); +} + SKIP: { skip "TLSv1.2 or TLSv1.1 disabled", 1 if disabled("tls1_2") || disabled("tls1_1"); - #Test 3: Check that the ClientHello version remains the same in the reneg + #Test 4: Check that the ClientHello version remains the same in the reneg # handshake $proxy->clear(); $proxy->filter(undef); @@ -84,7 +101,7 @@ SKIP: { skip "TLSv1.2 disabled", 1 if disabled("tls1_2"); - #Test 4: Test for CVE-2021-3449. client_sig_algs instead of sig_algs in + #Test 5: Test for CVE-2021-3449. client_sig_algs instead of sig_algs in # resumption ClientHello $proxy->clear(); $proxy->filter(\&sigalgs_filter); @@ -98,7 +115,7 @@ SKIP: { SKIP: { skip "TLSv1.2 and TLSv1.1 disabled", 1 if disabled("tls1_2") && disabled("tls1_1"); - #Test 5: Client fails to do renegotiation + #Test 6: Client fails to do renegotiation $proxy->clear(); $proxy->filter(undef); $proxy->serverflags("-no_tls1_3"); @@ -109,7 +126,60 @@ SKIP: { "Check client renegotiation failed"); } -sub reneg_filter +SKIP: { + skip "TLSv1 disabled", 1 + if disabled("tls1"); + + #Test 7: Check that SECLEVEL 0 sends SCSV not RI extension + $proxy->clear(); + $proxy->filter(undef); + $proxy->cipherc("DEFAULT:\@SECLEVEL=0"); + $proxy->start(); + + my $clientHello = first { $_->mt == TLSProxy::Message::MT_CLIENT_HELLO } @{$proxy->message_list}; + my $has_scsv = 255 ~~ @{$clientHello->ciphersuites}; + my $has_ri_extension = exists $clientHello->extension_data()->{TLSProxy::Message::EXT_RENEGOTIATE}; + + ok($has_scsv && !$has_ri_extension, "SECLEVEL=0 should use SCSV not RI extension by default"); +} + +SKIP: { + skip "TLSv1.2 disabled", 1 + if disabled("tls1_2"); + + #Test 8: Check that SECLEVEL0 + TLS 1.2 sends RI extension not SCSV + $proxy->clear(); + $proxy->filter(undef); + $proxy->cipherc("DEFAULT:\@SECLEVEL=0"); + $proxy->clientflags("-tls1_2"); + $proxy->start(); + + my $clientHello = first { $_->mt == TLSProxy::Message::MT_CLIENT_HELLO } @{$proxy->message_list}; + my $has_scsv = 255 ~~ @{$clientHello->ciphersuites}; + my $has_ri_extension = exists $clientHello->extension_data()->{TLSProxy::Message::EXT_RENEGOTIATE}; + + ok(!$has_scsv && $has_ri_extension, "TLS1.2 should use RI extension despite SECLEVEL=0"); +} + + +SKIP: { + skip "TLSv1.3 disabled", 1 + if disabled("tls1_3"); + + #Test 9: Check that TLS 1.3 sends neither RI extension nor SCSV + $proxy->clear(); + $proxy->filter(undef); + $proxy->clientflags("-tls1_3"); + $proxy->start(); + + my $clientHello = first { $_->mt == TLSProxy::Message::MT_CLIENT_HELLO } @{$proxy->message_list}; + my $has_scsv = 255 ~~ @{$clientHello->ciphersuites}; + my $has_ri_extension = exists $clientHello->extension_data()->{TLSProxy::Message::EXT_RENEGOTIATE}; + + ok(!$has_scsv && !$has_ri_extension, "TLS1.3 should not use RI extension or SCSV"); +} + +sub reneg_scsv_filter { my $proxy = shift; @@ -129,6 +199,23 @@ sub reneg_filter } } +sub reneg_ext_filter +{ + my $proxy = shift; + + # We're only interested in the initial ClientHello message + if ($proxy->flight != 0) { + return; + } + + foreach my $message (@{$proxy->message_list}) { + if ($message->mt == TLSProxy::Message::MT_CLIENT_HELLO) { + $message->delete_extension(TLSProxy::Message::EXT_RENEGOTIATE); + $message->repack(); + } + } +} + sub sigalgs_filter { my $proxy = shift; diff --git a/test/recipes/70-test_sslextension.t b/test/recipes/70-test_sslextension.t index 37fba871e9..5218d5ff94 100644 --- a/test/recipes/70-test_sslextension.t +++ b/test/recipes/70-test_sslextension.t @@ -206,6 +206,7 @@ SKIP: { #Test 3: Sending a zero length extension block should pass $proxy->clear(); $proxy->filter(\&extension_filter); + $proxy->cipherc("DEFAULT:\@SECLEVEL=0"); $proxy->ciphers("AES128-SHA:\@SECLEVEL=0"); $proxy->clientflags("-no_tls1_3"); $proxy->start(); diff --git a/test/recipes/70-test_sslmessages.t b/test/recipes/70-test_sslmessages.t index 0afb700679..cad0147ab5 100644 --- a/test/recipes/70-test_sslmessages.t +++ b/test/recipes/70-test_sslmessages.t @@ -128,7 +128,7 @@ my $proxy = TLSProxy::Proxy->new( checkhandshake::DEFAULT_EXTENSIONS], [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_RENEGOTIATE, TLSProxy::Message::CLIENT, - checkhandshake::RENEGOTIATE_CLI_EXTENSION], + checkhandshake::DEFAULT_EXTENSIONS], [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_NPN, TLSProxy::Message::CLIENT, checkhandshake::NPN_CLI_EXTENSION], diff --git a/test/recipes/70-test_tls13certcomp.t b/test/recipes/70-test_tls13certcomp.t index bc960c8b37..e2d65bd87c 100644 --- a/test/recipes/70-test_tls13certcomp.t +++ b/test/recipes/70-test_tls13certcomp.t @@ -109,6 +109,9 @@ plan skip_all => "$test_name needs compression and algorithms enabled" [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_COMPRESS_CERTIFICATE, TLSProxy::Message::CLIENT, checkhandshake::CERT_COMP_CLI_EXTENSION], + [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_RENEGOTIATE, + TLSProxy::Message::CLIENT, + checkhandshake::DEFAULT_EXTENSIONS], [TLSProxy::Message::MT_SERVER_HELLO, TLSProxy::Message::EXT_SUPPORTED_VERSIONS, TLSProxy::Message::SERVER, diff --git a/test/recipes/70-test_tls13kexmodes.t b/test/recipes/70-test_tls13kexmodes.t index 1f45edc7b7..738f2dcf7c 100644 --- a/test/recipes/70-test_tls13kexmodes.t +++ b/test/recipes/70-test_tls13kexmodes.t @@ -102,6 +102,9 @@ plan skip_all => "$test_name needs EC enabled" [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_PSK, TLSProxy::Message::CLIENT, checkhandshake::PSK_CLI_EXTENSION], + [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_RENEGOTIATE, + TLSProxy::Message::CLIENT, + checkhandshake::DEFAULT_EXTENSIONS], [TLSProxy::Message::MT_SERVER_HELLO, TLSProxy::Message::EXT_SUPPORTED_VERSIONS, TLSProxy::Message::SERVER, @@ -152,6 +155,9 @@ plan skip_all => "$test_name needs EC enabled" [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_PSK, TLSProxy::Message::CLIENT, checkhandshake::PSK_CLI_EXTENSION], + [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_RENEGOTIATE, + TLSProxy::Message::CLIENT, + checkhandshake::DEFAULT_EXTENSIONS], [TLSProxy::Message::MT_SERVER_HELLO, TLSProxy::Message::EXT_SUPPORTED_VERSIONS, TLSProxy::Message::SERVER, diff --git a/test/recipes/70-test_tls13messages.t b/test/recipes/70-test_tls13messages.t index f579cd3c9f..f8b5bf9663 100644 --- a/test/recipes/70-test_tls13messages.t +++ b/test/recipes/70-test_tls13messages.t @@ -105,6 +105,9 @@ plan skip_all => "$test_name needs EC enabled" [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_POST_HANDSHAKE_AUTH, TLSProxy::Message::CLIENT, checkhandshake::POST_HANDSHAKE_AUTH_CLI_EXTENSION], + [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_RENEGOTIATE, + TLSProxy::Message::CLIENT, + checkhandshake::DEFAULT_EXTENSIONS], [TLSProxy::Message::MT_SERVER_HELLO, TLSProxy::Message::EXT_SUPPORTED_VERSIONS, TLSProxy::Message::SERVER, @@ -158,6 +161,9 @@ plan skip_all => "$test_name needs EC enabled" [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_POST_HANDSHAKE_AUTH, TLSProxy::Message::CLIENT, checkhandshake::POST_HANDSHAKE_AUTH_CLI_EXTENSION], + [TLSProxy::Message::MT_CLIENT_HELLO, TLSProxy::Message::EXT_RENEGOTIATE, + TLSProxy::Message::CLIENT, + checkhandshake::DEFAULT_EXTENSIONS], [TLSProxy::Message::MT_SERVER_HELLO, TLSProxy::Message::EXT_SUPPORTED_VERSIONS, TLSProxy::Message::SERVER, diff --git a/test/sslapitest.c b/test/sslapitest.c index 0b2d7b5e6d..ce8f642802 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -713,14 +713,14 @@ static int full_client_hello_callback(SSL *s, int *al, void *arg) int *ctr = arg; const unsigned char *p; int *exts; - /* We only configure two ciphers, but the SCSV is added automatically. */ #ifdef OPENSSL_NO_EC - const unsigned char expected_ciphers[] = {0x00, 0x9d, 0x00, 0xff}; + const unsigned char expected_ciphers[] = {0x00, 0x9d}; #else const unsigned char expected_ciphers[] = {0x00, 0x9d, 0xc0, - 0x2c, 0x00, 0xff}; + 0x2c}; #endif const int expected_extensions[] = { + 65281, #ifndef OPENSSL_NO_EC 11, 10, #endif