Use empty renegotiate extension instead of SCSV for TLS > 1.0

Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #24161)
openssl · Apr 22, 2024 · 972ee92 · 972ee92
1 parent 6ee369c
commit 972ee92
Show file tree

Hide file tree

Showing 10 changed files with 153 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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